CDRIVER-4485 string append and truncation fixes for structured logging #1826

ghost · 2025-01-15T19:53:04Z

This PR is a follow-up for #1795 which depends on the new internal string append and json building API from #1816. (Update: Now addresses compatibility with #1821 also.)

Refines the concept of "maximum max document length" internally as a specific value near INT_MAX, to account for both ellipsis and bson overhead. Currently we choose not to expose the specific value of this constant publicly.
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.

…tring APIs Adds the truncation marker, defines string limits carefully to avoid bson length overflow or int overflow

… spec

PR #1821, merged as commit d08d455, included a TODO for this truncation refactor. Updated to use the new internal function _mongoc_structured_log_document_as_truncated_json, and to ASSERT that the length is now guaranteed not to overflow on cast.

return an error in the event the sequence has < 4 bytes of unexpected data at the end. previously, this extra data was silently ignored.

src/libmongoc/doc/mongoc_structured_log_opts_get_max_document_length.rst

src/libmongoc/doc/mongoc_structured_log_opts_set_max_document_length.rst

src/libmongoc/src/mongoc/mongoc-structured-log.c

src/libmongoc/tests/test-mongoc-command-logging-and-monitoring.c

src/libmongoc/tests/test-mongoc-structured-log.c

src/libmongoc/src/mongoc/mongoc-structured-log.c

…length.rst Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

Expect a very slight microarchitectural speed boost if this path is ever hot.

eramongodb

A couple more suggestions; otherwise, LGTM.

src/libmongoc/doc/mongoc_structured_log_opts_get_max_document_length.rst

src/libmongoc/tests/test-mongoc-structured-log.c

…length.rst Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

src/libmongoc/tests/test-mongoc-structured-log.c

…API change

src/libmongoc/tests/test-mongoc-structured-log.c

eramongodb

One last suggestion; otherwise, LGTM. 👍

eramongodb · 2025-01-17T19:05:17Z

src/libmongoc/tests/test-mongoc-structured-log.c

@@ -142,6 +143,7 @@ test_structured_log_plain (void)

   mongoc_structured_log_opts_t *opts = mongoc_structured_log_opts_new ();
   mongoc_structured_log_opts_set_handler (opts, structured_log_func, &assumption);
+   ASSERT (mongoc_structured_log_opts_set_max_document_length (opts, 10000));


Suggested change

ASSERT (mongoc_structured_log_opts_set_max_document_length (opts, 10000));

ASSERT (

mongoc_structured_log_opts_set_max_document_length (opts, MONGOC_STRUCTURED_LOG_DEFAULT_MAX_DOCUMENT_LENGTH));

Should these values of 10000 be MONGOC_STRUCTURED_LOG_DEFAULT_MAX_DOCUMENT_LENGTH (1000) instead?

The specific value doesn't really matter as long as it's more than the small lengths used in the test, but my intent was to follow the advice from the CLAM spec:

"Drivers MUST run the logging tests with their max document
length setting (as described in the logging specification)
set to a large value e.g. 10,000; this is necessary in order for the driver to emit the full server reply (and to allow
matching against that reply) on certain MongoDB versions and topologies."

It's mostly arbitrary but I'll add a comment explaining the choice just so it doesn't seem out of place.

Micah Scott added 4 commits January 15, 2025 07:51

structured logging json and truncation fixes, using the new mcommon_s…

e214629

…tring APIs Adds the truncation marker, defines string limits carefully to avoid bson length overflow or int overflow

Add libmongoc API control for MONGODB_LOG_MAX_DOCUMENT_LENGTH setting

c0ac63b

Unified tests use the suggested max document length of 10000 from the…

ba40de3

… spec

command-logging-and-monitoring prose tests

f616c88

ghost requested review from adriandole and eramongodb January 15, 2025 19:53

Micah Scott added 7 commits January 15, 2025 12:18

Remove extra :man_page: tag

c386923

Mark prose tests as requiring a live server

8bde27e

Check more API return values in CLAM logging prose tests

a940fc4

Merge branch 'master' into CDRIVER-4485-b

e71b3ea

Update for compatibility with #1821

e58dc1e

PR #1821, merged as commit d08d455, included a TODO for this truncation refactor. Updated to use the new internal function _mongoc_structured_log_document_as_truncated_json, and to ASSERT that the length is now guaranteed not to overflow on cast.

Rename local to avoid shadowing

fe5f948

additional document sequence validation

9bafef3

return an error in the event the sequence has < 4 bytes of unexpected data at the end. previously, this extra data was silently ignored.

eramongodb requested changes Jan 16, 2025

View reviewed changes

mdbmes and others added 8 commits January 16, 2025 11:29

Update src/libmongoc/doc/mongoc_structured_log_opts_get_max_document_…

7333080

…length.rst Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

Update src/libmongoc/doc/mongoc_structured_log_opts_set_max_document_…

039a78a

…length.rst Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

Update src/libmongoc/src/mongoc/mongoc-structured-log.c

9e71dee

Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

see also

c4f8167

Remove unintended printf

c4c6d16

Replace counter with flag for env error guards

90909e7

Expect a very slight microarchitectural speed boost if this path is ever hot.

Explicit requirement for unset env option

c96b9d3

comment correction, not using the mock server

7654e67

ghost requested a review from eramongodb January 16, 2025 21:29

eramongodb approved these changes Jan 16, 2025

View reviewed changes

src/libmongoc/doc/mongoc_structured_log_opts_get_max_document_length.rst Outdated Show resolved Hide resolved

src/libmongoc/tests/test-mongoc-structured-log.c Outdated Show resolved Hide resolved

mdbmes and others added 2 commits January 16, 2025 13:39

Update src/libmongoc/doc/mongoc_structured_log_opts_get_max_document_…

78634de

…length.rst Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>

getenv fixes, and skip testing env defaults when external values are set

b2f6d52

eramongodb requested changes Jan 17, 2025

View reviewed changes

Improved test skip for structured log env defaults, revert TestSuite …

989e2e4

…API change

eramongodb reviewed Jan 17, 2025

View reviewed changes

src/libmongoc/tests/test-mongoc-structured-log.c Show resolved Hide resolved

src/libmongoc/tests/test-mongoc-structured-log.c Outdated Show resolved Hide resolved

Avoid environment dependencies

fae30e3

ghost requested a review from eramongodb January 17, 2025 19:01

eramongodb approved these changes Jan 17, 2025

View reviewed changes

Add comment to explain where this max_document_length comes from

aae1bc1

adriandole approved these changes Jan 22, 2025

View reviewed changes

ghost merged commit ee16861 into mongodb:master Jan 22, 2025
45 checks passed

ghost deleted the CDRIVER-4485-b branch January 23, 2025 01:49

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CDRIVER-4485 string append and truncation fixes for structured logging #1826

CDRIVER-4485 string append and truncation fixes for structured logging #1826

Uh oh!

ghost commented Jan 15, 2025 •

edited by ghost

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb left a comment

Uh oh!

eramongodb Jan 17, 2025

Uh oh!

ghost Jan 17, 2025

Uh oh!

Uh oh!

Uh oh!

	ASSERT (mongoc_structured_log_opts_set_max_document_length (opts, 10000));
	ASSERT (
	mongoc_structured_log_opts_set_max_document_length (opts, MONGOC_STRUCTURED_LOG_DEFAULT_MAX_DOCUMENT_LENGTH));

CDRIVER-4485 string append and truncation fixes for structured logging #1826

CDRIVER-4485 string append and truncation fixes for structured logging #1826

Uh oh!

Conversation

ghost commented Jan 15, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

eramongodb Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Jan 15, 2025 •

edited by ghost

Loading

eramongodb left a comment •

edited

Loading