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
GODRIVER-2572 Add log messages to Server selection spec #1325
GODRIVER-2572 Add log messages to Server selection spec #1325
Changes from 48 commits
7b46fe0
7bde5ae
f7abd3c
92cb1bf
4c95947
faca1cc
d7005b5
5f4f342
9a9122d
a0dfef3
c0c767f
85dbb56
2f73b7c
1c05011
4d75016
d8eaf6c
27d61c0
a7d6685
66bec31
4299250
af250a1
5848ad5
a333d3e
0696575
dcb8380
bdd4e8c
8d4cd4c
02b7657
8d91a00
e291f7c
b2bff57
f2e8a83
fa90017
5d96f7e
46b62be
ccc4072
990f508
6ae5e38
1541fee
4a4bb38
735b497
43e4c3a
7e86550
b8b84a9
ebf07b4
0fd270a
eaad735
2a1771b
a474c8c
2bc53da
adf86c7
cb497c0
3da9902
79894b8
906d534
ffbfe15
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'm concerned that the nested JSON created here will be difficult to use when output by a structured logger. Is there another format we can use to combine multiple points of info that works better when nested in JSON? Or maybe only produce JSON when the output is unstructured?
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.
Could you elaborate on what you mean by this: "difficult to use when output by a structured logger" ? The output is the type of selector, associated data, and the child selectors (which is only relevant in the composite case). Here is an example:
We could omit child selectors. I would argue that, in the context of logging, more information is better.
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.
My concern is actually that the server selector information is a string containing escaped JSON, rather than a JSON object. Consider the example:
That selector information would be more usable and readable as a JSON object.
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.
The specifications require this field to be a string. This representation seems reasonable to me. However, if you want to unescape the string we can build this data directly, similar to what we do with the Server.
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.
Since the spec requires it to be a string, the escaped JSON seems like a reasonable solution.