Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add client logging with slf4j #1586
base: main
Are you sure you want to change the base?
feat: add client logging with slf4j #1586
Changes from 10 commits
c38282b
3f30267
35d9a75
a63d6fe
83329e7
6a5d3d3
930d0aa
44f3de8
598d5b9
445fe08
a7f5a5e
8bc1ae7
bde6ede
7c89c3f
6da450d
9e20be9
db99e70
68307fc
4202902
cfbe873
858b725
fb8889a
15d3016
c2a8d76
6485526
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: Could the message to be more descriptive? i.e. Signature to sign the blob or something that better describes this
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 does this need to be cleared?
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.
MDC carries contextual information in log messages. It is tied to thread, and is safer to clear it to avoid it be carried over to other log entries.
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.
I see. Can you add that as a comment? Or a link to a guide that tells us clear it so future readers?
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.
I think it would be good to duplicate information like
request.method
andrequest.url
in message instead of always relying on MDC, as we discussed yesterday. It would be great if we can make this change in this release. However, if not, it should not be a blocker either, as adding these info later is not a breaking change.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.
It's already done. (9e20be9)
It is verified in LoggingTest (here you can see the message is parsed then verified. The test is not extensive in verifying information. )
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.
In general, I think we should not try to call these methods if logging is not enabled due to performance concerns, similar to the comment in sdk-platform-java.
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.
nit: Can we change this to be something like
logResponsePayload
andparseResponsePayload
so it's clearer as to what it's doing?