Add Cosmos hedging detection API + observability surfacing - #4871
Add Cosmos hedging detection API + observability surfacing#4871Nalu Tripician (NaluTripician) wants to merge 30 commits into
Conversation
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>
|
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 Cosmos hedging detection APIs and surfaces hedge activity through tracing, metrics, and sampled logging.
Changes:
- Adds region/hedging diagnostics APIs and renames operation retries.
- Adds hedging telemetry attributes, logging fields, and an opt-in metric.
- Updates tests, specifications, architecture documentation, and changelogs.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
azure_data_cosmos/src/diagnostics/tracing/span_builder.rs |
Emits hedging span attributes. |
azure_data_cosmos/src/diagnostics/tracing/mod.rs |
Tests hedging trace output. |
azure_data_cosmos/src/diagnostics/mod.rs |
Re-exports requested-region types. |
azure_data_cosmos/src/diagnostics/metrics/options.rs |
Adds hedged-metric configuration. |
azure_data_cosmos/src/diagnostics/metrics/instruments.rs |
Defines the hedged counter. |
azure_data_cosmos/src/diagnostics/metrics/handler.rs |
Records and tests hedged metrics. |
azure_data_cosmos/src/diagnostics/metrics/attributes.rs |
Defines metric names and dimensions. |
azure_data_cosmos/src/diagnostics/logging/mod.rs |
Tests hedging log fields. |
azure_data_cosmos/src/diagnostics/logging/handler.rs |
Emits hedging log fields. |
azure_data_cosmos/src/diagnostics/attributes.rs |
Defines shared telemetry attributes. |
azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md |
Documents the detection API. |
azure_data_cosmos/CHANGELOG.md |
Records SDK-facing features. |
azure_data_cosmos_driver/src/driver/transport/transport_pipeline.rs |
Uses OperationRetry. |
azure_data_cosmos_driver/src/driver/pipeline/operation_pipeline.rs |
Updates retry dispatch contexts. |
azure_data_cosmos_driver/src/driver/pipeline/hedging_diagnostics.rs |
Adds terminal-state formatting. |
azure_data_cosmos_driver/src/driver/cosmos_driver.rs |
Updates retry context assignment. |
azure_data_cosmos_driver/src/diagnostics/mod.rs |
Exports requested-region types. |
azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs |
Implements hedging detection APIs. |
azure_data_cosmos_driver/CHANGELOG.md |
Documents driver API changes. |
azure_data_cosmos_driver/ARCHITECTURE.md |
Updates retry terminology and examples. |
Comments suppressed due to low confidence (3)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2388
- This loses hedging on aggregated PATCH diagnostics. PATCH executes its internal
Readwith the caller'sOperationOptions, so that sub-operation can hedge, butaggregate_sub_operationsunconditionally setshedge_diagnostics: Noneat line 2211. ForPrimaryWonAfterHedge, the losing hedge request is also dropped, leaving neither signal and making this returnfalse; for an alternate win it returnstruebut observability has no terminal state/region. Preserve an aggregate hedging summary and cover both winner cases.
pub fn hedging_started(&self) -> bool {
self.hedge_diagnostics
.as_ref()
.map(|hd| hd.alternate_region().is_some())
.unwrap_or(false)
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:92
Retryis not an enum-variant alias: it remains a separate constructible value and still serializes as"retry". The wire change applies to driver-generated retries because dispatch sites now produceOperationRetry. The compatibility section should state this behavioral distinction explicitly.
**Compatibility.** The old `Retry` variant is retained for one release as a
`#[deprecated]` alias so existing source keeps compiling. The **serialized form
changes from `"retry"` to `"operation_retry"`**: telemetry parsers that match on
the literal `"retry"` must update. (`ExecutionContext` derives `Serialize` only,
not `Deserialize`, so no `#[serde(alias)]` is needed.)
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:38
regions_contacted()is not sorted. It preserves first-contact order while deduplicating, as documented and implemented indiagnostics_context.rs:2295-2306. Correct this table because failover order is semantically significant.
| `DiagnosticsContext::regions_contacted` | `-> Vec<Region>` | **Sorted and deduplicated** distinct regions — not dispatch order. |
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2374
- The retained request vector is not always in dispatch order: concurrent hedge builders are appended when they finish. When the alternate returns transient first and the primary later wins,
merge_hedge_attemptleavesHedgingbeforeInitial, so this accessor violates its dispatch-order contract. Sort retained requests bystarted_at()before projecting them.
let mut regions: Vec<RequestedRegion> = self
.requests
.iter()
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2399
- Appending a recovered alternate at the end breaks dispatch order for aggregated PATCH diagnostics. For a hedged Read followed by a Replace, a dropped Read hedge is appended after the Replace attempts even though it was dispatched during the Read. Preserve the source/sub-operation position for recovered legs instead of always appending.
if !has_hedge_leg && !is_sentinel(alternate) {
regions.push(RequestedRegion {
region: alternate.clone(),
reason: RequestedRegionReason::Hedging,
});
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:184
- A both-transient hedge followed by a successful non-hedged fallback has
hedging_started() == truebut noHedgeDiagnostics. This block then emits onlyhedging_startedand silently omits the advertised hedge region and terminal state. The driver needs to retain the hedge race outcome through fallback so sampled spans have a consistent hedge schema.
if diagnostics.hedging_started() {
root_attrs.push(KeyValue::new(attributes::HEDGING_STARTED, true));
if let Some(hedge) = diagnostics.hedge_diagnostics() {
if let Some(alternate) = hedge.alternate_region() {
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (5)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2382
- This derives both region lists from
self.requests, but that is already the compacted list (DiagnosticsContextBuilder::complete, lines 1810-1824). Run compaction collapses duplicate retries and can omit whole buckets, sorequested_regions()no longer contains every dispatch andresponded_regions()can omit actual service replies, despite these new APIs promising duplicates and dispatch/completion order. Capture the ordered projections from the full attempt list before compaction (and concatenate those projections during aggregation) instead of reconstructing them from retained diagnostics.
let mut regions: Vec<RequestedRegion> = self
.requests
.iter()
.filter_map(|r| {
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:291
- This condition detects all fan-outs, but
record_hedgedimmediately returns whenhedge_diagnostics()is absent. That happens for a both-transient hedge followed by a successful failover, so the documented “operations that dispatched a cross-region hedge” counter undercounts real hedges (the new test at lines 715-765 codifies the omission). Preserve the hedge race outcome through fallback, or define a terminal-state value that lets this counter increment for everyhedging_started()operation.
// Hedging counter: emitted only when opted in and a hedge actually
// fanned out. Reuses H1's hedging_started() detection.
if self.options.hedged_metric_enabled() && diagnostics.hedging_started() {
self.record_hedged(diagnostics, &attributes);
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs:98
- Gating on
hedge_diagnosticssuppresses evenhedging_startedfor a real both-transient fan-out that later succeeds via failover, despite the logging contract saying hedging fields are added when hedging occurred. Branch ondiagnostics.hedging_started()for the boolean signal and preserve/conditionally emit the outcome fields; ideally retain the completed hedge race diagnostics in the driver so logging, metrics, and tracing agree.
if let Some(hedge) = diagnostics
.hedge_diagnostics()
.filter(|hedge| hedge.alternate_region().is_some())
{
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:202
- This is not an append-only attempt list: finalized diagnostics compact it when
max_request_diagnosticsis exceeded, andhedging_started()returns a boolean rather than a collection. Describerequested_regions()/responded_regions()as projections of the retained, potentially compacted list and document the separate boolean computation accurately.
The three accessors return owned/borrowed collections computed on demand from
the append-only attempt list, so callers allocate only when they read a derived
collection. If `ExecutionContext` becomes a prominent part of the public
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2372
- The retained vector is not always in dispatch order for hedges. In a both-transient race,
execute_hedgedmerges whichever future completes first and then the partner (operation_pipeline.rs:3332-3416, 3484-3563), so an alternate that finishes first is stored before the primary and this accessor returns[Hedging, Initial]. Order by a dispatch timestamp/sequence captured when each request starts rather than relying on merge insertion order.
/// Order is the retained attempts' dispatch (insertion) order, with the two
/// fan-out legs placed in dispatch order (the `Initial` primary before the
/// `Hedging` alternate). Under a `429`/`410` retry storm the retained list
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (7)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2382
self.requestsis not always in dispatch order after finalization: the global-bucket compactor groups records in first-seen bucket order (diagnostics/compaction.rs:359-380). Iterating it directly can therefore returnA, A, B, Bfor an actualA, B, A, Bdispatch sequence, violating this API's ordering contract. Sort retained records bystarted_at()before projecting them.
let mut regions: Vec<RequestedRegion> = self
.requests
.iter()
.filter_map(|r| {
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2402
- Checking only
(reason, region)treats an earlier dispatch as the missing hedge leg, even though this API explicitly preserves duplicates. For example, an aggregated PATCH with an earlierInitialrequest to East US and a laterAlternateWonrace from East US will suppress recovery of the dropped primary, so one actual dispatch is absent. Recovery needs race identity/terminal-state semantics rather than deduplication by region and reason; aggregates may need per-sub-operation hedge diagnostics.
let has_hedge_leg = regions
.iter()
.any(|r| r.reason == RequestedRegionReason::Hedging && &r.region == alternate);
if !has_hedge_leg && !is_sentinel(alternate) {
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2462
- This derives responses only from the compacted retained request list. Retry-storm compaction drops middle replies (and can omit entire buckets), so
responded_regions()can omit service responses and duplicates despite the public contract saying each responding request contributes an entry. Capture the completion-ordered response regions from the full attempt list before compaction, asregions_contactedalready does.
let mut responded: Vec<&RequestDiagnostics> = self
.requests
.iter()
.filter(|r| r.responded_with_service_reply())
.collect();
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2492
- This can become a false negative after a both-transient hedge falls back successfully. That path intentionally leaves
hedge_diagnosticsunset, and retry-storm compaction can omit the retainedHedgingrequest, making both sides of this disjunction false even though fan-out occurred. Persist an operation-level fan-out flag before compaction and propagate it through aggregated contexts so this detection API remains exact.
self.hedge_diagnostics
.as_ref()
.map(|hd| hd.alternate_region().is_some())
.unwrap_or(false)
|| self
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:291
- The enabled counter still drops a real fan-out when
hedging_started()is true buthedge_diagnostics()is absent (the both-transient-then-failover-success path covered by the new test). This contradictswith_hedged_metric's public contract that the counter counts operations that dispatched a hedge and systematically undercounts the failure mode operators most need to measure. Preserve the race outcome through fallback, or define a bounded terminal-state value for this path, rather than silently returning fromrecord_hedged.
// Hedging counter: emitted only when opted in and a hedge actually
// fanned out. Reuses H1's hedging_started() detection.
if self.options.hedged_metric_enabled() && diagnostics.hedging_started() {
self.record_hedged(diagnostics, &attributes);
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs:98
- Gating on
hedge_diagnosticshideshedging_startedas well as the unavailable terminal fields for a both-transient hedge followed by failover success. The operation did fan out, so the sampled line currently contradicts the logging contract and disagrees with the tracing root, which does emithedging_started = true. Gate the boolean ondiagnostics.hedging_started()and make only genuinely unavailable fields conditional.
if let Some(hedge) = diagnostics
.hedge_diagnostics()
.filter(|hedge| hedge.alternate_region().is_some())
{
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:202
- The attempt list is explicitly not append-only after finalization: §2 notes that compaction can drop and reorder records, and clean hedge losers are absent. Calling it append-only contradicts the implemented data source and overstates what these derived collections contain.
The three accessors return owned/borrowed collections computed on demand from
the append-only attempt list, so callers allocate only when they read a derived
collection. If `ExecutionContext` becomes a prominent part of the public
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2406
pushdoes not preserve dispatch order for aggregated operations. PATCH appends each Read context before its later Replace context (patch_handler.rs:216-219,281-289), so when the Read hedge loser is absent, this appends its recovered alternate after the Replace request even though the hedge was dispatched first. Multiple hedged sub-operations can also makehas_hedge_legmatch a different sub-operation. Preserve the recovery data at each source boundary (or retain an explicit full dispatch history) before aggregation rather than reconstructing it from one representativeHedgeDiagnostics.
if !has_hedge_leg && !is_sentinel(alternate) {
regions.push(RequestedRegion {
region: alternate.clone(),
reason: RequestedRegionReason::Hedging,
});
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2462
- This only examines the compacted request list, so it does not satisfy the documented “each request that produced a service reply” contract during retry storms.
complete()runscompact_requestsbefore constructing this context (diagnostics_context.rs:1810-1824), and compaction deliberately drops middle responses and sometimes entire buckets (compaction.rs:209-225). Capture the response-region completion history before compaction (as is already done forregions_contacted) so this accessor remains complete and ordered.
let mut responded: Vec<&RequestDiagnostics> = self
.requests
.iter()
.filter(|r| r.responded_with_service_reply())
.collect();
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2504
- The request-based fallback is not durable across diagnostics compaction. On the both-transient→failover-success path there is intentionally no
hedge_diagnostics; if the subsequent retry storm triggers global bucket compaction, the one-offHedgingbucket can be discarded because only first/final and largest buckets are retained (compaction.rs:262-335).hedging_started()can then return false even though fan-out occurred. Persist a fan-out signal independently of the compacted attempts (or retain the hedge-race diagnostics through fallback).
self.hedge_diagnostics
.as_ref()
.map(|hd| hd.alternate_region().is_some())
.unwrap_or(false)
|| self
.requests
.iter()
.any(|r| matches!(r.execution_context(), ExecutionContext::Hedging))
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:230
- This silently undercounts the public “operations that dispatched a cross-region hedge fan-out” counter. The handler first confirms
hedging_started(), butrecord_hedgeddrops the known both-transient→failover-success case because that path has nohedge_diagnostics(the new test at lines 715-765 codifies the omission). Preserve the hedge race’sBothTransientoutcome through fallback, or otherwise define a terminal-state value for this case, so every actual fan-out increments the counter asMetricsOptions::with_hedged_metricpromises.
fn record_hedged(&self, diagnostics: &DiagnosticsContext, base_attrs: &[KeyValue]) {
let Some(hedge) = diagnostics.hedge_diagnostics() else {
return;
};
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs:98
- Gating the whole field set on
hedge_diagnosticsomitshedging_startedfor a real fan-out on the both-transient→failover path, contradicting the logging contract that this field is added when hedging occurred. Avoiding empty region/state values does not require dropping the boolean: emithedging_started = truewheneverdiagnostics.hedging_started()is true, and add region/state only when available (or preserve the hedge race diagnostics in the driver).
if let Some(hedge) = diagnostics
.hedge_diagnostics()
.filter(|hedge| hedge.alternate_region().is_some())
{
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:125
hedge_diagnostics().is_some()is not a reliable “strategy configured” check. The newDiagnosticsContext::hedging_starteddocumentation explicitly says it is alsoNonefor the both-transient→failover path and when a configured strategy is ineligible. Please describe this as availability of a recorded hedge outcome, or add a separate configuration/activation signal.
`true` iff at least one hedge arm was actually dispatched. This is `false` — not
an error — when the primary returns before the hedging threshold elapses, even
though a hedging strategy was active. To check whether a strategy was merely
*configured*, use `ctx.hedge_diagnostics().is_some()` (a superset that includes
primary-wins-under-threshold).
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2504
- On the both-transient→failover path,
hedge_diagnosticsis intentionallyNone, so this relies entirely on a retainedHedgingrequest.DiagnosticsContextBuilder::completecompactsrequests, and the global-bucket fallback can omit a non-first/non-final one-off hedge bucket; this then returnsfalseeven though fan-out occurred. Persist the fan-out signal from the full pre-compaction list (or retain the hedge race diagnostics), and add a compacted retry-storm regression test.
|| self
.requests
.iter()
.any(|r| matches!(r.execution_context(), ExecutionContext::Hedging))
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2462
- This derives responses from the post-compaction request list. Once a retry run exceeds
max_request_diagnostics, compaction retains only selected records, so real service replies and duplicate region entries disappear even though this API promises one entry per responding request in completion order. Capture the response-region timeline from the full list before compaction and preserve it on the finalized/aggregated context.
let mut responded: Vec<&RequestDiagnostics> = self
.requests
.iter()
.filter(|r| r.responded_with_service_reply())
.collect();
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2234
- A single representative
HedgeDiagnosticsloses which sub-operation it belongs to. For example, if a PATCH read's alternate is structurally dropped and a later Replace runs,requested_regions()appends the recovered alternate after the Replace; if the alternate won, a later same-regionInitialcan also suppress recovery of the dropped primary. Both violate the promised dispatch order/duplicate semantics. Preserve each source's recovered hedge legs and source position during aggregation rather than attaching an unpositioned representative.
hedge_diagnostics: sources
.iter()
.rev()
.find_map(|c| {
c.hedge_diagnostics
.clone()
.filter(|hd| hd.alternate_region().is_some())
})
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:230
- This return silently drops a real fan-out from the counter when both hedge legs are transient and a later failover resolves the operation:
hedging_started()is true, but this path deliberately leaveshedge_diagnostics()unset. The public option and instrument describe all operations that dispatched a hedge, so this undercounts them. Preserve theBothTransienthedge-race outcome (even when the overall operation later succeeds) or otherwise provide a terminal-state value before recording.
let Some(hedge) = diagnostics.hedge_diagnostics() else {
return;
};
sdk/cosmos/azure_data_cosmos/src/diagnostics/logging/handler.rs:98
- A both-transient hedge followed by successful failover has
hedging_started() == truebut nohedge_diagnostics(), so it falls through and emits none of the new hedge fields—not even the independently knownhedging_started. This contradicts the logging contract for sampled operations where fan-out occurred. Preserve the race outcome or add a fan-out branch that emits the available signal instead of treating it as non-hedged.
if let Some(hedge) = diagnostics
.hedge_diagnostics()
.filter(|hedge| hedge.alternate_region().is_some())
{
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:200
- For a both-transient hedge later resolved by failover, this condition is false even though the outer
hedging_started()gate is true. The root span therefore omitshedge_regionandhedge_terminal_state, despite the PR's tracing contract applying those attributes whenever fan-out occurs. Retain theBothTransientrace diagnostics across fallback (or expose equivalent outcome data) so this path can emit the same schema.
if let Some(hedge) = diagnostics.hedge_diagnostics() {
if let Some(alternate) = hedge.alternate_region() {
root_attrs.push(KeyValue::new(
attributes::HEDGE_REGION,
alternate.as_str().to_string(),
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:125
- This recommendation is incorrect: the updated driver docs explicitly state that
hedge_diagnostics()is alsoNonewhen hedging is configured but ineligible and on a both-transient hedge later resolved by failover. It therefore cannot probe whether a strategy was configured. Please reconcile this statement and the same claims in the building-block and §5 tables with the actual accessor semantics.
`true` iff at least one hedge arm was actually dispatched. This is `false` — not
an error — when the primary returns before the hedging threshold elapses, even
though a hedging strategy was active. To check whether a strategy was merely
*configured*, use `ctx.hedge_diagnostics().is_some()` (a superset that includes
primary-wins-under-threshold).
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:202
- The attempt list is not append-only after finalization; §2 correctly says it may be compacted, reordered, and lose a structurally cancelled hedge leg. Describing all three accessors as derived from an append-only list contradicts the implemented semantics. Refer to the retained/compacted list and the hedge-recovery behavior instead.
The three accessors return owned/borrowed collections computed on demand from
the append-only attempt list, so callers allocate only when they read a derived
collection. If `ExecutionContext` becomes a prominent part of the public
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
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated 2 comments.
Files excluded by content exclusion policy (2)
- .github/workflows/issue-triage.lock.yml
- .github/workflows/review-sdk.lock.yml
Suppressed comments (2)
sdk/cosmos/azure_data_cosmos/src/diagnostics/mod.rs:55
- These SDK-owned types are disconnected from the advertised accessor.
DiagnosticsContextabove is still the re-exported driver type, so its inherentrequested_regions()returnsazure_data_cosmos_driver::diagnostics::RequestedRegionwith anExecutionContextreason—not either type exported here. Consequently SDK consumers cannot obtain these types fromDiagnosticsContextas documented. Add an SDK-owned context wrapper/adapter that projects the return value (consistent withsdk/cosmos/AGENTS.md:139-149), or keep the driver types as the actual public contract.
pub use region::{RequestedRegion, RequestedRegionReason};
sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs:80
- Silently mapping every future driver execution context to
Initialproduces false diagnostics and defeats the claimed exhaustive mapping: the spec says a new driver variant should force this projection to be updated. Because the driver enum is non-exhaustive across this crate boundary, use an explicitUnknownreason or a fallible conversion rather than misclassifying unknown reasons as initial dispatches.
// The driver enum is #[non_exhaustive]; map any future variants to
// the closest known reason rather than panicking.
_ => RequestedRegionReason::Initial,
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>
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (2)
- .github/workflows/issue-triage.lock.yml
- .github/workflows/review-sdk.lock.yml
Suppressed comments (4)
sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs:90
- This SDK-owned type is not actually returned by the linked accessor.
DiagnosticsContextis re-exported directly from the driver (diagnostics/mod.rs:43-46), so its inherentrequested_regions()returnsVec<azure_data_cosmos_driver::diagnostics::RequestedRegion>and this conversion is never applied. Consumers therefore get a distinct same-named type whosereasonis stillExecutionContext. Wire the projection through an SDK-owned context or extension API, or expose the actual returned type consistently.
/// Realizes the cross-SDK Hedging Detection API's `RequestedRegion` value type.
/// Returned by
/// [`DiagnosticsContext::requested_regions`](crate::diagnostics::DiagnosticsContext::requested_regions).
.github/workflows/issue-triage.md:48
- Step 4 of this workflow requires reading
.github/CODEOWNERS(line 207), but repository file access is provided by therepostoolset, not byissuesorpull_requests. With this removal, the agent cannot perform its required owner lookup even thoughcontents: readpermission remains. Restorereposto the toolset list.
toolsets: [issues, pull_requests]
sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs:80
- Because the driver enum is
#[non_exhaustive], any future execution context reaches this arm and is falsely reported as an initial dispatch. That silently corrupts diagnostics and contradicts the spec's claim that the mapping is total. Add an explicitUnknown/Otherreason and map future variants to it instead of assigningInitialsemantics.
This issue also appears on line 88 of the same file.
// The driver enum is #[non_exhaustive]; map any future variants to
// the closest known reason rather than panicking.
_ => RequestedRegionReason::Initial,
sdk/cosmos/azure_data_cosmos/src/diagnostics/tracing/span_builder.rs:349
- The emitted root attribute is
hedging_started, nothedge_started. Keeping the wrong name here makes the fallback guidance misleading when correlating child and root spans.
// (`hedge_started` / `hedge_region` / `hedge_terminal_state`, plus the
// alternate region in `requested_regions`), so the fan-out is still
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>
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (2)
- .github/workflows/issue-triage.lock.yml
- .github/workflows/review-sdk.lock.yml
Suppressed comments (3)
sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs:107
- This conversion is not wired to the advertised accessor.
azure_data_cosmos::diagnostics::DiagnosticsContextis a direct re-export of the driver type, so its inherentrequested_regions()method returnsVec<azure_data_cosmos_driver::diagnostics::RequestedRegion>(whose reason isExecutionContext), not this SDK-ownedRequestedRegion/RequestedRegionReason; no production use of thisFromimplementation exists. As a result, consumers cannot obtain the newly exported SDK type from the documented API. Either expose an SDK-owned diagnostics wrapper/adapter that performs this projection, or re-export the actual driver return types and document that coupling.
impl From<azure_data_cosmos_driver::diagnostics::RequestedRegion> for RequestedRegion {
fn from(driver: azure_data_cosmos_driver::diagnostics::RequestedRegion) -> RequestedRegion {
RequestedRegion {
region: driver.region,
reason: driver.reason.into(),
.github/workflows/verify-links.yml:33
- This Cosmos-focused PR also changes the shared link-verification matrix by adding the Azure C repository and removing Azure SDK for Rust's
Analyzecheck-run trigger, but the PR description does not mention any CI behavior change. That makes the Rust-specificcheck_runpath stop scheduling this job. Restore the existing Rust condition (and move the C synchronization to its intended workflow update) unless this CI change is deliberate and documented.
(github.repository == 'Azure/azure-sdk-for-c' && contains(github.event.check_run.name, 'GenerateReleaseArtifacts')) ||
(github.repository == 'Azure/azure-sdk-for-cpp' && contains(github.event.check_run.name, 'GenerateReleaseArtifacts')) ||
(github.repository == 'Azure/azure-sdk-for-go' && contains(github.event.check_run.name, 'Analyze')) ||
(github.repository == 'Azure/azure-sdk-for-ios' && contains(github.event.check_run.name, 'Analyze'))
.github/aw/actions-lock.json:36
- This silently downgrades the immutable setup action used by the generated agentic workflows from v0.84.3 to v0.83.4, although this Cosmos PR does not describe an agent-runtime rollback. Because
actions-lock.jsoncontrols the exact action code executed, this is not harmless generated churn and may roll back fixes across both workflows. Restore the existing pin (and regenerate the lock files with the current compiler), or move and explain the downgrade in a dedicated workflow change.
"github/gh-aw-actions/setup@v0.83.4": {
"repo": "github/gh-aw-actions/setup",
"version": "v0.83.4",
"sha": "e89c65e17eb281bbd5ff2ff9e9199a03e96654c7"
Resolve the driver changelog conflict by retaining both the CosmosOperation reference optimization and hedging execution-context breaking-change entries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 821eaa5c-6f91-4dcd-b744-c098fa1550c6
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 30 out of 33 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (2)
- .github/workflows/issue-triage.lock.yml
- .github/workflows/review-sdk.lock.yml
Suppressed comments (3)
sdk/cosmos/azure_data_cosmos/src/diagnostics/mod.rs:55
DiagnosticsContextis still a direct re-export of the driver type, so its inherentrequested_regions()method returnsVec<azure_data_cosmos_driver::diagnostics::RequestedRegion>, not either SDK-owned type exported here. Consumers only get these types by manually mapping.into(), and the documentedRequestedRegionReasonreturn surface therefore does not exist. Wrap/adapt the context API (or re-export the driver types) so the advertised SDK types are actually returned.
pub use region::{RequestedRegion, RequestedRegionReason};
sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs:80
- A future driver execution context is silently reported as
Initial, which gives consumers a false dispatch reason. Because the driver enum is#[non_exhaustive], add an explicitUnknown/OtherSDK reason (or preserve the opaque driver value) rather than mapping every new reason to an initial request; the spec's claim that this mapping has no wildcard must also be corrected.
// The driver enum is #[non_exhaustive]; map any future variants to
// the closest known reason rather than panicking.
_ => RequestedRegionReason::Initial,
sdk/cosmos/azure_data_cosmos_driver/src/models/resource_reference.rs:33
- This public resource-reference redesign (including removal of
DatabaseReference::into_account) is unrelated to hedging, is attributed to separate PR #4908 in the changelog, and contradicts this PR's statement that the execution-context rename is its only behavioral/API change. Drop the unrelated resource-reference commits from this PR or expand the stated scope and review it as an additional breaking change.
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (6)
sdk/cosmos/azure_data_cosmos/src/diagnostics/mod.rs:55
- These SDK-owned types are not the types returned by the exported
DiagnosticsContext. That context is still a direct driver re-export (lines 43–46), soDiagnosticsContext::requested_regions()returnsVec<azure_data_cosmos_driver::diagnostics::RequestedRegion>, and this SDKRequestedRegionis only reachable through a manualIntoconversion. The advertised SDK detection API therefore does not actually expose these projection types. Wrap/projectDiagnosticsContextat the SDK boundary, or otherwise make its public accessor return the SDK-owned type.
pub use region::{RequestedRegion, RequestedRegionReason};
sdk/cosmos/azure_data_cosmos_driver/src/models/resource_reference.rs:33
- This breaking resource-reference refactor is unrelated to the described hedging-detection/observability work and is explicitly attributed to PR #4908 in this crate's changelog. It removes
DatabaseReference::into_accountand changes all four public reference shapes, contradicting this PR's claim that the only behavioral change is theExecutionContextrename. Rebase or split the #4908 work so this PR contains only its stated scope.
pub struct DatabaseReference(Arc<DatabaseReferenceInner>);
/// Shared state behind [`DatabaseReference`].
#[derive(Debug, PartialEq, Eq, Hash)]
struct DatabaseReferenceInner {
sdk/cosmos/azure_data_cosmos_driver/src/driver/pipeline/operation_pipeline.rs:969
- This changes production log severity from
errortodebug, but the PR description says the only behavioral change is the execution-context rename. The same diff also changes hedge terminal log levels/messages, and the changelog attributes that work to PR #4711. Split or rebase those logging changes out of this hedging-detection PR.
tracing::debug!(
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/options.rs:85
- Hedge frequency affects sparsity, not metric cardinality. This counter inherits database name, container name, server address, status, and error attributes from
build_attributes, so it is not “near-zero cardinality” even before the optional hedge-region dimension. Rephrase this to describe only the bounded terminal-state value set so users do not underestimate telemetry-series cost.
/// The counter increments only for operations where hedging actually fanned
/// out, so it is near-zero cardinality; it always carries the low-cardinality
/// `hedge_terminal_state` dimension. The higher-cardinality hedge-region
/// dimension is added only when [`with_extended_attributes`](Self::with_extended_attributes)
/// is also enabled.
sdk/cosmos/azure_data_cosmos/src/diagnostics/region.rs:80
- A future driver execution-context variant is silently reported as
Initial, which is not the “closest” reason and can make retries/probes look like first dispatches. This also defeats the forward-compatibility purpose of both non-exhaustive enums. Add an explicitUnknown/OtherSDK reason (or use a fallible conversion) rather than fabricatingInitial.
// The driver enum is #[non_exhaustive]; map any future variants to
// the closest known reason rather than panicking.
_ => RequestedRegionReason::Initial,
sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.md:160
- This conflates dispatch history with response history. An in-flight loser was already dispatched and must remain in
requested_regions(); only adding it toresponded_regions()would invent a response. The current journal drops it from both, which contradicts §4.3's “complete dispatch history” contract.
An attempt that was still **in flight** when its leg was cancelled observed no
reply, so it is deliberately *not* recovered — reporting it would invent a
response that never arrived.
Stale review and I don't want to block merging on it.
Why
Re-implements the Cosmos Hedging Detection API (previously proposed in the now-closed #4558) on current
main— which carries the merged hedging implementation (#4432) and the client-side observability layer (#4789) — and surfaces hedging through that observability layer. Refs #4410.What
Driver detection API (
azure_data_cosmos_driver)DiagnosticsContextaccessors:requested_regions() -> Vec<RequestedRegion>(dispatch order, per-attemptRequestedRegionReason),responded_regions() -> Vec<&Region>(completion order, service-replies-only via a newRequestDiagnostics::responded_with_service_reply()predicate; non-2xx service replies still count), andhedging_started() -> bool.RequestedRegionstruct +RequestedRegionReasonenum (both#[non_exhaustive]) with a totalFrom<ExecutionContext>mapping.ExecutionContext::Retry->ExecutionContext::OperationRetry; the old variant is kept as a#[deprecated]alias for one release. Serialized string changes"retry"->"operation_retry"; all dispatch sites, tests, and JSON expectations updated.sdk/cosmos/azure_data_cosmos/docs/HEDGING_DETECTION_API_SPEC.mdto match main's reality. SDK re-exports the two new types.Observability surfacing (
azure_data_cosmos)distributed_tracingfeature): hedging span attributes on the operation root (hedging_started, hedge region, hedge terminal state, requested/responded regions as OTel string arrays) gated onhedging_started(); hedge-leg child span tagged.metricsfeature): opt-inazure.cosmosdb.client.operation.hedgedcounter viaMetricsOptions::with_hedged_metric; low-cardinalityhedge_terminal_statedimension always, high-cardinalityhedge_regiononly underwith_extended_attributes.hedging_started/hedge_region/hedge_terminal_statefields added to the sampled diagnostics log line when hedging occurred.Notes
ExecutionContextrename, softened by a one-release deprecated alias.DiagnosticsContext+ observability handlers — no foundation rebuild.Verification
cargo fmt --checkclean;cargo clippy(both crates, default +metrics,distributed_tracing+ the driver test feature)-D warningsclean.cargo test -p azure_data_cosmos --features metrics,distributed_tracing(incl. 8 new hedging tests) andcargo test -p azure_data_cosmos_driver— all pass. (--all-featuresskipped: environmentalopenssl-syson the dev box.)Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com