Add Cosmos hedging diagnostics - #5198
Open
Ashley Stanton-Nurse (analogrelay) wants to merge 27 commits into
Open
Add Cosmos hedging diagnostics#5198Ashley Stanton-Nurse (analogrelay) wants to merge 27 commits into
Ashley Stanton-Nurse (analogrelay) wants to merge 27 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 3 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Ashley Stanton-Nurse (analogrelay)
marked this pull request as ready for review
September 1, 2026 16:39
Ashley Stanton-Nurse (analogrelay)
requested a review
from a team
as a code owner
September 1, 2026 16:39
|
Azure Pipelines: Successfully started running 1 pipeline(s). 3 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Copilot started reviewing on behalf of
Ashley Stanton-Nurse (analogrelay)
September 1, 2026 16:40
View session
Contributor
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
| inherent accessors on the driver's `DiagnosticsContext` plus two small public | ||
| value types, all re-exported from `azure_data_cosmos`: | ||
|
|
||
| | Member | Signature | Semantics | |
There was a problem hiding this comment.
NIT: New VsCode Markup Liniting will drastically reformat all tables here - worth doing it now explciitly?
Member
Author
There was a problem hiding this comment.
I'm not getting any reformatting in VS Code, is this something you're seeing with markdown tables? For now I'll merge this as-is and we can deal with reformatting if/when it happens.
Fabian Meiswinkel (FabianMeiswinkel)
approved these changes
Sep 1, 2026
Fabian Meiswinkel (FabianMeiswinkel)
left a comment
Fabian Meiswinkel (FabianMeiswinkel)
left a comment
Member
There was a problem hiding this comment.
LGTM
Re-implement the driver-side Hedging Detection API (H1) on current main, which already carries the merged observability layer and landed hedging implementation (Azure#4432). Refs Azure#4410. Driver (azure_data_cosmos_driver): - Rename `ExecutionContext::Retry` -> `OperationRetry`; keep `Retry` for one release as a `#[deprecated]` alias. Serialized form changes "retry" -> "operation_retry"; update `as_str()` and all dispatch sites in the operation/transport pipelines and driver retry loop. - Add public `RequestedRegion` struct and `RequestedRegionReason` enum (both `#[non_exhaustive]`) plus a total `From<ExecutionContext>` mapping. - Add `DiagnosticsContext::requested_regions()` (dispatch order, duplicates, per-region reason), `responded_regions()` (completion order, service replies only via a new `RequestDiagnostics::responded_with_service_reply()` predicate), and `hedging_started()` (alternate-region / Hedging predicate). - Export the two new types from the diagnostics module. - Add unit tests for the accessors, mapping totality, and the rename; refresh ARCHITECTURE.md and the "retry" JSON expectations. SDK (azure_data_cosmos): - Re-export `RequestedRegion` / `RequestedRegionReason`, mirroring the existing `DiagnosticsContext` re-export. Docs/CHANGELOGs: - Add `docs/HEDGING_DETECTION_API_SPEC.md` reflecting main's reality and the landed API; add CHANGELOG entries to both crates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Build on H1's Hedging Detection API to surface hedging in each completed-operation signal, additive and opt-in where high-cardinality. Driver (azure_data_cosmos_driver): - Add `HedgeTerminalState::as_str()` + `Display` for a stable, low-cardinality snake_case terminal-state value (single source of truth for the attribute / log-field value). - Add `__internal_test_diagnostics_construction`-gated, `#[doc(hidden)]` test seams so the wrapper SDK can build a hedged context: `HedgeDiagnostics::for_testing`, `DiagnosticsContext::for_testing_with_hedge`, and `RequestDiagnostics::with_execution_context_for_testing`. SDK (azure_data_cosmos): - Tracing: when `hedging_started()`, add `hedging_started`, `hedge_region`, `hedge_terminal_state`, and `requested_regions`/`responded_regions` (`string[]`, like `contacted_regions`) to the sampled operation span, and tag the hedge-leg child span (`azure.cosmosdb.request.hedge`). - Metrics: add the opt-in `azure.cosmosdb.client.operation.hedged` counter (`MetricsOptions::with_hedged_metric`), emitted only when hedging fanned out; low-cardinality `hedge_terminal_state` dim always, high-cardinality `hedge_region` dim only under the existing extended-attributes gate. - Logging: add `hedging_started` / `hedge_region` / `hedge_terminal_state` fields to the compact sampled diagnostics line when hedging occurred. - Add shared attribute-name constants and unit tests (hedged vs non-hedged) for each signal using in-memory OTel exporters / a tracing capture layer. Refs Azure#4410. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback on the hedging detection API: - requested_regions(): recover a structurally-dropped hedge fan-out leg from the authoritative hedge_diagnostics (primary->Initial, alternate-> Hedging, in dispatch order) so a clean primary/alternate win no longer omits a dispatched region or contradicts the hedge_region attribute. - aggregate_sub_operations(): propagate a representative hedge_diagnostics (prefer a fanned-out sub-op) so aggregated ops (PATCH) report hedging consistently instead of dropping it. - metrics record_hedged(): guard on hedge_diagnostics so the hedged counter never emits a data point missing the hedge_terminal_state dimension. - Docs/changelogs/spec: clarify that deprecated ExecutionContext::Retry is a distinct variant still serializing "retry" (the wire change comes from dispatch sites emitting OperationRetry); add the SDK breaking-change entry; note that requests is not a guaranteed-complete append-only list and regions_contacted is first-contact order (not sorted); document the recovered-leg behavior and that a dropped hedge leg has no HEDGE_LEG child span (the root span carries the authoritative signal). - Tests: replace impossible both-legs synthetic shapes with production shapes (PrimaryWonAfterHedge, AlternateWon single-leg) and add recovery, PATCH aggregation, PrimaryWonPreThreshold, metric-guard, and HedgeTerminalState::as_str all-variant coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round-2 review follow-up. The both-transient→failover-resolved path leaves a retained Hedging request (so hedging_started() is true) but no recorded hedge terminal outcome (finalize_both_transient deliberately does not stamp hedge_diagnostics on the non-terminal path). The metrics guard already skipped this case, but the logging handler still emitted empty-string hedge_region / hedge_terminal_state fields via unwrap_or_default(). - logging handler: gate the dedicated hedge fields on hedge_diagnostics (with a fanned-out alternate) instead of hedging_started(), so no misleading empty strings are emitted; consistent with the metrics counter. Add a regression test for the both-transient→failover shape. - metrics: correct the guard test's comment — the None case is production- reachable (both-transient then failover success), not an aggregate-only belt-and-suspenders case; document that the counter measures hedges with a resolved terminal outcome. - docs: note that requested_regions() on an aggregated operation recovers only the single representative fan-out's dropped leg (multi-hedge PATCH caveat). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Verify Links / Analyze CI checks failed on two links in HEDGING_DETECTION_API_SPEC.md: - The docs.rs deep link to struct.DiagnosticsContext.html 404s because that page is not yet on the published 'latest' crate docs; point it at the crate root instead, matching the README convention. - The relative link to HEDGING_SPEC.md violated the link guidance (relative links disallowed); switch to the absolute GitHub blob/main URL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Non-blocking doc precision fixes surfaced by the round-3 reviewers:
- operation_pipeline.rs: correct the compute_execution_context doc bullet
("session retry in progress -> OperationRetry", was the stale "Retry").
- diagnostics_context.rs: reframe the hedging_started() doc — hedge_diagnostics()
is a resolved-terminal-outcome surface, not a "was hedging configured" probe;
document that it is None (while hedging_started() stays true) on the
both-transient -> failover path, and that the metric/log surfaces key off it.
- span_builder.rs: document the intentional asymmetry — the tracing span gates
hedge attributes on hedging_started() (surfacing region history for any
fan-out, no empty/placeholder values) while the metric counter and log hedge
fields key off hedge_diagnostics (a resolved terminal outcome).
- CHANGELOGs: add the PR link (Azure#4871) alongside the tracking issue (Azure#4410) on the
hedging feature / surfacing / breaking-change entries, per changelog convention.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The hedge_diagnostics() method doc still claimed Some iff execute_hedged() was entered, which contradicts the updated hedging_started() doc: on the both-transient->failover path execute_hedged() runs but hedge_diagnostics is deliberately left None. Reword to describe it as a resolved-terminal-outcome surface and enumerate the both-transient->failover None case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The hedged counter, the sampled log line and the root span each answered "did this operation hedge?" differently. The counter and the log gated on `hedge_diagnostics()` — a *retained terminal outcome* — while the span gated on `hedging_started()`. That difference was documented as a deliberate asymmetry, but it is really an undercount: a race that ends both-transient and is then resolved by a failover attempt deliberately retains no terminal outcome, so an operation that demonstrably fanned out was dropped from the counter entirely and lost its `hedging_started` field on the log line. Now that `hedging_started()` is materialized from the dispatch-time fan-out log it is the authoritative signal, so all three surfaces gate on it: - `metrics/handler.rs`: `record_hedged` gates on `hedging_started()` and keeps the counter's attribute schema uniform by falling back to an `unresolved` sentinel for `hedge_terminal_state`, so `group by hedge_terminal_state` still never fragments. - `logging/handler.rs`: adds an arm for the fanned-out-without-outcome case. `tracing` field sets are fixed per call site, so this reports `hedging_started` while omitting the per-outcome fields rather than emitting empty strings for them. - `tracing/span_builder.rs`: comment only — the asymmetry it described no longer exists. Both tests that asserted the old drop-it behavior now assert the signal is reported, and the spec gains §5.1 covering the shared rule. Also corrects §5's comparison table, which still described `hedge_diagnostics()` as answering "was a strategy active?". It does not: it is an optional retained race outcome, absent both for a configured-but-ineligible operation and on the both-transient path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 37e127ae-4bbe-4eb4-a533-c6ddd54e884e
`HedgeTerminalState` lives at `azure_data_cosmos_driver::diagnostics::`, not at the crate root, so the reference link on the new `HEDGE_TERMINAL_STATE_UNRESOLVED` sentinel failed to resolve and `cargo doc` with `-D warnings` errored. Switched to plain backticks, matching how `diagnostics/attributes.rs` already refers to the same type. The const is private, so a rustdoc hyperlink adds no navigational value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 37e127ae-4bbe-4eb4-a533-c6ddd54e884e
The Build Analyze CI job runs cSpell over every changed file and failed on two words introduced by this PR. `fanouts` comes from the `hedge_fanouts` builder field; cSpell splits the identifier on `_`, and the dictionary already carried the singular `fanout`. `undercount` is standard English but absent from cSpell's base dictionary. Both are correct as written, so they are ignored rather than reworded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 37e127ae-4bbe-4eb4-a533-c6ddd54e884e
Materializing `requested_regions` / `responded_regions` from the full pre-compaction attempt list is what lets a structurally-dropped hedge leg survive, but on its own it made both lists grow one entry per attempt while `requests()` stayed capped. A 410/429 retry storm therefore produced an O(attempt-count) diagnostics artifact, contradicting the bounded-size guarantee in DIAGNOSTICS-CONTRACT.md section 8 and flowing straight into root-span attributes (section "Span tree"). There were three separate unbounded paths: `complete()`, the concatenation in `aggregate_sub_operations` (a PATCH conflict loop adds a sub-operation per retry, so re-applying the cap there is not redundant), and the span builder consuming both lists. Both histories are now capped at `max_request_diagnostics`, keeping head and tail and eliding the repetitive middle — the same shape `compact_requests` already uses, at whole-list granularity. The head preserves the initial dispatch and any early hedge fan-out; the tail preserves where the operation landed. Truncation is explicit, not silent: `total_requested_regions()` / `total_responded_regions()` report the exact pre-truncation counts, and the span emits a matching `*_total` attribute only when the history was actually truncated, so the normal path carries no redundant integer. Both totals are compared in `PartialEq` for the same reason `total_request_charge` is: after truncation they are no longer derivable from the retained vectors. Also corrects the `hedging_started()` doc, which still described the metric counter and log fields as keying off `hedge_diagnostics` after b81bf39 moved them onto `hedging_started()`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 37e127ae-4bbe-4eb4-a533-c6ddd54e884e
Addresses the remaining review feedback on the hedging detection API. Two related defects made hedged operations misreport diagnostics. Each hedge leg records into a private builder created by `clone_for_hedge_attempt`, and the race cancels the loser by dropping its future -- which also dropped every attempt that leg had *already completed*. A leg that received a `429`, entered the transport pipeline's throttle-retry backoff, and only then lost the race contributed nothing at all: its region vanished from the histories and its RU charge went unbilled in `total_request_charge()`. Separately, `HedgeFanout` recorded its position as `at: parent.requests.len()`, which cannot describe where a fan-out sits relative to attempts held in a concurrently running child builder, so a primary-leg retry dispatched before the alternate fanned out was reported after it. Both share one root cause -- per-leg state that the parent cannot see -- so they get one fix: - `HedgeJournal`, an operation-scoped `Arc<Mutex<..>>` shared by the parent and every leg, allocated lazily on the first `clone_for_hedge_attempt` so the non-hedged path pays nothing. Legs mirror each attempt into it as the attempt reaches a terminal state; `merge_hedge_attempt` retains out the winner's copies, and `complete()` folds the remainder back in. Exactly one record per attempt survives regardless of which leg won. An attempt still in flight when its leg was cancelled observed no reply and is deliberately not recovered. - Ordering moves from list indices to `RequestDiagnostics::started_at`, a free operation-wide monotonic clock. `complete()` sorts the union by it, restoring true global dispatch order across both legs. - `HedgeFanout` becomes a pair of `HedgeLegDispatch` records (region, reason, leg id, launch instant) and is now only a fallback: a leg that dispatched describes itself through its own surviving attempts, so a synthetic entry is emitted solely for a leg cancelled before it ever reached the wire. That case is still real -- `select` polls the primary first, so an already-resolved primary drops the alternate without ever polling it. Documentation: - `bound_region_history` now documents that it is deliberately atomicity-oblivious, and corrects the premise that the two histories are index-paired: `requested_regions` is dispatch-ordered while `responded_regions` is arrival-ordered over the subset that replied, so they routinely differ in both length and order. - `aggregate_sub_operations` documents why bounding per sub-operation and again on the concatenation is intentional rather than a missed cap-sizing decision. - `TRANSPORT_PIPELINE_SPEC.md` no longer claims the primary leg always uses `ExecutionContext::Initial`; it now distinguishes the STAGE 2b race from a STAGE 7 retry-upgraded hedge, and describes the journal. - `HEDGING_DETECTION_API_SPEC.md` replaces the "a dropped leg cannot be lost because the fan-out is recorded up front" reasoning with the journal semantics, and states the compound-bounding rationale. Tests: - Hedging tests now build legs through the real `clone_for_hedge_attempt` /`record_hedge_fanout`/`merge_hedge_attempt` flow via new `spawn_primary_leg`/`spawn_alternate_leg` helpers, so they exercise the production path instead of hand-assembled fan-out records. - New behaviour tests cover a loser that already observed a `429`, a primary retry dispatched before fan-out, interleaved retries on both legs, the both-transient path, and a leg cancelled mid-flight. - A serde insurance test pins the deprecated `ExecutionContext::Retry` to `"retry"` so it cannot silently become an alias for `"operation_retry"` during the deprecation window. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 421e5ae3-c3da-4c9e-bc34-75feaccd0604
The ExecutionContext::Retry -> OperationRetry deprecation broke CI: three pre-existing tests in diagnostics_context.rs still constructed the deprecated variant incidentally, and -D warnings turns the deprecation lint into a hard error, so �zure_data_cosmos_driver failed to compile as a lib test across every clippy and test job. Those three sites (the PATCH sub-operation compaction test and the single-operation request-name test) don't assert anything about the deprecated variant, so they now use OperationRetry, which maps to the same RequestedRegionReason. The tests that deliberately exercise the deprecated variant's serialization contract keep their local #[allow(deprecated)] and are untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22b24639-d6f1-4849-98df-36b2f3c8630b
Replace direct re-exports of RequestedRegion and RequestedRegionReason from �zure_data_cosmos_driver with SDK-owned wrapper types in a new diagnostics::region module. - Add sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs with RequestedRegion, RequestedRegionReason, and From<driver> impls - Update diagnostics/mod.rs to export the SDK-owned types instead of the driver types directly Addresses review feedback that the driver types should not be part of the �zure_data_cosmos public API surface, per the Cosmos versioning rule. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…o ExecutionContext Per reviewer feedback (r3714936305, r3714941471): - Remove ExecutionContext::Retry variant entirely (pre-GA breaking change is fine) - Remove RequestedRegionReason from driver; RequestedRegion.reason is now typed as ExecutionContext directly - Update HedgeLegDispatch.reason, leg_dispatch(), and operation_pipeline.rs to use ExecutionContext directly - Update SDK-owned RequestedRegionReason to convert From<driver::ExecutionContext> (instead of the now-removed From<driver::RequestedRegionReason>) - Delete obsolete tests; bulk-update remaining test uses of RequestedRegionReason::* to ExecutionContext::* - Fix useless .into() in operation_pipeline.rs (clippy) - Update both CHANGELOGs to reflect full removal Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d51868ed-95ef-4dff-b237-4f8226e5ec9a
- Remove tracking issue link from HEDGING_DETECTION_API_SPEC.md header - Remove PR links from spec (lines formerly referencing previous PRs) - Simplify Section 3 in spec: remove deprecated Retry compat text - Remove Retry (deprecated) row from reason-mapping table in spec - Remove Azure#4410 issue links from azure_data_cosmos CHANGELOG entries Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d51868ed-95ef-4dff-b237-4f8226e5ec9a
Resolve upstream/main merge conflicts in Cosmos hedging files and keep compatible changelog entries for both streams. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d51868ed-95ef-4dff-b237-4f8226e5ec9a
Keep PR Azure#4871 conflict resolutions while resetting workflow and action-lock files to remote head so push is not blocked by workflow-scope enforcement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d51868ed-95ef-4dff-b237-4f8226e5ec9a
The hedged field was referenced in the Self { .. } literal but its
let hedged = .. binding was lost in an earlier merge, so every build of
azure_data_cosmos with the metrics feature failed with E0425. This broke
all six ADO pullrequest jobs.
Construct the counter from METRIC_OPERATION_HEDGED with the {operation}
unit, matching how the sibling instruments are built.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Check spelling (cspell) task in the Build Analyze job rejected the British `finalisation` in the module docs for the SDK-owned region types. The repo dictionary standardises on US spellings, so switch to `finalization` rather than widening the dictionary for a doc comment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Restore agentic workflow and link-verification files to upstream main so PR Azure#4871 remains scoped to Cosmos hedging diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 821eaa5c-6f91-4dcd-b744-c098fa1550c6
Preserve hedging detection fields when cloning diagnostics with a new operation name after the rebase. Use the initial execution context for primary hedge fan-out dispatch records, matching the primary attempt itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0dd6592-7dc3-4fb5-804f-d2082c310fe9
Remove duplicate metrics declarations left by the rebase conflict resolution so the metrics feature compiles cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0dd6592-7dc3-4fb5-804f-d2082c310fe9
Clean up a duplicated #[test] attribute left in diagnostics_context after the rebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0dd6592-7dc3-4fb5-804f-d2082c310fe9
Consolidate the overlapping hedging diagnostics notes and clarify the serialized retry-name compatibility impact raised during review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c214b7b0-4151-4d5c-8332-ec57d6f65bf3
Keep the hedging changes concise and link both affected crate entries to the current pull request. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c214b7b0-4151-4d5c-8332-ec57d6f65bf3
Update the binary round-trip tests for the new container client options parameter so all-target builds compile after rebasing on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c214b7b0-4151-4d5c-8332-ec57d6f65bf3
Ashley Stanton-Nurse (analogrelay)
force-pushed
the
ashleyst/cosmos-hedge-obs
branch
from
September 1, 2026 22:34
0acd4a5 to
33089f7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the Cosmos Hedging Detection API on diagnostics: materialized requested/responded region histories, exact pre-truncation counts, and a
hedging_started()fan-out signal that survives compaction, aggregation, and structurally dropped hedge-loser legs. The branch also threads that signal through logging, tracing, and opt-in metrics so hedged operations are reported consistently, including both-transient-to-failover races.Also separates operation-level retries from transport retries by removing the old driver
ExecutionContext::Retryvariant in favor ofOperationRetry, and keepsazure_data_cosmosexposing SDK-owned requested-region value types. Validated with targeted Cosmos build and clippy.Fixes #4410