DRIVERS-3617 Specify error.type on OpenTelemetry command spans - #1974
DRIVERS-3617 Specify error.type on OpenTelemetry command spans#1974blink1073 wants to merge 21 commits into
Conversation
…error.type absent on operation span
… name on non-server errors
…n failures aren't retried away
# Conflicts: # source/open-telemetry/open-telemetry.md
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate specification issues leave operation-span behavior and retried command fallback values incorrect or undefined.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Defines consistent OpenTelemetry error.type behavior for MongoDB command spans and adds test coverage.
Changes:
- Specifies command and operation span behavior.
- Adds server and connection-error fixtures.
- Updates the changelog.
File summaries
| File | Description |
|---|---|
source/open-telemetry/tests/operation/error_type.yml |
Adds unified tracing tests for server and connection errors. |
source/open-telemetry/tests/operation/error_type.json |
Adds the generated JSON fixture. |
source/open-telemetry/open-telemetry.md |
Documents error.type; operation-span behavior and fallback semantics require correction and additional coverage. |
Review details
Suppressed comments (2)
source/open-telemetry/open-telemetry.md:378
- The new fixture only exercises failed command spans, so the new MUST NOT requirement on successful commands is not enforced. Existing successful-span expectations also omit this key rather than asserting
$$exists: false, allowing an implementation that emitserror.typeon success to pass. Add a successful command case with an explicit absence assertion.
Drivers MUST NOT set this attribute when the command succeeds. Per the
source/open-telemetry/open-telemetry.md:385
- This is inaccurate for server errors: there
error.typeis the numeric status code whileexception.typeis the exception class, so no duplication occurs and the class is not promoted to the span-levelerror.type. Limit this explanation to the no-status-code fallback.
`error.type` deliberately duplicates the `exception.type` attribute of the exception *event* recorded on the same span
(see Exceptions below). Tracing backends query and aggregate on span attributes, not on the attributes of events nested
within a span, so only `error.type` makes the error class queryable at the span level.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The response-status requirement conflicts with connection failures, and the fallback behavior is not adequately tested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
source/open-telemetry/tests/operation/error_type.yml:129
- This assertion only proves that
error.typeis some string; it does not verify the test's stated fallback to the exception class name or the specified equality withexception.type. An implementation that emits a constant such as"network_error"would pass. Since unified matching cannot compare two observed attributes, add an appended prose test that assertserror.type == exception.type(and, where observable, the raised exception's class name) for this failure.
source/open-telemetry/open-telemetry.md:376
- The value is only a SHOULD even though the table makes the attribute required and this change is intended to give drivers a shared definition. A conforming implementation could emit an unrelated constant string for every failure, defeating interoperable grouping while still satisfying the normative text. Make both derivation branches mandatory; the separate low-cardinality guidance can remain a SHOULD.
This attribute SHOULD match `db.response.status_code` when the command failed with a server error, meaning the server
returned an error code in its response. Otherwise, this attribute SHOULD be the name of the exception class the driver
raises to the application.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
… on a non-server error
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain in the specification requirements and test coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
source/open-telemetry/tests/README.md:103
AGENTS.md:121-124requires relative1.numbering for every prose-test step. Using literal numbers makes future insertion and formatting unnecessarily fragile.
source/open-telemetry/open-telemetry.md:376
- The table makes
error.typerequired on every failed command, but theseSHOULDstatements leave its actual value optional, allowing drivers to emit incompatible values despite this PR's shared definition. Make both derivation branches mandatory.
This attribute SHOULD match `db.response.status_code` when the command failed with a server error, meaning the server
returned an error code in its response. Otherwise, this attribute SHOULD be the name of the exception class the driver
raises to the application.
source/open-telemetry/open-telemetry.md:385
- This rationale conflicts with the existing command-span contract below at lines 389–395:
exception.typeis already added as a span attribute (and the fixtures match it there), so it is already queryable at span level. Explain thaterror.typesupplies OpenTelemetry's standard error dimension instead of claiming it introduces span-level queryability.
Tracing backends query and aggregate on span attributes, not on the attributes of events nested within a span.
`error.type` makes the error class queryable at the span level. For a non-server error, it carries the same value as the
`exception.type` attribute of the exception *event* recorded on the same span (see Exceptions below).
source/open-telemetry/open-telemetry.md:255
- No fixture exercises the successful-retry case that motivates this prohibition. The new tests cover a failed operation and a clean success, so an implementation that copies the first failed command's
error.typeonto an operation that later succeeds would still pass. Add a retry-success case asserting no operation-levelerror.typewhile the failed command has one.
Unlike command spans, operation spans MUST NOT have an `error.type` attribute. An operation can succeed through a retry
even when one of its commands failed, so the command-level derivation does not carry over.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
…s fixture case Explains error.type as OpenTelemetry's standard grouping/alerting attribute rather than claiming it is the only span-level error dimension, since exception.type is already a span attribute per the existing Exceptions section. Adds a test covering a failed retried command whose retry succeeds, asserting error.type stays off the operation span.
There was a problem hiding this comment.
🟡 Changes recommended
The required attribute’s value derivation remains underspecified, preventing consistent cross-driver behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
source/open-telemetry/tests/README.md:103
- These prose-test steps use literal numbering, contrary to the repository requirement to use
1.for every numbered bullet (AGENTS.md:121-124). Use relative numbering so later edits do not require renumbering the list.
source/open-telemetry/tests/operation/error_type.yml:152
- The linked Python implementation PR vendors only the first two cases from this fixture; this success case and the retry-success case below are absent. Therefore these newly added requirements have not been exercised in the checked-off language-driver implementation. Sync the current fixture and run it there before merging.
- description: error.type is absent when the command succeeds
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
…unified runner supports one tracing-observed client per test
The retry-success case's second observeTracingMessages client is not
something every driver's unified test runner supports (confirmed:
PyMongo's rejects more than one). Drops the client-level retryReads
override too, using mode: { times: 2 } to make the non-server-error
case durable across a retry instead.
…xception, not one raised to the application A retried command can fail without ever surfacing an exception to the application, if the retry succeeds. Rewords the fallback to describe the exception associated with that command's failure instead, and adds a test covering a retried non-server error whose retry succeeds.
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate specification issues must be resolved before approval.
Review details
Suppressed comments (4)
source/open-telemetry/open-telemetry.md:503
- This changelog summary excludes command failures hidden by a successful retry, although the normative text explicitly assigns their command spans an exception-class value. Describe the class associated with the command failure rather than one the driver raises.
command failed with a server error and is otherwise the name of the exception class the driver raises. Specified
source/open-telemetry/open-telemetry.md:376
- The attribute is required on every failed command, but both value-selection rules are only recommendations. That permits conforming drivers to emit different arbitrary values and defeats the cross-driver definition this change introduces; the fixture also requires the exact server-code value. Make both rules
MUSTso the required attribute has an interoperable value.
This attribute SHOULD match `db.response.status_code` when the command failed with a server error, meaning the server
returned an error code in its response. Otherwise, this attribute SHOULD be the name of the exception class associated
with that command's failure, whether or not the operation ultimately raises it to the application: a retry of the same
source/open-telemetry/open-telemetry.md:255
- This blanket prohibition conflicts with OpenTelemetry’s MongoDB span convention, which requires
error.typeif and only if the operation failed. A successful retry should omit the attribute, but an operation that ultimately fails still needs its own final error type; otherwise the public operation span cannot be grouped by the standard error dimension. Base this attribute on the operation’s final outcome and update the failed-operation fixtures accordingly.
Unlike command spans, operation spans MUST NOT have an `error.type` attribute. An operation can succeed through a retry
even when one of its commands failed, so the command-level derivation does not carry over.
source/open-telemetry/tests/README.md:103
- Repository guidance requires relative
1.markers for every prose-test step (AGENTS.md:121-124). Replace these literal numbers so the list remains automatically numbered and future edits do not require manual renumbering.
1. Create a `MongoClient` with tracing enabled and `retryReads` disabled.
2. Configure a `failCommand` fail point on `find` with `closeConnection: true`.
3. Call `find` on a test collection and let it fail.
4. Assert that the command span's `error.type` attribute equals its `exception.type` attribute, and that both equal the
raised exception's fully qualified class name.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…n itself fails The blanket 'operation spans MUST NOT have error.type' prevented backends from grouping failed MongoDB operations by the standard error dimension, even though operation spans already carry exception.type in that case. Operation spans still MUST NOT carry error.type when the operation succeeds despite a failed command, but now SHOULD carry it, matching exception.type, when the operation itself ultimately fails. Updates the two fixtures whose operation span fails to assert this.
…wing the whole table
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the conflicting operation-span requirements and fixture expectations before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
source/open-telemetry/open-telemetry.md:380
- These
SHOULDrequirements allow a conforming driver to choose another value, while the fixture requires the exact server code and the prose test requires exact equality withexception.type. UseMUSTso the normative specification and mandatory tests define the same behavior.
This attribute SHOULD match `db.response.status_code` when the command failed with a server error, meaning the server
returned an error code in its response. Otherwise, this attribute SHOULD be the name of the exception class associated
with that command's failure, whether or not the operation ultimately raises it to the application: a retry of the same
operation may still succeed.
source/open-telemetry/tests/README.md:103
- Use relative
1.markers for every prose-test step, as required byAGENTS.md:123; this prevents later insertions from renumbering steps that drivers may reference.
1. Create a `MongoClient` with tracing enabled and `retryReads` disabled.
2. Configure a `failCommand` fail point on `find` with `closeConnection: true`.
3. Call `find` on a test collection and let it fail.
4. Assert that the command span's `error.type` attribute equals its `exception.type` attribute, and that both equal the
raised exception's fully qualified class name.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Specification requirements and tests are inconsistent, operation-span equality lacks coverage, and the fixture includes unrelated attributes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
source/open-telemetry/open-telemetry.md:380
- The value contract is weaker than the fixture: these
SHOULDs permit a driver to emit another value, whileerror_type.ymlrequires the server code and the prose test requires the exception type. Make both mappings mandatory so conforming implementations cannot disagree with the conformance tests.
This attribute SHOULD match `db.response.status_code` when the command failed with a server error, meaning the server
returned an error code in its response. Otherwise, this attribute SHOULD be the name of the exception class associated
with that command's failure, whether or not the operation ultimately raises it to the application: a retry of the same
operation may still succeed.
source/open-telemetry/open-telemetry.md:196
- The operation-span requirement is inconsistent with the new fixture: this table marks
error.typeas conditional and line 257 only says it SHOULD be set, whileerror_type.ymlrequires it on every failed operation span. A driver that legitimately omits this recommended attribute would fail the conformance test. Please either make it required when the operation fails or relax those operation-span expectations.
| `error.type` | `string` | The exception class's name, if the operation fails (see below) | Conditional |
source/open-telemetry/tests/operation/error_type.yml:68
- This focused fixture repeats the full baseline command-span contract in every case.
AGENTS.md:126-129requires new tests to omit fields unrelated to the behavior under test; retaining namespace, network, address, query-summary, and connection attributes makes these tests fail when unrelated telemetry evolves. Reduce all command expectations in this fixture to the attributes needed forerror.typebehavior and span structure.
db.system.name: mongodb
db.namespace: *database0Name
db.collection.name: *collection0Name
db.command.name: find
network.transport: tcp
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
…n itself fails, and test its equality with exception.type Separates presence (Required if it fails, in the table and prose) from the value's existing SHOULD-derivation, matching the command-level pattern. Extends prose Test 5 to also assert the operation span's error.type equals its exception.type.
There was a problem hiding this comment.
🟡 Changes recommended
Critical specification/test inconsistencies and missing server-error coverage must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
source/open-telemetry/tests/README.md:99
- Use relative Markdown numbering (
1.for every step), as required byAGENTS.md:121-124; this keeps later insertions from renumbering prose tests.
1. Create a `MongoClient` with tracing enabled and `retryReads` disabled.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
… on a server error's operation span
There was a problem hiding this comment.
🟡 Changes recommended
The new prose tests in source/open-telemetry/tests/README.md require a “fully qualified class name,” which is stricter than the spec’s “exception class name” wording and may impose unintended cross-driver requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
source/open-telemetry/tests/README.md:118
- As written, this step requires the raised exception’s fully qualified class name, but the spec only requires the exception class name (and that operation-span
error.typematchesexception.type). Consider relaxing this to “class name” unless the spec is updated to define “fully qualified” for all drivers.
4. Assert that the operation span's `error.type` attribute equals its `exception.type` attribute, and that both equal
the raised exception's fully qualified class name rather than the server error code.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
…n the error.type prose tests
There was a problem hiding this comment.
🟢 Approval recommended
The spec changes are internally consistent with the accompanying fixtures and changelog update, and no correctness issues were found in the reviewed diffs.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
| returned an error code in its response. Otherwise, this attribute SHOULD be the name of the exception class associated | ||
| with that command's failure, whether or not the operation ultimately raises it to the application: a retry of the same | ||
| operation may still succeed. |
There was a problem hiding this comment.
Do we have guidance for programming languages where an "exception class" is not always available or practical to determine?
For example, Go has error types, but not all errors require an explicit type, and some error types are part of an error tree containing multiple error types. The guidance below says:
this attribute SHOULD have a low number of distinct values, because tracing backends use it as a dimension for grouping and alerting on failures.
Should drivers for languages without exception classes use an explicitly constrained set of constants?
Also, what should we do for bulk write errors where the error may contain many error codes, one per failed write?
| this attribute SHOULD have a low number of distinct values, because tracing backends use it as a dimension for grouping | ||
| and alerting on failures. |
There was a problem hiding this comment.
If grouping dimensions have too many distinct values, some tracing/metrics backends may have performance issues or may bill more per number of distinct values.
For example, Datadog bills for Custom Metrics by the number of distinct combinations of metric name, host, and tags. If a customer using Datadog wanted to derive a custom metric based on operation traces, tagged with the "error.type" attribute, their costs could scale with the number of distinct "error.type" attribute values.
Should we change this to MUST instead of SHOULD to protect customers from those issues?
DRIVERS-3617
Please complete the following before merging:
Summary
error.typeis the OpenTelemetry semantic-convention attribute that tracing backends recognize for grouping and alerting on failures, but the command span spec defined onlydb.response.status_code.exception.typeisn't part of that convention, even though drivers also add it to the span, so drivers that emit it anyway have no shared definition of its value.Changes
error.typeto the Command Span Attributes table as a string, required if an error happens.db.response.status_codefor server errors, otherwise the name of the exception class associated with that command's failure (which may never reach the application, if a retry saves the operation).exception.type, when the operation itself ultimately fails.error_type.yml) covering: a server error; a closed connection falling back to the class name; success; a failed command whose retry succeeds; and a failed retry whose exception never reaches the application.open-telemetry.md.Testing
make -C source, then confirmed the regenerated JSON left no diff.pre-commit run --all-files: clean.mkdocs build --strict: clean.schema-1.27.jsonwithajv-cli: valid.