Skip to content
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

DRIVERS-1677, DRIVERS-1673: Add general logging specification and command logging #1303

Merged
merged 45 commits into from Oct 14, 2022

Conversation

kmahar
Copy link
Contributor

@kmahar kmahar commented Sep 9, 2022

This is the first in a series of PRs for the logging spec.

This PR adds:

  • A new general logging specification describing requirements for how drivers implement logging and defining shared concepts other specs can use, e.g. common log levels and component names
  • Additions to the unified test format to support logging assertions (I know there is an existing PR that also adds a schema version 1.11; if that goes in first I will update to use 1.12 instead).
  • Rebrands the command monitoring spec as the command logging and monitoring spec (I considered CMAL but it felt like the acronym CLAM was too good to pass up...), and adds information about command logging, along with both unified format and prose tests.

Future PRs will add logging and tests for CMAP, SDAM, and server selection.

@kmahar kmahar force-pushed the DRIVERS-1204/logging-spec branch 3 times, most recently from 0fb43c7 to 1dcb013 Compare September 9, 2022 19:06
source/logging/logging.rst Show resolved Hide resolved
source/logging/logging.rst Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/logging/logging.rst Show resolved Hide resolved
@kmahar kmahar marked this pull request as ready for review September 9, 2022 19:25
@kmahar kmahar requested review from a team as code owners September 9, 2022 19:25
@kmahar kmahar requested review from jmikola, durran, alcaeus, jyemin and BorisDog and removed request for a team and alcaeus September 9, 2022 19:25
source/command-logging-and-monitoring/tests/README.rst Outdated Show resolved Hide resolved
source/command-logging-and-monitoring/tests/README.rst Outdated Show resolved Hide resolved
source/command-logging-and-monitoring/tests/README.rst Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of comments, but this looks fantastic. Nice work.

@kmahar kmahar requested review from a team as code owners September 15, 2022 01:07
@kmahar kmahar requested review from nbbeeken and kevinAlbs and removed request for a team September 15, 2022 01:07
@kmahar kmahar requested a review from jyemin October 11, 2022 23:37
has a redacted failure, e.g. an empty string or object.
The exact form of those assertions will vary based on the driver's chosen
representations for failures and redacted failures in log messages.
in its attached data. Then this field is set to ``false``, the test runner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then -> When

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks; ended up deleting this per above thread about using a single boolean

@kmahar kmahar requested review from jmikola and jyemin October 12, 2022 16:33
@kmahar
Copy link
Contributor Author

kmahar commented Oct 12, 2022

I'm confused about the "Check if all JSON test files are up-to-date" failure. it seems to be complaining about a CMAP test I haven't touched here, and the "diff" is a line that looks identical before and after. it doesn't seem relevant

diff --git a/source/connection-monitoring-and-pooling/tests/pool-clear-interrupt-immediately.json b/source/connection-monitoring-and-pooling/tests/pool-clear-interrupt-immediately.json
index e61ca51..54e2566 100644
--- a/source/connection-monitoring-and-pooling/tests/pool-clear-interrupt-immediately.json
+++ b/source/connection-monitoring-and-pooling/tests/pool-clear-interrupt-immediately.json
@@ -24,7 +24,7 @@
       "name": "waitForEvent",
       "event": "ConnectionPoolCleared",
       "count": 1,
-      "timeout": 1000      
+      "timeout": 1000
     },
     {
       "name": "waitForEvent",

edit: ah it's a whitespace change. I'll make a small separate PR to correct that so we can get a complete CI run on the new tests. I wonder why this only showed up now... the js-yaml version hasn't changed


- ``failureIsRedacted``: Optional boolean. This field SHOULD only be specified
when ``data`` contains ``{ failure: { $$exists: true }}``; the test runner
MUST report an error if this is not the case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect this would require a subsequent assertion on the contents of data field in the test file. Rather, the mere presence of failureIsRedacted would require a driver to assert that there is some failure to inspect in the first place. In other words, a test could use failureIsRedacted on its own with no meaningful data assertion and trust the assertion would fail outright if the log data omitted a failure altogether.

I imagine there might also be cases where data.failure could be something other than { $$exists: true } (perhaps a $$type assertion or something more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I misunderstood your comment above. and good point re: other assertions on failure being possible.

I updated the language here to just say

This field SHOULD only be specified when the log message data is expected to contain a failure value.

source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
@kmahar kmahar requested a review from jmikola October 13, 2022 21:42
@kmahar
Copy link
Contributor Author

kmahar commented Oct 14, 2022

#1316 bumped the unified format schema version; just pushed another few commits to reflect this so that the logging changes are added via schema version 1.13.

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!!

Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (2 minor comments)


Unacknowledged/Acknowledged Writes
----------------------------------

Unacknowledged writes must provide a ``CommandSucceededEvent`` with a ``{ ok: 1 }`` reply.
Unacknowledged writes must provide a ``CommandSucceededEvent`` and a "command succeeded" log message with a ``{ ok: 1 }`` reply .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: redundant space in reply .?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. thanks!

* - command
- String
- Relaxed extJSON representation of the command. This document MUST be truncated appropriately according to rules defined in the
`logging specification <../logging/logging.rst>`_, and MUST be replaced with an empty document "{ }" if the command is considered sensitive.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your consideration, you could add a link to the truncation section #truncation-of-large-documents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good thought to add a link. that section covers truncation rationale, but I am going to add one to the section on truncation rules: Configurable Max Document Length.

@kmahar kmahar merged commit d4f83d8 into master Oct 14, 2022
@kmahar kmahar deleted the DRIVERS-1204/logging-spec branch October 14, 2022 21:50
@kmahar kmahar changed the title DRIVERS-1204: Add general logging specification and command logging DRIVERS-1677, DRIVERS-1673: Add general logging specification and command logging Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants