CDRIVER-4487 server selection logging #1821
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
January 9, 2025 06:54
from commit ccb981601f3400784d187bb583b5c3d1dac7a963 in the specifications repository
The other server_selection tests don't use the unified format.
This has an immense amount of plumbing relative to the actual functionality, due to the additional context we need about the operation that prompted server selection.
ghost
requested review from
eramongodb
requested changes
Jan 9, 2025
There was a problem hiding this comment.
[...] the logs need information about both low-level server selection progress and high-level operation context. I considered alternatives, including a thread-local stack allowing functions to append log items to logs issued by a callee. This explicit context structure was chosen in order to optimize for clarity at the expense of some verbosity.
I support this design decision.
Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>
Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>
eramongodb
approved these changes
Jan 10, 2025
Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>
ghost
mentioned this pull request
adriandole
approved these changes
Jan 15, 2025
|
Thanks for the reviews! |
ghost
pushed a commit
that referenced
this pull request
Jan 22, 2025
#1826) This PR is a follow-up for #1795, #1821, and #1816. The new internal string and json APIs are used to implement missing functionality and tests for the structured logging subsystem. * Adds public APIs for the max_document_length within a mongoc_structured_log_opts_t. * Addresses the remainder of CDRIVER-4485: JSON documents within structured log messages are truncated correctly according to the rules set out in the Logging specification. * Addresses the remainder of CDRIVER-4486 by implementing prose tests. * Addresses CDRIVER-4814. Command or payload data beyond the truncation limit no longer degrades logging performance. (New serializer for command-with-payload) * Unified tests use the suggested max document length of 10000 from the spec * Replace atomic counter with atomic flag for environment error guards * Tests for environment defaults now skip if relevant variables are set externally
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This patch addresses CDRIVER-4487: structured log messages for server selection.
Note:
Contents:
Much of the noise in here is related to that new mongoc_ss_log_context_t, required because the logs need information about both low-level server selection progress and high-level operation context. I considered alternatives, including a thread-local stack allowing functions to append log items to logs issued by a callee. This explicit context structure was chosen in order to optimize for clarity at the expense of some verbosity.
The mongoc_ss_log_context_t provides the "operation" name, defined in the spec as "The name of the operation for which a server is being selected. When server selection is being performed to select a server for a command, this MUST be the command name." In case operations are never expected to generate user-visible logs, like tests, the operation name is still required but it may be arbitrary. I considered making the entire mongoc_ss_log_context_t optional, but retained it as a required parameter to reduce the potential for omissions in non-testing paths. Some operations are not commands but nevertheless will be user-visible (mongoc_client_session_start_transaction) and in this case I chose the libmongoc API name as the "operation".
We might consider C99 compound literals for the mongoc_ss_log_context_t instances. So far I've opted not to, for simplicity and in order to keep the parameter list lengths under control.