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
feat: add client logging with slf4j #1586
Changes from all 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
3c65d45
5666430
7f882d8
9e9af89
11cbc54
692080d
fb5142a
7069230
fa7d4a0
314123c
5d6eeb0
084fa17
71fd705
ee3643d
5bcd4ee
7c95364
23712d0
d979f60
a3bb3e4
23844c9
821826d
0fd818d
3b8205d
e1d9a78
b8caf5a
eaca20b
420e0ec
f708d91
ae7e2b7
5f5fb09
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.
I think logging here makes sense, I am just wondering if it would make sense to have the Credential pass the logger in here. I don't know what the standard would be and I'll defer to you.
I think this would currently show up as something like:
IamUtils.class - Sending request to get signature to sign the blob
The concern that pops up is that IamUtils is an internal class and doesn't tell the user which Credentials is actually access the
sign
call.Do you think it would be better as:
ComputeEngineCredentials - Sending request to get signature to sign the blob
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.
hmm, usually the convention is to logging in the class where it occurs. Although to your point, we can probably add another log entry before entering the
IamUtils
methods, but I am not sure if that's too much details?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.
Ok gotcha I see. In the case I think it's fine. Maybe if anything it could be something like
Sending request to get signature to sign the blob for %s, Credentials.getClass())
. That also might be overkill. I think we can keep as is and improve it from user feedback.