Closed Bug 955526 Opened 6 years ago Closed 6 years ago

Debug logs are binary only

Categories

(Chat Core :: Yahoo! Messenger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: qheaden)

References

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 2089 at 2013-08-02 11:42:00 UTC ***

It might be useful to add some human-readable debug logging: e.g. when a message is sent, include a log entry with the message in plaintext rather than binary.

You may WONTFIX this, but I thought it was worth discussing.
*** Original post on bio 2089 at 2013-09-03 16:41:25 UTC ***

Debug logs also include NO incoming data at all. (Apart from the logging the socket does.) This makes them fairly useless.
Blocks: 955574
Attached patch Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2089 as attmnt 2822 at 2013-09-04 20:49:00 UTC ***

This patch adds human readable logging for incoming and outgoing packets. The log messages contain info about the packet's service type, status number, session ID, and the key/value pair data that is contained within it.
Attachment #8354592 - Flags: review?(clokep)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
Comment on attachment 8354592 [details] [diff] [review]
Patch 1

*** Original change on bio 2089 attmnt 2822 at 2013-09-05 02:28:06 UTC ***

>diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
>@@ -167,25 +167,25 @@ YahooSession.prototype = {
>-    this.sendBinaryData(packet.toArrayBuffer());
>+    this.sendBinaryData(packet.toArrayBuffer(), packet.toString());
Code duplication alert! Let's add a sendPacket method that calls sendBinaryData(packet.toArrayBuffer(), packet.toString())

>@@ -756,16 +757,29 @@ YahooPacket.prototype = {
>+  toString: function() {
>+    let s = "";
Just set s = to the first line to start with.

>+    // First, add packet header information.
>+    s += "Service: " + this.service.toString(16) + "\n";
>+    s += "Status: " + this.status.toString(16) + "\n";
>+    s += "Session ID: " + this.sessionId.toString(16) + "\n\n";
These should probably have a 0x in front of them if they're hex.

>+    // Now we add the packet data.
>+    s += "Packet Key-Value Data:\n";
>+    for each (let pair in this.keyValuePairs)
>+      s += pair.key + ":\t" + pair.value + "\n";
What if there are no key-value pairs? Can that happen?
Attachment #8354592 - Flags: review?(clokep) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2089 as attmnt 2827 at 2013-09-05 16:41:00 UTC ***

This patch addresses the previous review points.
Attachment #8354597 - Flags: review?(clokep)
Comment on attachment 8354592 [details] [diff] [review]
Patch 1

*** Original change on bio 2089 attmnt 2822 at 2013-09-05 16:41:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354592 - Attachment is obsolete: true
Comment on attachment 8354597 [details] [diff] [review]
Patch 2

*** Original change on bio 2089 attmnt 2827 at 2013-09-05 16:51:15 UTC ***

My only comment would be that it's kind of weird that there will be two hanging \n if there are no key-value pairs. I don't have a strong feeling against this though.

Let's give this a try, if we think we can improve it later we'll do that.
Attachment #8354597 - Flags: review?(clokep) → review+
Attached patch Patch 3Splinter Review
*** Original post on bio 2089 as attmnt 2835 at 2013-09-06 05:48:00 UTC ***

Made some small fixes as per the conversation here: http://log.bezut.info/instantbird/130906/#m6. I also addressed the feedback given in comment 5. Since Florian brought up the suggestion in the IRC, I've given him the review this time.
Attachment #8354605 - Flags: review?(florian)
Comment on attachment 8354597 [details] [diff] [review]
Patch 2

*** Original change on bio 2089 attmnt 2827 at 2013-09-06 05:48:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354597 - Attachment is obsolete: true
Comment on attachment 8354605 [details] [diff] [review]
Patch 3

*** Original change on bio 2089 attmnt 2835 at 2013-09-06 10:27:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354605 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2089 at 2013-09-09 23:35:41 UTC ***

Thanks! http://hg.instantbird.org/instantbird/rev/7bdbfa0b739f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.