DRIVERS-3598 Specify and test getMore span nesting in the OpenTelemetry spec - #1973
DRIVERS-3598 Specify and test getMore span nesting in the OpenTelemetry spec#1973blink1073 wants to merge 18 commits into
Conversation
Resolves a changelog conflict in source/open-telemetry/open-telemetry.md: upstream's DRIVERS-3597 entry and this branch's DRIVERS-3598 entry were both prepended to the same section. Kept both, newest first.
| This operation span MUST NOT be nested under the operation span of the command that created the cursor. A host | ||
| application may do unrelated work between batches, and nesting each `getMore` under the cursor-creating operation would | ||
| attribute that work to the original operation. Ordinary nesting still applies otherwise: a cursor iterated inside a | ||
| `withTransaction` callback nests into the `withTransaction` span. |
There was a problem hiding this comment.
| `withTransaction` callback nests into the `withTransaction` span. | |
| `withTransaction` callback nests into the `withTransaction` span; a cursor iterated inside a transaction created using core transactions api nests into the pseudo operation `transaction` span. |
| 5. Assert that on each of those two spans, the value is the cursor id the driver sent in the `getMore` command, and not | ||
| the `0` returned in that command's reply. | ||
|
|
||
| *Test 4: `getMore` inside a transaction nests under the transaction span* |
There was a problem hiding this comment.
Would it be better to implement this test as a unified? If we do not use the convenient transaction api, it seems to be possible. Something like this:
operations:
- { name: startTransaction, object: *session0 }
- name: createFindCursor
object: *collection0
arguments: { filter: {}, batchSize: 2, session: *session0 }
saveResultAsEntity: &cursor0 cursor0
- { name: iterateUntilDocumentOrError, object: *cursor0, expectResult: { _id: 1 } }
- { name: iterateUntilDocumentOrError, object: *cursor0, expectResult: { _id: 2 } }
- { name: iterateUntilDocumentOrError, object: *cursor0, expectResult: { _id: 3 } }
- { name: commitTransaction, object: *session0 }
expectTracingMessages:
- client: *client0
ignoreExtraSpans: false
spans:
- name: transaction
attributes: { db.system.name: mongodb }
nested:
- name: find transaction-get-more.test
- name: getMore transaction-get-more.test
- name: commitTransaction adminIf we want to test withTransaction explicitly, then we need a prose test.
There was a problem hiding this comment.
I added a unified test and clarified the intent of the prose test
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
Review feedback asked for the transaction case as a unified test rather than a prose test. A cursor iterated inside a transaction started with the core transaction API is expressible in the unified format, since the operations are flat rather than inside a callback, so add tests/transaction/get_more.yml for it. Prose test 4 now covers the convenient transaction API specifically, and points at the unified test for the core API case. Also state in the spec that a cursor iterated inside a transaction started with the core transaction API nests into the pseudo operation `transaction` span, alongside the existing `withTransaction` case.
comandeo-mongo
left a comment
comandeo-mongo
left a comment
There was a problem hiding this comment.
Great work, thank you!
|
@paulinevos, can you please review the changes to unified-test-format.md and approve on behalf of dbx-spec-maintainers-unified-test-format? |
mdb-ad
left a comment
mdb-ad
left a comment
There was a problem hiding this comment.
LGTM (reviewed unified test changes only)
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
nhachicha
left a comment
nhachicha
left a comment
There was a problem hiding this comment.
Great work on this 🥇
One idea for a possible follow-up (not blocking): since getMore operation spans are now siblings of the cursor-creating operation's span, their only correlation is the shared db.mongodb.cursor_id attribute, which backends don't render as a relationship.
OpenTelemetry defines span links for exactly this shape: a causal association between spans that must not be parent and child. It could be layered on as an optional overlay for drivers whose tracing stack supports it, without touching anything this PR specifies. Something along the lines of:
Drivers MAY add a span link from each caller-driven getMore operation span to the span context of the operation span that created the cursor. Span links complement rather than replace the requirements above: nesting, span lifetimes, and the
db.mongodb.cursor_idattribute are unaffected, and the unified tests do not assert on links (an optional prose test can).
| # This getMore drains the cursor, so the server's reply carries cursor id 0. | ||
| # The attribute must still be present. $$type only enforces presence and BSON | ||
| # type — it cannot express "non-zero", because the unified test format has no | ||
| # numeric comparison operator, and 0 satisfies $$type: int. The requirement that | ||
| # this holds the non-zero id the driver sent rather than the reply's 0 is covered | ||
| # by prose test 3 in ../README.md. | ||
| db.mongodb.cursor_id: { $$type: [ int, long ] } |
There was a problem hiding this comment.
Note that a numeric comparison is still possible via $$lte (but not applicable in our use case here since $$lte will only gives us an upper bound. see https://mongodb.slack.com/archives/C72LB5RPV/p1787664154418809)
| db.namespace: *database0Name | ||
| db.collection.name: *collection0Name | ||
| db.operation.name: getMore | ||
| db.operation.summary: getMore transaction-get-more.test |
There was a problem hiding this comment.
missing db.mongodb.cursor_id: { $$type: [ int, long ] } for the operation
There was a problem hiding this comment.
Done (with $$gte now).
|
|
||
| *Test 3: `getMore` records the cursor id it sent, not the cursor id returned* | ||
|
|
||
| 1. Create a `MongoClient` with tracing enabled on the client, using the `tracing.enabled` client option. |
There was a problem hiding this comment.
The mechanism for enabling tracing varies across drivers, some drivers enable tracing differently (e.g., the Java driver takes an ObservationRegistry via its observability settings and has no tracing.enabled option). Suggest keeping this step mechanism-neutral.
| 1. Create a `MongoClient` with tracing enabled on the client, using the `tracing.enabled` client option. | |
| 1. Create a `MongoClient` with tracing enabled. |
|
|
||
| This test requires a replica set or a sharded cluster running server version 4.4 or later. | ||
|
|
||
| 1. Create a `MongoClient` with tracing enabled on the client, using the `tracing.enabled` client option. |
There was a problem hiding this comment.
| 1. Create a `MongoClient` with tracing enabled on the client, using the `tracing.enabled` client option. | |
| 1. Create a `MongoClient` with tracing enabled. |
Introduces $$gte, the lower-bound counterpart to $$lte, as unified test
format schema version 1.29.
The getMore fixtures now assert db.mongodb.cursor_id: { $$gte: 1 }
instead of a bare type check. That pins the requirement directly: a
getMore records the cursor id it sent, and the reply's 0 would fail the
lower bound. $$type cannot express this, since 0 is a valid int, and
$$lte only bounds from above.
Review feedback also addressed:
- Add the missing db.mongodb.cursor_id assertions to the operation spans
in tests/transaction/get_more.yml.
- Make step 1 of prose tests 3 and 4 mechanism-neutral. Enabling tracing
varies by driver, so naming the tracing.enabled client option excluded
drivers that configure it differently.
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
Adds tests/transaction/get_more.yml from DRIVERS-3598, which covers a cursor iterated inside a transaction started with the core transaction API: the getMore operation span is a sibling of the find operation span that created the cursor, and both nest under the transaction span. Review feedback on mongodb/specifications#1973 asked for this case as a unified test rather than a prose test. It passes against the driver unchanged. The fixture comes from a specification change that is not merged yet.
The nested-span tightening changes what a conformant runner must do, so it belongs behind a version marker rather than standing as an unversioned prose entry. Both unified test format changes in this PR ship together, so they share schema version 1.29 rather than taking a bump each. Follows the precedent of the 1.9 entry, which lists several changes under one version.
Filed as DRIVERS-3627 |
nhachicha
left a comment
nhachicha
left a comment
There was a problem hiding this comment.
Nice improvement introducing $$gte 🎉
A few points need addressing before this is mergeable, though 👍
| Syntax: | ||
|
|
||
| ```yaml | ||
| { $$gte: 1 } |
There was a problem hiding this comment.
1 - schema-latest.json looks out of date. The PR adds schema-1.29.json but doesn't regenerate schema-latest.json, which must match the highest version (make update-schema-latest -C source) should cover it.
(curious if you used Claude why it didn't pick this up? it's part of the AGENTS.md 🤷 )
2 - Since $$gte is a new matching operator, could this also add a runner reference test under source/unified-test-format/tests/valid-pass/ e.g. operator-gte.yml mirroring operator-lte.yml ?
3 - Please update the PR description/DRIVERS-3598 to emphasis that the implementer must update the test runner (bump to 1.29 with new operator)
There was a problem hiding this comment.
-
Is a no-op because this is a behavioral change the does not show up in the schema itself. It is similar to the
$$lteupdate. -
Done
-
Done
| 8. Perform the same database operation. | ||
| 9. Assert that the emitted tracing span does not include the `db.query.text` attribute. | ||
|
|
||
| *Test 3: `getMore` records the cursor id it sent, not the cursor id returned* |
There was a problem hiding this comment.
Prose test 3 is now redundant since you introduced $$gte to express what $$type couldn't.
There was a problem hiding this comment.
Ah, when I went to change it in PyMongo I noticed that this isn't redundant, because it asserts recorded value equals the id sent, which $$gte can't express. Okay to re-add it?
| This test covers the convenient transaction API. The core transaction API case is covered by the unified test | ||
| [tests/transaction/get_more.yml](transaction/get_more.yml). | ||
|
|
||
| This test requires a replica set or a sharded cluster running server version 4.4 or later. |
There was a problem hiding this comment.
requires server version 4.4 or later
but transaction/get_more.yml (same scenario, core API) uses 4.0/4.1.8 like the existing transaction fixtures. Should these match, or is there a reason the convenient-API case needs 4.4?
There was a problem hiding this comment.
4.4 came from copying the pattern in convenient.yml. Arguably new test should target 4.4+ anyway since we're in the process of dropping 4.2. Support. I'm happy to revert if you disagree.
| a cursor that is never exhausted (e.g., a tailable cursor) leaves nothing unfinished. | ||
|
|
||
| A `getMore` is not retryable, but a change stream may resume after one fails. A resume MUST NOT extend the failed | ||
| `getMore` operation span: drivers MUST finish that span with its error, and the `killCursors`, `aggregate`, and |
There was a problem hiding this comment.
killCursors doesn't belong in this list (it's internal cleanup), and the Python reference implementation gives it no operation span (its command span parents to the current context), so as written the reference implementation violates this MUST
Suggest removing it from the sentence and stating its treatment explicitly, e.g.: "Drivers MUST NOT create an operation span for the killCursors sent during a resume."
WDYT?
- A resume no longer requires an operation span for its killCursors. That command is internal cleanup rather than a public API call, and the reference implementation gives it none, so the previous wording made a conformant driver non-conformant. - Add tests/valid-pass/operator-gte.yml as the runner reference test for the new operator, mirroring operator-lte.yml. - Update the Current Schema Version header field to 1.29.0. The changelog and schema file were added without it. - Drop the prose test asserting the sent cursor id. $$gte: 1 expresses it directly, so it was redundant; the remaining transaction prose test is renumbered to 3. - Align that prose test's server requirements with tests/transaction/get_more.yml. The 4.4 bound was copied from convenient.yml and is stricter than the convenient transactions API needs.
…e test Matches the existing convenient transaction API fixture rather than the core API bounds. Stating the reason in the text so the split between the two transaction tests is not read as an oversight.
|
I'm running another patch build in PyMongo with the updated changes before re-requesting review. |
5 participants
Please complete the following before merging:
clusters). PyMongo implementation: PYTHON-5993 Add OpenTelemetry operation spans for cursor getMores mongo-python-driver#2994 — Evergreen patch, 24/24
tasks green across standalone, replica set and sharded cluster:
https://spruce.corp.mongodb.com/version/6a9072ed0671d4000712f122
Context
Command spans must nest under "the corresponding driver operation span", which is unambiguous only while an operation sends a single command. For a cursor the spec never said which operation span a
getMorebelongs to, and no fixture exercisedgetMore, so both readings passed the suite anddb.mongodb.cursor_idwas asserted absent in all 25 places it appeared.The ambiguity was not hypothetical. PyMongo had implemented one reading, a single operation span covering a cursor's whole lifetime, and the new fixture caught it on its first run.
Changes
Nesting. A caller-driven
getMoregets its own operation span, sibling to the cursor-creating operation's span. Driver-internal iteration, where one public API call drains the cursor itself, creates no additional operation spans. No span is scoped to a cursor's lifetime, so a cursor that is never exhausted leaves nothing unfinished. A change-stream resume ends the failedgetMoreoperation span rather than extending it, and thekillCursorsit sends gets no operation span. A cursor iterated inside a transaction nests under thewithTransactionspan or the pseudo operationtransactionspan, depending on which API started it.db.mongodb.cursor_id. Raised from SHOULD to MUST and added to operation spans. It holds the id the driver sent for agetMore, even when that reply returns0. It is omitted rather than emitted as0for a cursor-creating command that leaves no cursor open, and omitted for commands that may operate on several cursors at once.Tests.
operation/get_more.ymlcovers the nesting and bothcursor_idoutcomes.transaction/get_more.ymlcovers a cursor iterated inside a core-API transaction. Both useignoreExtraSpans: falseand assertdb.mongodb.cursor_id: { $$gte: 1 }where the attribute is required, because a lower bound of 1 is what pins "the id sent, not the reply's0". Prose test 3 covers the convenient transaction API's callback, which the unified format cannot express. The prose test that asserted the sent cursor id is gone, since$$gte: 1now expresses it.Unified test format.
ignoreExtraSpansapplies at every level of the span tree, without which the negative assertion inoperation/get_more.yml, that thegetMoreis not nested underfind, is not enforceable by a conformant runner. Schema version 1.29 introduces$$gte, the lower-bound counterpart to$$lte, withvalid-pass/operator-gte.ymlas the runner reference test mirroringoperator-lte.yml.Two decisions worth a second opinion
Neither is stated in the design document, so please confirm rather than assume:
getMore <db>.<collection>, following the existing convention of naming operation spans after commands rather than public-API methods. Naming it after the driver's iteration method would vary per driver and make the test unassertable cross-driver.0. This matches the existingfind.ymlandaggregate.ymlfixtures, which omitcursor_idon commands that exhaust in the first batch, and OpenTelemetry's convention of omitting unavailable attributes rather than encoding a sentinel.Verification
Both fixtures pass against PyMongo,
transaction/get_more.ymlunchanged from its first run, and the prose test is implemented there. The generated JSON is byte-identical to a fresh run of this repository's generator, and both fixtures are schema-valid against their declared schema version 1.29.AI disclosure
Drafted with Claude Code (Claude Opus 5 and Sonnet 5). Reviewed by the human author before opening.