Add Cosmos client-side observability layer#4789
Conversation
Introduce the SDK-side emission extension point on top of the existing driver DiagnosticsContext foundation: - DiagnosticsHandler trait + ordered DiagnosticsHandlerChain in azure_data_cosmos::diagnostics, invoked once per operation at completion. An empty chain is a zero-overhead no-op. - Register point: CosmosClientBuilder::with_diagnostics_handler and CosmosClientOptions, threaded into the shared ClientContext. - ClientContext::complete_operation completion seam wired into the singleton data/control-plane operation paths (container/database/cosmos clients). - Unit tests with a recording handler proving per-op receipt, deterministic ordering, and empty-chain no-op. Feed/query per-page dispatch, default handler registration, and the built-in metrics/tracing/log handlers are deferred to later workstreams. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a `max_request_diagnostics` cap (default 512, min 16, env `AZURE_COSMOS_DIAGNOSTICS_MAX_REQUEST_DIAGNOSTICS`) to `DiagnosticsOptions` and compact the finalized per-attempt `RequestDiagnostics` list when an operation exceeds it, so a 429/410 retry storm cannot grow a `DiagnosticsContext` — or any artifact derived from it — without bound. Salvages the bounding algorithm from the (capture-engine-based) Azure#4683, re-implemented over the upstream plain `DiagnosticsContext`/`RequestDiagnostics` with no dependency on the closed Azure#4619 capture engine (WS6). - `options/diagnostics_options.rs`: `max_request_diagnostics` field, builder method, env-backed resolution + bounds validation, and option tests. - `diagnostics/compaction.rs` (new): `CompactionInfo`/`CompactedRun` plus `compact_requests` — Phase 1 order-preserving run-length collapse (keep first+last per run + exact count/charge/min/max/P50) and Phase 2 order-robust global key-bucket fallback (keep the cap largest buckets) for region ping-pong and high-cardinality fan-outs. Keys on (region, endpoint, status incl. sub-status, execution context). Every drop is marked explicitly, never silent. - `diagnostics/diagnostics_context.rs`: compact at `complete()` (never mid-operation, so request handles stay valid); keep `request_count()` and `total_request_charge()` exact; add `retained_request_count()` and `compaction()`; surface compaction in detailed and summary JSON (absent — and byte-identical — for non-storm operations); storm unit tests. - `diagnostics/mod.rs`: export `CompactionInfo`/`CompactedRun`. - CHANGELOG, ARCHITECTURE, and cspell dictionary updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implement CosmosMetricsHandler, an off-by-default (otel_metrics feature) DiagnosticsHandler that emits OTel metrics from each completed DiagnosticsContext. Builds on WS2's handler chain; register via CosmosClientBuilder::with_diagnostics_handler. - Stable-first: db.client.operation.duration histogram (seconds) with the stable semconv attributes (db.system.name, db.operation.name, db.collection.name, db.namespace, db.response.status_code, error.type, server.address). Operation-scope tags always on (D7). - Development tier (opt-in): request_charge / returned_rows histograms and active_instance.count (deduped by machine_id), plus development attributes (consistency.level, contacted_regions, sub_status_code, connection.mode). Off by default. - Emitted via the raw opentelemetry metrics API (no azure_core Meter yet, D2); all metric/attribute name literals centralized in one module (D6). - Operation-scope identity carried via CosmosOperationContext on the pipeline Context. - Add an additive, internal-test-only DiagnosticsContext::for_testing_completed constructor (gated behind __internal_test_diagnostics_construction) so the SDK can unit-test emission with a realistic context. Verified with an in-memory OTel meter that db.client.operation.duration is emitted with the correct stable attributes, and that the feature/exporter-absent paths are zero-cost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implements the threshold-gated diagnostics emission layer (WS4 + WS5) on top of the existing driver `DiagnosticsContext` and the WS2 `DiagnosticsHandler` chain. Driver (`azure_data_cosmos_driver`): - Add `DiagnosticsThresholds` (point/non-point latency, RU charge, payload size) with Java-like defaults, configurable via the options chain. - Add `DiagnosticsContext` predicates `is_failure`, `is_completed`, and `is_threshold_violated`, plus an optional `operation_name` seam/accessor (defaults `None`; classifies point vs non-point latency when known). - Add feature-gated test constructors for building rich, backdated contexts. SDK (`azure_data_cosmos`): - WS4 `CosmosTracingHandler` behind the off-by-default `otel_tracing` feature: tail-based sampling (`is_failure || is_threshold_violated`) that reconstructs a backdated OpenTelemetry span tree (operation root + one child per retained attempt) via the raw `opentelemetry` `SpanBuilder`, with database semantic-convention attributes. - WS5 `SamplingLogHandler`: logs a compact `to_json_string` line only for completed failed/threshold-breaching operations, rate-limited by a reusable count-per-interval limiter (~100/min default, failure reserve, one "suppressed N" notice per window) via the `tracing` ecosystem. Tests cover the backdated parent/child span tree with past timestamps, a fast success emitting nothing, and a storm capped to N lines/window with a single suppression notice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nding' into nalutripician/cosmos-observability
…rics' into nalutripician/cosmos-observability
Union all four observability workstreams onto the WS2 handler-chain base: - WS6 (retry-storm bounding): driver max_request_diagnostics, compaction module, retained_request_count()/compaction() accessors. - WS3 (OTel metrics): SDK metrics handler + otel_metrics feature + CosmosOperationContext + driver for_testing_completed test ctor. - WS45 (tail-sampled tracing + sampling log): SDK tracing/logging modules + otel_tracing feature; driver DiagnosticsThresholds, is_failure/ is_completed/is_threshold_violated predicates, operation_name seam, for_testing_with_requests test ctor. Conflicts in diagnostics_context.rs, SDK diagnostics/mod.rs, SDK Cargo.toml, and driver CHANGELOG.md resolved by unioning every workstream's additive changes (all additions retained; conflicts were textual co-location). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
WS2's completion path dispatched an empty azure_core Context, so the metrics and tracing handlers could not see operation identity in production. Populate a CosmosOperationContext (operation name plus database and container) at the ~14 completion call sites routed through ClientContext::complete_operation, and attach it to the Context passed to the handler chain. - Relocate CosmosOperationContext from the otel_metrics-gated metrics module to an always-compiled diagnostics::operation_context module so both the metrics and tracing handlers (and the feature-independent client code that populates it) can use it. Re-exported unconditionally. - complete_operation/dispatch_diagnostics take the identity as a lazy closure, so it is only materialized when a handler is registered, preserving the zero-overhead no-op when the chain is empty. - CosmosTracingHandler now reads the op context from the pipeline Context and uses it as the operation-name fallback (the driver context's operation_name is never set in production) and to emit db.namespace / db.collection.name span attributes. - Fix a merge gap: WS45's for_testing_with_requests test constructor now initializes the total_request_charge and compaction fields WS6 added to DiagnosticsContext. - Add SDK CHANGELOG entries for the handler chain and metrics handler; add "semconv" to the Cosmos cSpell dictionary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds client-side Cosmos observability with pluggable handlers, OpenTelemetry emission, sampled logging, and bounded retry diagnostics.
Changes:
- Adds diagnostics handler registration and operation context APIs.
- Adds feature-gated metrics/tracing handlers and sampled logging.
- Adds diagnostics thresholds, compaction, and exact aggregate counters.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/cosmos/azure_data_cosmos/src/options/client.rs |
Stores registered handlers. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs |
Builds backdated span trees. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/mod.rs |
Exports and tests tracing. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/handler.rs |
Implements tracing handler. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/attributes.rs |
Defines tracing attributes. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/operation_context.rs |
Adds operation identity context. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/mod.rs |
Exports diagnostics APIs. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/options.rs |
Configures optional metrics. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/mod.rs |
Defines metrics module. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/instruments.rs |
Creates OTel instruments. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs |
Records operation metrics. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/attributes.rs |
Defines metric contracts. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/rate_limiter.rs |
Limits sampled logs. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/mod.rs |
Exports and tests logging. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs |
Implements sampled logging. |
sdk/cosmos/azure_data_cosmos/src/diagnostics/handler.rs |
Adds handler trait and chain. |
sdk/cosmos/azure_data_cosmos/src/diagnostics.rs |
Replaced by module directory. |
sdk/cosmos/azure_data_cosmos/src/clients/mod.rs |
Adds singleton completion dispatch. |
sdk/cosmos/azure_data_cosmos/src/clients/database_client.rs |
Adds database operation identities. |
sdk/cosmos/azure_data_cosmos/src/clients/cosmos_client.rs |
Adds account operation identity. |
sdk/cosmos/azure_data_cosmos/src/clients/cosmos_client_builder.rs |
Registers handler chains. |
sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs |
Adds container operation identities. |
sdk/cosmos/azure_data_cosmos/CHANGELOG.md |
Documents observability APIs. |
sdk/cosmos/azure_data_cosmos/Cargo.toml |
Adds OTel features and tests. |
sdk/cosmos/azure_data_cosmos_driver/src/options/mod.rs |
Exports diagnostics thresholds. |
sdk/cosmos/azure_data_cosmos_driver/src/options/diagnostics_thresholds.rs |
Defines sampling thresholds. |
sdk/cosmos/azure_data_cosmos_driver/src/options/diagnostics_options.rs |
Adds request diagnostics cap. |
sdk/cosmos/azure_data_cosmos_driver/src/lib.rs |
Re-exports thresholds. |
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/mod.rs |
Exports compaction metadata. |
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs |
Integrates compaction and predicates. |
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/compaction.rs |
Implements retry-storm compaction. |
sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md |
Documents driver APIs. |
sdk/cosmos/azure_data_cosmos_driver/ARCHITECTURE.md |
Describes diagnostics bounding. |
sdk/cosmos/.cspell.json |
Adds observability terminology. |
Cargo.lock |
Locks OTel dependencies. |
Bring DIAGNOSTICS-CONTRACT.md onto the observability branch (from the former standalone doc PR Azure#4782) and correct its now-stale framing: - The earlier draft assumed the DiagnosticsContext foundation was NOT yet on main and tagged it [WS0] as a blocking-root workstream. In reality the full foundation (DiagnosticsContext, CosmosResponse::diagnostics(), RequestDiagnostics, DiagnosticsVerbosity, DiagnosticsOptions) is already on upstream/main. Re-tag all foundation references [WS0] -> [main] and drop the "WS0 builds the foundation" narrative (WS0 dropped; PR Azure#4785 closed as redundant). - DiagnosticsThresholds was mis-tagged [main]; it is the one diagnostics type NOT on upstream, added by this PR -> re-tag [WS4/WS5]. Correct the regions_contacted() accessor name. - Add WS8 (op-scope wiring) to the legend; note WS2-WS8 ship together in the combined PR Azure#4789; mark DiagnosticsLevel as a [design]-only surface. - Fix the "documentation only" scope note now that the doc rides with the implementation. Keep the OTel semconv names accurate. Doc-only change: cSpell + markdownlint pass; added the doc's technical terms (workstream(s), materializer, underspecified, flatbuffer) to the Cosmos cSpell dictionary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses the 13 Copilot reviewer threads on PR Azure#4789: - Dispatch diagnostics on operation failure via a result-aware complete_result seam, and skip the empty-chain Arc clone (clients). - Thread the handler chain + operation identity into the query and change-feed iterators so paged operations emit once per page, and populate returned_rows from the page item count. - Read the operation name from CosmosOperationContext before the tracing/logging threshold gate (is_threshold_violated_for) so non-point operations use the 3s threshold. - Rebuild aggregate compaction metadata and re-bound the concatenated list in aggregate_sub_operations so request_count() stays exact. - Include total_request_charge in DiagnosticsContext PartialEq. - Derive SafeDebug on CosmosOperationContext. - Count only reserve admissions in the sampling-log failure reserve. - Omit the misleading active_instance.count metric until client lifecycle wiring exists. - Link/condense the SDK and driver CHANGELOG entries (retarget the compaction entry from the closed Azure#4683 to Azure#4789). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment c8 on PR Azure#4789 was resolved by removing the misleading active_instance.count instrument (machine_id is process-wide and the counter never decremented); the CHANGELOG entry dropped it too. The diagnostics contract's operation-level metrics table still listed it as an emitted metric, so annotate that row as deferred until real client-lifecycle wiring exists, keeping the doc consistent with the shipped handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (8)
sdk/cosmos/azure_data_cosmos/src/diagnostics/handler.rs:65
- This parameter is not actually the pipeline/trace context described here. Every completion path constructs a fresh
Context::new()containing onlyCosmosOperationContext(clients/mod.rs:139andfeed/mod.rs:67), so custom handlers cannot observe caller/pipeline values or use this argument to correlate telemetry with the caller trace. Either carry the real operation context through execution or describe/pass a dedicated diagnostics context rather than promising trace correlation.
/// * `cx` - The pipeline/trace [`Context`] associated with the operation,
/// available so handlers can correlate emitted telemetry with the caller's
/// trace context.
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/handler.rs:77
- This public predicate can disagree with what this handler actually does. Production contexts lack an operation name, so for a 2-second
query_itemscontext this call uses the 1-second fallback and returnstrue, whilehandlereceivesCosmosOperationContext("query_items"), uses the 3-second non-point threshold, and skips the span (the test at tracing/mod.rs:120-142 demonstrates exactly this distinction). Accept the operation context/name here or avoid exposing this as the handler's decision.
/// Returns whether the given completed context should emit a span, per the
/// tail-based sampling policy: emit iff the operation failed or crossed a
/// threshold.
pub fn should_emit(&self, diagnostics: &DiagnosticsContext) -> bool {
should_emit_span(diagnostics, &self.thresholds, None)
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs:94
- This public predicate has the same mismatch as the tracing predicate: it always passes no operation identity, while
handleuses the SDK context. A 2-second query therefore returnstruehere but is not logged by the registered handler because the actual non-point threshold is 3 seconds. Accept the operation identity/name or avoid presenting this as the handler's effective decision.
/// Returns whether the given completed context should be logged, per the
/// tail-based sampling gate (before rate limiting).
pub fn should_log(&self, diagnostics: &DiagnosticsContext) -> bool {
should_log(diagnostics, &self.thresholds, None)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:1945
- When any source was already compacted, this re-compacts only its retained first/last exemplars and discards its exact source rollups. For example, a source run representing 600 attempts reaches
compact_requestshere as two records, so the aggregaterunsreports count 2 and the wrong RU/P50 even though the PR promises exact per-run rollups. Re-bound/merge the sourceCompactionInfo::runsrather than recomputing rollups from already-compacted exemplars.
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:1975 - This assigns all collapsed middle attempts to
omitted_request_count, even though their runs are present. In the new 600+400 regression test, both exact source runs are retained andomitted_runs == 0, but this reports 996 omitted requests;CompactionInfodocuments this field as attempts represented by omitted runs. Sum the source omission counts instead.
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2157 - After Phase 2 compaction,
requests.last()is not guaranteed to be the terminal attempt: global buckets are emitted in first-seen key order and then truncated. Thus a status-less error whose final failed key is dropped can becomeis_failure() == false, suppressing exactly the failure span/log this fallback was added to preserve. Store the terminal outcome before compaction (or guarantee that the actual terminal attempt is retained) instead of inferring it from the compacted list.
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:142 - A failure inferred from the terminal request has
op_failed == truebut no operation-level status, so this block adds noerror.typeeven though the span is marked Error below. These status-less failure paths are explicitly supported byDiagnosticsContext::is_failure; emit the terminal status or_OTHERso failed root spans satisfy the error semantic convention.
if op_failed {
if let Some(status) = diagnostics.status() {
root_attrs.push(KeyValue::new(
attributes::ERROR_TYPE,
u16::from(status.status_code()).to_string(),
sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md:423
- This shipped-contract table overstates the implementation. The root builder names spans only with
<operation>(not<operation> <target>), andemit_backdated_span_treeemits neither request timeline events nor a hedge span. Implement these rows or mark them deferred so the contract accurately describes the telemetry users receive.
|
@NaluTripician can you compact the PR description? Feel free to move other content to a doc or comment. |
Resolve the open PR review threads on the diagnostics emission layer: - Capture regions_contacted at finalization from the full attempt list, before compaction, and preserve first-contact (failover) order instead of sorting. A region contacted only by a bucket that global-bucket compaction drops now still surfaces at the operation level, and the metrics/tracing contacted-region attribute keeps its ordered semantics. - Gate the tracing tail-sampler on is_completed() so a context with neither a status nor any attempts no longer emits on duration alone. - Honor a CosmosOperationContext::server_address override on the tracing root span, matching the metrics handler. - Derive the backdated root span's start from the earliest retained attempt so an aggregate operation (whose duration() is the sum of its sub-op durations) still fully contains its children. - Skip the redundant per-page diagnostics Arc clone on the successful query-page path when the handler chain is empty. - Thread ClientContext and operation identity through the offers and throughput-poller paths so read_throughput, begin_replace_throughput, and each poll dispatch the handler chain on success and failure. Adds driver and tracing regression tests covering the region ordering / compaction-survival, completion gate, server-address override, and aggregate root-span window. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
analogrelay
left a comment
There was a problem hiding this comment.
Done an initial review, but not yet finished some of the larger files. Posting some of my comments now before I have to step out for a bit. Will plan to get back to it ASAP!
|
|
||
| /// Builds the SDK-side [`CosmosOperationContext`] for this container's | ||
| /// operations, carrying the operation name plus the database and container | ||
| /// identity the driver context does not know. |
There was a problem hiding this comment.
Surprised the driver doesn't have this context. Doesn't it have these on CosmosOperation?
There was a problem hiding this comment.
It has some of it. CosmosOperation carries the ContainerReference (database + container names) and an OperationType / ResourceType, but two gaps make CosmosOperationContext worthwhile: (1) the handler chain receives the finalized DiagnosticsContext, not the CosmosOperation — the driver's context deliberately doesn't embed the caller-facing identity; and (2) the semconv db.operation.name (read_item vs query_items) is an SDK-level name, finer-grained than the driver's coarse OperationType. The alternative is threading the identity onto the driver's DiagnosticsContext, but that pushes SDK naming concerns into the driver. Happy to go that way if you'd prefer.
| pub struct SamplingLogHandler { | ||
| thresholds: DiagnosticsThresholds, | ||
| limiter: RateLimiter, | ||
| } |
There was a problem hiding this comment.
Should this be a wrapping handler rather than writing directly to tracing? That would be a little more composable IMO. A SamplingLogHandler would hold an Arc<dyn DiagnosticsHandler> and apply the rate limits, only calling the child handler when the rate limits and thresholds allow.
Then, we would have a separate TracingLogHandler which logs to tracing and could theoretically have handlers that emit to other outputs.
I'm totally fine with us have a "default" configuration somewhere that configures SamplingLogHandler wrapping TracingLogHandler so that users don't need to think about it, but I think we should allow them the flexibility.
There was a problem hiding this comment.
Agreed this is more composable. Plan: a SamplingLogHandler that holds an Arc<dyn DiagnosticsHandler> and only calls the child when the thresholds + rate limit allow, plus a separate TracingLogHandler that writes to tracing, and a default that wires SamplingLogHandler around TracingLogHandler. It's a public-API reshape, so I'd like to land it as a focused follow-up once we agree on how thresholds vs rate-limiting split across the two — tracking it. (Made RateLimiterConfig public in the meantime, per the sibling comment.)
There was a problem hiding this comment.
If you're not addressing it in this PR, can you make an issue and parent it to #4822 ?
|
|
||
| /// `db.system.name` — identifies the database system. Always | ||
| /// [`DB_SYSTEM_NAME_VALUE`] for Cosmos DB. | ||
| pub(crate) const DB_SYSTEM_NAME: &str = "db.system.name"; |
There was a problem hiding this comment.
Some duplication here. Maybe just have a top level src/diagnostics/attributes.rs? It's fine if some are dead code when certain features are/are not enabled.
There was a problem hiding this comment.
Agreed there's duplication. Happy to hoist a shared src/diagnostics/attributes.rs with #[allow(dead_code)] for the feature-conditional names. Tracking as a follow-up so it lands as one clean move.
| pub use handler::CosmosTracingHandler; | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Again, would like to see these closer to the actual code-under-test unless there are really multiple separate pieces being tested together here.
There was a problem hiding this comment.
Same as the logging tests — these span the sampling predicate (handler) and the backdated span-tree builder (span_builder) together, so they're integration-level. Can split the pure predicate checks out if you'd prefer.
| mod logging; | ||
| mod operation_context; | ||
|
|
||
| #[cfg(feature = "otel_metrics")] |
There was a problem hiding this comment.
We should avoid otel in the feature name. In fact, I might suggest splitting this up into separate metrics, tracing, and opentelemetry features. Especially since the tracing one shouldn't need a direct dependency on opentelemetry
There was a problem hiding this comment.
Agree on dropping otel. One snag: a feature literally named tracing collides with the non-optional tracing crate dependency, so Cargo rejects it as-is. Options: make the tracing dep optional and gate it, or names like metrics + distributed_tracing, or metrics / tracing sitting on an underlying opentelemetry feature. Which naming do you prefer? Agree the metrics/tracing split shouldn't force an OTel dep on both — I'll wire it up once we pick names.
There was a problem hiding this comment.
distributed_tracing works for me.
| assert_eq!(t.point_operation_latency(), Duration::from_secs(1)); | ||
| assert_eq!(t.non_point_operation_latency(), Duration::from_secs(3)); | ||
| assert_eq!(t.request_charge(), 1000.0); | ||
| assert_eq!(t.payload_size(), 1024 * 1024); |
There was a problem hiding this comment.
Not really useful tests, but I'm not gonna make you remove them :D
There was a problem hiding this comment.
😄 thanks — leaving them in.
Cosmos observability layer — full detailMoved out of the PR description (per review) for reference. This is the per-workstream breakdown and the full verification matrix. Per-workstream breakdown
Also folds in the small driver additions the emission layer depends on and that upstream lacked: VerificationRan on the touched crates (
|
|
@analogrelay done — compacted the description down to Why / What / Verification and moved the per-workstream breakdown plus the full verification matrix into #4789 (comment). Also pushed 74c29b7 addressing the open review threads (region first-contact ordering + compaction survival, tracing completion gate, server-address override, aggregate root-span window, feed zero-overhead guard, and throughput/offers/poller dispatch). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (7)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2190
- Global-bucket compaction does not preserve chronological order and may omit the terminal attempt, so
requests.last()is not reliably the terminal status. For a status-less error after an alternating-key retry storm, the retained list can end in an earlier successful attempt andis_failure()becomes false, suppressing exactly the error telemetry this fallback was added for. Preserve the terminal status from the full list before compaction and use that for the fallback.
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:1970 - When compacted source contexts collectively exceed the cap, this recompacts only their retained exemplars and discards their exact source
CompactedRunrollups. The newrunscounts/charges/durations therefore describe only exemplars, whileomitted_request_count = original - retainedalso counts collapsed attempts that are not from omitted runs, contradicting that field's contract. Merge and re-bound the source rollups rather than rebuilding them from retained records.
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:165 is_failure()intentionally recognizes status-less failures from the terminal attempt, but this block emitserror.typeonly from the absent operation status. Those sampled root spans are marked Error yet violate this PR's contract that failed spans carryerror.type; use the effective terminal status or the same_OTHERfallback as the metrics handler.
if op_failed {
if let Some(status) = diagnostics.status() {
root_attrs.push(KeyValue::new(
attributes::ERROR_TYPE,
u16::from(status.status_code()).to_string(),
));
}
}
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/handler.rs:77
- This public predicate can disagree with
handle: production contexts lack an operation name, soshould_emitapplies the 1-second point fallback, whilehandleusesCosmosOperationContextand applies 3 seconds to queries/batches. For example, it reports true for a 2-second query that the handler will skip. Accept an operation name/context (or clearly expose a separate context-free fallback predicate) so this API answers the behavior it advertises.
/// Returns whether the given completed context should emit a span, per the
/// tail-based sampling policy: emit iff the operation failed or crossed a
/// threshold.
pub fn should_emit(&self, diagnostics: &DiagnosticsContext) -> bool {
should_emit_span(diagnostics, &self.thresholds, None)
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs:94
- This public predicate does not model the handler's actual decision for non-point operations: it uses the 1-second fallback because production diagnostics lack an operation name, whereas
handlereads the operation context and uses the 3-second threshold. Accept operation identity or expose/document this as a context-free fallback to avoid returning the opposite of what the handler will do.
/// Returns whether the given completed context should be logged, per the
/// tail-based sampling gate (before rate limiting).
pub fn should_log(&self, diagnostics: &DiagnosticsContext) -> bool {
should_log(diagnostics, &self.thresholds, None)
sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md:423
- The shipped span builder never reads
RequestDiagnostics::events()orhedge_diagnostics()and emits neither timed events nor a distinct hedge span. Because this document presents itself as the contract implemented by this PR, these rows overstate the tracing output; mark them deferred or implement the described emission.
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:89 - The contract requires a database span name containing the operation and target (for example,
read_item orders), but this always uses only the operation name even when the container is available inCosmosOperationContext. Include the appropriate collection/namespace target so emitted spans follow the documented semantic convention.
let op_name_ref = diagnostics
.operation_name()
.or_else(|| op.and_then(CosmosOperationContext::operation_name));
let op_name = op_name_ref
.unwrap_or(DEFAULT_OPERATION_SPAN_NAME)
.to_string();
Apply the actionable feedback from the maintainer's review: - Soften the firm "zero-overhead" performance assertions in the diagnostics doc comments to plain "does nothing / is skipped" wording. - Make RateLimiterConfig part of the public API and rename the SamplingLogHandler constructor to the public with_thresholds_and_rate_limit, so callers can tune the rate limiter. - Compute the sampled-log JSON inside each tracing macro so to_json_string only runs when a subscriber is actually listening. - Apply the suggested SamplingLogHandler and CosmosTracingHandler doc comments (drop the "(Java-like)" aside). - Remove azure_core module-visibility / design-decision references from the metrics attribute and module docs, and add a UCUM spec reference for the instrument units. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (7)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:1970
- When any source was already compacted,
aggregated_requestscontains only retained exemplars. Re-running compaction over those records rebuildsrunswith exemplar-only counts, charges, and percentiles, discarding the exact source rollups;omitted_request_countthen counts all missing exemplars even when no rollup row was omitted. Preserve and merge the sourceCompactionInforollups when re-bounding the aggregate so these public fields remain internally consistent and exact.
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:1992 - This branch preserves every source rollup row, so
omitted_runscan be zero, but it setsomitted_request_countto all attempts not retained as full records (996 in the existing 600+400 test). That contradicts the field contract that it counts attempts represented by omitted rollup rows. Sum the sources'omitted_request_countvalues instead.
sdk/cosmos/azure_data_cosmos/src/feed/iterator.rs:239 - Taking an owned operation context here forces query callers to construct it before the iterator can check whether the handler chain is empty. Container/database call sites therefore clone names and allocate strings even with no registered handler, contradicting the off-by-default allocation-free path. Accept a lazy factory or an optional context created only when
diagnosticsis non-empty.
diagnostics: DiagnosticsHandlerChain,
op_context: CosmosOperationContext,
sdk/cosmos/azure_data_cosmos/src/feed/change_feed_iterator.rs:267
- The change-feed iterator also requires an eagerly-built operation context, so
ContainerClient::query_change_feedallocates database/container names even when the diagnostics chain is empty. Make this context lazy or optional so the advertised no-handler path remains allocation-free.
diagnostics: DiagnosticsHandlerChain,
op_context: CosmosOperationContext,
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2190
- This fallback assumes the last retained record is the terminal attempt, but global-bucket compaction groups records by first-seen key and may truncate later buckets. In a compacted error path with no operation status,
requests.last()can therefore be a non-terminal success and suppress the failure span/log—the exact storm scenario this layer targets. Capture the effective terminal status from the full request list before compaction and store/use it on the finalized context.
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:113 - For failures whose operation-level status was not stamped, the root span omits
db.response.status_codeand sub-status here (anderror.typebelow), even though the driver recognizes the operation as failed. Use an effective final status preserved before compaction for all root status/error attributes.
if let Some(status) = diagnostics.status() {
sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md:423
- This contract promises behavior the implementation does not provide: root names contain only the operation name (not
<operation> <target>), request timelines are not emitted as events, and hedges are represented only as ordinary request spans rather than a hedge span. Because this document defines the shipped contract, either implement these span shapes or narrow these rows to the actual emitted tree before release.
| // Children hang off the root via its span context so the exporter records the | ||
| // correct parent/child linkage. | ||
| let parent_cx = Context::current().with_remote_span_context(root.span_context().clone()); |
There was a problem hiding this comment.
Good nuance. The manual backdated span tree needs the root to stay a mutable, owned Span so it can be ended with an explicit end_with_timestamp after its children — which fights Context::with_span / mark_span_as_active (both take ownership and hand back only an immutable SpanRef). with_remote_span_context is the only public way to set an explicit parent SpanContext while keeping the owned root. Parent/child linkage is correct; the only exposure is parent-based samplers that treat local vs remote parents differently. I'd like to leave this as a tracked follow-up rather than restructure the backdating flow here — flag if you'd prefer I take it on in this PR.
Address the latest automated review comments: - Correct the CosmosMetricsHandler doc: the Meter is captured from the global provider at construction, so a provider must be installed before the handler is built (or bind an explicit meter with with_meter); otherwise metrics are silently dropped. Drops the misleading "wire an exporter later" claim. - Add end-to-end tests that register a DiagnosticsHandler on a real emulator-backed client and assert the chain fires on singleton success, singleton failure (with a failed context), and paginated success — covering the completion seams against wiring regressions that compile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address the automated re-review comments on the observability layer: - Size-limited diagnostics summary: the truncated fallback no longer re-serializes the full CompactionInfo (up to `cap` endpoint-bearing runs) that likely blew the budget. It now emits a counts-only CompactionSummary, so a storm's "truncated" record stays bounded and SamplingLogHandler cannot emit oversized lines. - Status-less failures: add DiagnosticsContext::effective_status (operation status, else the terminal attempt's) and use it in the metrics and tracing emitters so graft-onto-error finalization paths report an accurate status / error.type instead of the _OTHER catch-all. - preview_dtx: wire commit_distributed_write and execute_distributed_read through the completion seam so the handler chain fires on distributed transaction success and failure. - Correct the DiagnosticsHandler::handle doc: `cx` carries the SDK CosmosOperationContext (operation metadata), not the caller's pipeline/trace context. Adds driver tests for the bounded truncation and the effective-status fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address the latest automated re-review comments: - RateLimiter::new normalizes a zero window (now reachable via the newly public RateLimiterConfig) to the default, so the limiter can't roll over every call and bypass the cap. - The backdated root span now ends at the last retained attempt's completion rather than the handler's invocation time, so an earlier handler's delay no longer inflates or shifts the reconstructed span. The per-attempt end computation is shared with the root via a helper. - Point the DIAGNOSTICS-CONTRACT.md `[ctx]` reference at diagnostics/diagnostics_context.rs instead of models/cosmos_response.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (6)
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:148
- The fallback contradicts the comment and the metrics handler: metrics uses the last contacted endpoint, while this root span uses the first. After a retry or regional failover, the same operation therefore reports different
server.addressvalues in metrics and tracing. Use the terminal request here as well; child spans still retain each attempt's actual address.
}
root_attrs.push(KeyValue::new(
attributes::AZURE_COSMOSDB_REQUEST_CHARGE,
diagnostics.total_request_charge().value(),
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:89
- The emitted root span name omits the target even when
container_nameis available. This conflicts with the PR's diagnostics contract (<operation> <target>) and the OpenTelemetry database convention ({db.operation.name} {target}), producingread_iteminstead of e.g.read_item orders. Include the low-cardinality container target in the span name and update the span-name assertions.
// operation's `duration()` already reaches back to before its first attempt,
// but an aggregate operation's `duration()` is the SUM of its sub-op
// durations (see `DiagnosticsContext::aggregate_sub_operations`) and omits
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:1993
- Recompacting
aggregated_requestsloses exact source rollups because this vector contains only each already-compacted source's retained exemplars. If multiple compacted PATCH sub-operations contribute more thancapexemplars, the new run counts, RU totals, and P50 values are recomputed from those exemplars (often two records per original hundreds-attempt run), soCompactionInfo::runsis no longer exact. Rebound while merging the sourceCompactionInfostatistics rather than treating retained samples as the full attempts.
sdk/cosmos/azure_data_cosmos/src/feed/iterator.rs:239 - Requiring a fully built operation context here makes every query call construct it before the iterator knows whether the handler chain is empty. Database/container query call sites consequently clone database/container strings on the default no-handler path, contradicting the PR's allocation-free empty-chain guarantee. Store an optional dispatch bundle or lazy factory and construct the context only when the chain is non-empty.
options: OperationOptions,
diagnostics: DiagnosticsHandlerChain,
op_context: CosmosOperationContext,
sdk/cosmos/azure_data_cosmos/src/feed/change_feed_iterator.rs:267
- As with the query iterator, this by-value context forces
query_change_feedto clone the database and container names even when no diagnostics handler is registered. That adds steady-state allocations to the supposedly allocation-free default path. Make diagnostics state optional or lazy so the operation context is only created for a non-empty chain.
options: OperationOptions,
diagnostics: DiagnosticsHandlerChain,
op_context: CosmosOperationContext,
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/rate_limiter.rs:40
windowacceptsDuration::ZERO, butcheckthen rolls the window on every call (elapsed >= 0) and resetsemitted, allowing every event through and completely bypassing storm protection. Validate a non-zero window at this public configuration boundary or define zero with a safe explicit behavior.
/// Maximum number of emissions allowed per [`window`](Self::window).
pub max_per_window: u32,
/// Length of a rate-limiting window.
pub window: Duration,
/// Number of failures permitted per window *in addition to* the normal cap,
/// so a bounded number of failures is always logged during a storm.
pub failure_reserve: u32,
| pub fn with_thresholds(thresholds: DiagnosticsThresholds) -> Self { | ||
| Self { | ||
| thresholds, | ||
| tracer: global::tracer(TRACER_NAME), | ||
| } |
| pub fn with_request_charge(mut self, request_charge: f64) -> Self { | ||
| self.request_charge = request_charge; | ||
| self |
| //! - **Stable (always on):** `db.client.operation.duration` (histogram, seconds). | ||
| //! - **Development (opt-in via [`MetricsOptions`]):** | ||
| //! `azure.cosmosdb.client.operation.request_charge` and | ||
| //! `db.client.response.returned_rows`. |
| if let Some(page_diagnostics) = err.diagnostics() { | ||
| crate::feed::dispatch_page_diagnostics( | ||
| this.diagnostics, | ||
| this.op_context, | ||
| &page_diagnostics, |
| let keep: Vec<bool> = if total_runs > cap { | ||
| let mut ranked: Vec<usize> = (0..total_runs).collect(); | ||
| ranked.sort_by(|&a, &b| counts[b].cmp(&counts[a]).then(a.cmp(&b))); | ||
| let mut kept = vec![false; total_runs]; | ||
| for &i in ranked.iter().take(cap) { | ||
| kept[i] = true; |
| if !should_emit_span(diagnostics, &self.thresholds, op) { | ||
| return; | ||
| } | ||
| emit_backdated_span_tree( | ||
| &self.tracer, |
| } | ||
|
|
||
| impl DiagnosticsHandler for CountingHandler { | ||
| fn handle(&self, diagnostics: &DiagnosticsContext, _cx: &Context<'_>) { |
| assert!( | ||
| handler.total() > before, | ||
| "the handler chain must fire on a singleton success" | ||
| ); |
| assert!( | ||
| handler.total() > before_total, | ||
| "the handler chain must fire on a singleton failure" | ||
| ); |
| assert!( | ||
| handler.total() > before, | ||
| "the handler chain must fire for query pages" | ||
| ); |
Why
Long-running Cosmos benchmarks and production workloads need observability that is quiet at steady state and rich on error: enough signal to root-cause a short error window in a 10–12h run without paying a steady-state CPU/telemetry tax. Azure's
mainalready ships the always-collectedDiagnosticsContextfoundation; what was missing is the SDK emission layer that decides what to emit from it, plus bounding of the context under retry storms.This PR adds that layer. It builds on top of the existing
DiagnosticsContextand is additive and off-by-default: the new OpenTelemetry code is behind theotel_metrics/otel_tracingfeatures, and with no handler registered the completion path is a zero-overhead no-op, so merging this cannot change existing behavior.What
A pluggable
DiagnosticsHandlerchain onazure_data_cosmos, three built-in handlers, and driver-side support for thresholds and bounded diagnostics. Register handlers viaCosmosClientBuilder::with_diagnostics_handler.DiagnosticsHandlertrait + orderedDiagnosticsHandlerChain, invoked once per operation at completion; empty chain = zero overhead.otel_metrics) —CosmosMetricsHandleremits the stabledb.client.operation.durationhistogram, plus opt-in development-tier metrics.otel_tracing) —CosmosTracingHandlerreconstructs a backdated span tree only for failures / threshold breaches; fast successful reads emit nothing.SamplingLogHandlerlogs a compact JSON line for failed/slow operations, capped per window.DiagnosticsOptions::max_request_diagnosticscaps the per-attempt list under a retry storm while keeping counts and charges exact and surfacing every truncation.CosmosOperationContext(operation name + database + container) is attached at each completion call site so handlers carry realdb.*attributes, built lazily to keep the empty-chain path allocation-free.A per-workstream breakdown and the full verification matrix are in this comment.
Verification
cargo fmt --check,cargo clippy --all-targets, andcargo testare clean on the touched crates across default,otel_metrics, andotel_tracing;cspellis clean. (--all-featuresskipped locally: it fails onopenssl-sys, an unrelated Windows/env issue.)Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com