-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log protocol messages at micro level #139
Log protocol messages at micro level #139
Conversation
A few comments above but overall lgtm. |
ce92eb4
to
d39c5e6
Compare
@paddybyers & @SimonWoolf I think I have addressed all your comments + I refactored the way we do the REST logging because it was not decoding MsgPack payloads. Mind reviewing the recent commits again? Also, can you please advise how you would like me to handle 4724f17. Should I remove the unrelated code for now? |
return '[ ' + result.join(', ') + ' ]'; | ||
} | ||
|
||
ProtocolMessage.stringify = function(msg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not .prototype.toString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this a class method, not an instance method. We are using the ProtocolMessage
class method to convert a JSON object into a string representation. When we log the messages, they are rarely ProtocolMessage
s
Do your other changes depend on it? If not, then we should handle it as a separate PR. |
@SimonWoolf can you confirm that the logging will break when using MsgPack encrypted payloads in a browser if we don't merge this in? If so, I would like to merge and then add a separate issue to review. |
Yes, the binary crypto tests will all break. |
Ok, I will merge in as is for now and raise a separate issue that @paddybyers can look at it if he wishes. |
…mance * Converts action into a human readable value i.e. MESSAGE instead of 15. * Logs outgoing & incoming ProtocolMessages at the MICRO level * Will not serialise objects if the log level is not MICRO by wrapping these expensive operations with `shouldLog(level)`
When logging messages within the Http modules, the payloads are still encrypted. It is simpler to add this logging to the centrally used Resource module instead.
Can use: * ABLY_LOG_LEVEL=4 grunt test:nodeunit * grunt test:web server and then http://localhost:3000/nodeunit.html?log_level=4 Additionally small env var fixes + documentation fixes
bbe23fd
to
a7acd40
Compare
@paddybyers and @SimonWoolf please review this PR as I find it quite difficult to understand where failures are occurring over Realtime and REST when using the
ably-js
library.I would like to merge this in if possible so that I can add the necessary logging easily from the realtime QOS tests that are failing.