-
Notifications
You must be signed in to change notification settings - Fork 246
DRIVERS-719 OpenTelemetry specification #1826
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
base: master
Are you sure you want to change the base?
Conversation
…logy. Adding toplogy requirements for transaction tests
source/unified-test-format/tests/invalid/expectedTracingSpans-spanMalformedMissingName.yml
Outdated
Show resolved
Hide resolved
source/unified-test-format/tests/invalid/expectedTracingSpans-spanMalformedMissingName.yml
Outdated
Show resolved
Hide resolved
source/unified-test-format/tests/invalid/expectedTracingSpans-spanMalformedTagsMustBeObject.yml
Outdated
Show resolved
Hide resolved
source/unified-test-format/tests/invalid/expectedTracingSpans-spanMalformedTagsMustBeObject.yml
Outdated
Show resolved
Hide resolved
- `enableCommandPayload`: Optional boolean. When set to `true`, enables capturing of command payload details in | ||
tracing spans. | ||
- If `true`, the test runner SHOULD capture detailed command payload information in tracing spans. | ||
- If `false` or omitted, the test runner SHOULD exclude command payload 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.
I realize this impacts the test runner, but should it also refer to configuring the MongoClient to enable tracing?
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.
This point is similar to #1826 (comment) ?
This option is both configured via the test runner and should be available to toggle on/off via MongoClient. This should be covered by a prose test 👍
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.
Prose test added
Co-authored-by: Jeremy Mikola <[email protected]>
Co-authored-by: Jeremy Mikola <[email protected]>
…TracingMessages-additionalProperties.yml Co-authored-by: Jeremy Mikola <[email protected]>
…spanMalformedTagsMustBeObject.yml Co-authored-by: Jeremy Mikola <[email protected]>
…TracingMessages-additionalPropertyType.yml Co-authored-by: Jeremy Mikola <[email protected]>
…missingPropertySpans.yml Co-authored-by: Jeremy Mikola <[email protected]>
…spanMalformedMissingName.yml Co-authored-by: Jeremy Mikola <[email protected]>
…spanMalformedTagsMustBeObject.yml Co-authored-by: Jeremy Mikola <[email protected]>
…summary with examples - Adding test for abortTransaction - Renaming transaction test to core_api
- **MongoClient Level**: Drivers SHOULD provide a configuration option for `MongoClient`'s Configuration/Settings that | ||
enables or disables tracing for operations and commands executed with this client. This option MUST override | ||
settings on higher levels. This configuration can be implemented with a `MongoClient` option, for example, | ||
`tracing.enabled`. |
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.
Agreed. We usually call out if an option should be URI, MongoClient settings, or both. Like for versioned api:
https://github.com/mongodb/specifications/blob/master/source/versioned-api/versioned-api.md#no-uri-options
Also can you clarify how tracing.enabled
is supposed to be translated into code? Is it MongoClient(tracing=True)
, MongoClient(tracing="enabled")
, or something else?
| `abortTransaction` | [tests/transaction/transaction.yml](tests/transaction/transaction.yml) | | ||
| `createCollection` | [tests/transaction/create_collection.yml](tests/operation/create_collection.yml) | | ||
| `createIndexes` | [tests/transaction/create_indexes.yml](tests/operation/create_indexes.yml) | | ||
| `createView` | [tests/transaction/create_view.yml](tests/operation/create_view.yml) | |
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.
createView is not a spec method. I don't think it should be included here or in the unified tests.
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.
It's a fair point, my understanding is that some operations predate the spec ex createView defined in https://jira.mongodb.org/browse/DRIVERS-309 or createCollection
only defined in unified tests. To your point in #1826 (comment) We should aim to cover the majority of operations that will help the developer trace and troubleshoot their application
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.
So are we going to remove createView here? Or mention that it's not standard?
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.
It's included (with its corresponding test) https://github.com/mongodb/specifications/blob/DRIVERS-719/source/open-telemetry/open-telemetry.md#covered-operations 👍
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 problem is that createView doesn't exist in any specs. I don't think this PR should introduce it as if it's a spec method that exists in all drivers.
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 removed tests for the createView
, and the row in this table.
- **MongoClient Level**: Drivers SHOULD provide a configuration option for `MongoClient`'s Configuration/Settings that | ||
enables or disables tracing for operations and commands executed with this client. This option MUST override | ||
settings on higher levels. This configuration can be implemented with a `MongoClient` option, for example, | ||
`tracing.enabled`. |
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.
@ShaneHarvey: thanks for bringing up the typing. If this really is just a boolean type, I think tracing
or enableTracing
(more active language than tracingEnabled
) with a default value of false
would be most consistent with other driver option names in PHP. But since these aren't URI options there should be more naming flexibility available for drivers.
It looks like @comandeo-mongo is talking about this in a comment below, too: https://github.com/mongodb/specifications/pull/1826/files#r2339648026
Co-authored-by: Jeremy Mikola <[email protected]>
Co-authored-by: Jeremy Mikola <[email protected]>
1a4c85c
to
39863b0
Compare
- **MongoClient Level**: Drivers SHOULD provide a configuration option for `MongoClient`'s Configuration/Settings that | ||
enables or disables tracing for operations and commands executed with this client. This option MUST override | ||
settings on higher levels. This configuration can be implemented with a `MongoClient` option, for example, | ||
`tracing.enabled`. |
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.
@nhachicha: Do you want to chime in here?
|
||
Drivers MUST take care to avoid exposing sensitive information (e.g. authentication credentials) in traces. | ||
|
||
## Future Work |
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.
@comandeo-mongo: Did you want to add a concise mention of "query parameterization" under this section? Doesn't need to be explained in detail, but just something to capture our original exchange in the earlier comment thread, which you can resolve.
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.
Added some clarification, I hope I understand correctly what you suggested.
| `db.mongodb.server_connection_id` | `int64` | Server connection id | Required if available | | ||
| `db.mongodb.driver_connection_id` | `int64` | Local connection id | Required if available | | ||
| `db.query.text` | `string` | Database command that was sent to the server. Content should be equivalent to the `document` field of the CommandStartedEvent of the command monitoring. | Conditional | | ||
| `db.mongodb.cursor_id` | `int64` | If a cursor is created or used in the operation | Required if available | | ||
| `db.mongodb.lsid` | `string` | Logical session id | Required is available | | ||
| `db.mongodb.txn_number` | `int64` | Transaction number | Required is available | |
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.
for lsid and txn_number: is -> if
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.
Fixed, thanks!
##### Exceptions | ||
|
||
If the driver operation fails with an exception, drivers MUST record an exception to the current operation span. When | ||
recording an exception, drivers SHOULD add the following attributes to the span, when the content for the attribute if |
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.
This typo still exists.
| `abortTransaction` | [tests/transaction/transaction.yml](tests/transaction/transaction.yml) | | ||
| `createCollection` | [tests/transaction/create_collection.yml](tests/operation/create_collection.yml) | | ||
| `createIndexes` | [tests/transaction/create_indexes.yml](tests/operation/create_indexes.yml) | | ||
| `createView` | [tests/transaction/create_view.yml](tests/operation/create_view.yml) | |
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 problem is that createView doesn't exist in any specs. I don't think this PR should introduce it as if it's a spec method that exists in all drivers.
DRIVERS-719
observeTracingMessages
andexpectTracingMessages