Add Cosmos diagnostics contract design doc#4684
Conversation
Design-first (contract-first) deliverable: define the driver<->SDK Cosmos diagnostics CONTRACT and its OpenTelemetry mapping as a standalone document on main. Follow-up to the deferred capture-engine prototype (PR Azure#4619), referenced here conceptually only. DIAGNOSTICS-CONTRACT.md describes: - a cheap handle (Arc<DiagnosticsContext>, already on main) plus explicit, lazy materialization into three representations - structured metrics, OTel spans, and a JSON string (no single fixed format); - an opaque-handle + explicit-materialize FFI boundary for the next-major Go/Python SDKs (never forcing JSON at the boundary); - a DiagnosticsLevel that bounds high-cardinality transport telemetry (never collection); - a bounded-size guarantee for retry storms; - the OTel metrics+traces mapping (operation-level always-on; transport- level level-gated to control time-series cardinality), aligned with azure_core's span-attribute constants; and - the resolved design decisions. Documentation only - no code changes, no public API change, CosmosResponse::diagnostics() stays non-optional, additive/non-breaking. References to main types are tagged [main]; proposed surface [contract]; deferred capture-engine items [Azure#4619]. No CHANGELOG (doc/design-only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a312c64 to
3b4c830
Compare
|
Design reads well, and the §6 / Q4 numbers don't match what #4683 shipsThe doc states bounded-size defaults of 64 attempt records / 128 spans / 8 KB string, "first+last-per-region." #4683 ships a default cap of 512, no span or byte cap, keyed on endpoint (not region). Per §4 rule 5 an FFI consumer would size a buffer to 64/128/8 KB and under-allocate ~8× on attempts (with no span/byte bound at all). Worth reconciling the numbers, or explicitly marking them as the target for the deferred engine rather than today's behavior. "Deferred / OFF" vs. hard dependency§0/§1/Q1 frame #4619 as a deferred optimization that stays OFF and "is not on main," but #4683 depends on #4619's §7.3 "one child span per attempt"This becomes unrealizable once compaction elides the middle of a run (and §6 itself caps spans at 128). Might note that the per-attempt span mapping describes the uncompacted view, and a compacted run collapses to a single span + count. |
Address self-review on PR Azure#4684: - Reconcile 6/Q4 bounded-size numbers with what PR Azure#4683 actually ships: the per-attempt list is bounded by max_request_diagnostics (default 512, min 16), keyed on (region, endpoint, status, sub_status, execution_context), first+last-per-run + exact aggregates + a bounded per-run rollup. The old 64-attempt / 128-span / 8 KB figures are re-labelled as target defaults for the not-yet-implemented span/string representations and the deferred engine, and FFI buffer sizing (4 rule 5) is told to read the configured cap. - Clarify deferred/OFF vs hard dependency: the contract doc is standalone on main and depends on neither Azure#4619 nor Azure#4683, but the Azure#4683 compaction impl reuses Azure#4619's DiagnosticsSummary and must merge after it -- so the deferred/OFF language describes the capture engine, not the compaction impl. - Note in 7.3 that the one-span-per-attempt mapping is the uncompacted view; under compaction a run collapses to one span + a repeat count, so emitters build the tree from requests() (retained), not request_count(). Documentation only; markdownlint clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed in 60eeb5b (documentation only):
markdownlint clean. |
…ntract Address review nit: clarify that the shipped 512 structured-object record cap and the 128-span target are independent per-representation caps, not a single number. A span emitter reaches <=128 spans by collapsing each run to one span + a repeat count (per 7.3), so span count tracks runs (already capped), not retained records. Also note PR Azure#4683's guarantee that the retained attempts and the per-run rollup are drawn from the same cap-bounded set, so an emitter never sees a retained attempt whose run was omitted. Documentation only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the round-2 doc nit in
Documentation only. |
There was a problem hiding this comment.
Pull request overview
This documentation-only PR adds DIAGNOSTICS-CONTRACT.md to the azure_data_cosmos_driver crate. It specifies the driver↔SDK diagnostics contract and its OpenTelemetry mapping as a standalone design document, deliberately decoupled from the deferred capture-engine prototype (#4619). It defines a cheap always-available handle with explicit, lazy materialization into three representations (structured metrics / OTel spans / JSON string), an opaque-handle FFI boundary for future Go/Python SDKs, a DiagnosticsLevel gating knob that bounds only transport-level telemetry, a bounded-size guarantee for retry storms, and the concrete OTel metric/trace/attribute mapping aligned with azure_core.
Changes:
- Adds a new design doc describing the diagnostics contract, FFI boundary, level/threshold gating, and bounded-size guarantee.
- Documents the OTel metrics + traces mapping and
azure_coreattribute alignment (including theaz.service_request.iddot and private-constant caveats). - Records resolved design decisions (handle/materialize surface, FFI lifetime, gating enum, caps, SDK-vs-driver emission split, aggregated span shape); no code or public API changes.
Address the Copilot review comments and unblock the documentation-only CI checks on PR Azure#4684: - Reword the two "Shipped today (PR Azure#4683)" claims (in the bounded-size section and the Q4 row) to "Proposed in PR Azure#4683 (not yet on `main`)". `max_request_diagnostics` is added by the unmerged Azure#4683, so this stays consistent with the "standalone on `main`" framing and no longer implies a reader will find the symbol via the `[opts]` link, which on `main` exposes only `max_summary_size_bytes` and `default_verbosity`. - Convert the five relative source-file reference links to absolute GitHub URLs so the link verification check passes. - Add the doc's technical terms (flatbuffer, materializer(s), uncompacted, underspecified) to the cSpell dictionary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed
Build Pack Crates is a pre-existing, workspace-wide failure that this documentation PR doesn't introduce and can't fix: |
analogrelay
left a comment
There was a problem hiding this comment.
As a contract between SDK and Driver, I think this generally looks good. I think we'll want to think carefully about the collection logic, but that can come next. The key thing is that idea of thresholds being used to gate materialization rather than collection, since we need to be able to evaluate them after the request completes.
Move the five Cosmos-specific spellings (flatbuffer, materializer, materializers, uncompacted, underspecified) out of the global .vscode/cspell.json and into the Cosmos-scoped sdk/cosmos/.cspell.json ignoreWords list, per reviewer feedback from @heaths. These terms are only used by sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md, so scoping them keeps the global dictionary free of Cosmos-only words. The global config is now identical to its pre-PR state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolved sdk/cosmos/.cspell.json by unioning main's new ignoreWords with the five Cosmos diagnostics terms scoped in this PR (flatbuffer, materializer, materializers, uncompacted, underspecified). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
heaths
left a comment
There was a problem hiding this comment.
The cspell-related changes look good. I didn't review the other files.
Salvage the DIAGNOSTICS-CONTRACT.md skeleton from PR Azure#4684 onto a fresh main-based branch and fix its substantive errors: - Correct the OpenTelemetry semconv names to the real ones: db.client.operation.duration, db.response.status_code, db.operation.name, azure.cosmosdb.* (request_charge, consistency.level, response.sub_status_code, active_instance.count) and db.system.name=azure.cosmosdb, replacing the non-existent db.cosmosdb.* names. - Add the DiagnosticsHandler chain as the SDK emission model (D1). - Add tail-based sampling as a concept distinct from DiagnosticsLevel (D4). - Add cross-operation rate limiting (D5), distinct from the per-artifact size bound. - Note the azure_core metrics/backdating gaps and the develop-local-then-upstream plan (D2/D3). - Re-tag DiagnosticsContext and friends as WS0 (not on main yet); only DiagnosticsThresholds is actually on main today. - Add an R1-R9 requirement-to-section map. Documentation only; no code changes and no CHANGELOG entry. Spelling and markdownlint pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@NaluTripician ready to merge? |
|
Superseded by #4789. The diagnostics contract/OpenTelemetry-mapping doc was corrected (real semconv names, handler-chain emission model, tail-based sampling, rate-limiting, and the |
Design-first: driver↔SDK Cosmos diagnostics contract + OpenTelemetry mapping
Documentation only. Defines the driver↔SDK diagnostics contract and its OpenTelemetry mapping as a standalone design doc. Contract-first: the append-only capture engine prototyped in #4619 is a deferred optimization, referenced here only conceptually — this PR does not depend on or modify it, and stands alone on
main.DIAGNOSTICS-CONTRACT.mdArc<DiagnosticsContext>, already onmain) + explicit, lazy materialization into three representations — structured metrics, OTel spans, JSON string. No single fixed format; JSON (the costliest step,3.7µs→6.6µs) is paid only when asked for.DiagnosticsLevelthat bounds high-cardinality transport telemetry (never collection).azure_core's span-attribute constants (incl. theaz.service_request.iddot + private-constant gotchas).Notes for reviewers
CosmosResponse::diagnostics()stays non-optional; additive/non-breaking. No CHANGELOG (doc/design-only).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com