Populate Cosmos operation_name + active_instance metric - #4874
Populate Cosmos operation_name + active_instance metric#4874Nalu Tripician (NaluTripician) merged 14 commits into
Conversation
Cosmos observability enrichment #3. Driver: add CosmosOperation::db_operation_name mapping to canonical OTel db.operation.name values, thread it through DiagnosticsContextBuilder (new field + setter + hedge propagation) and set it in the operation pipeline (execute_operation_direct) so DiagnosticsContext::operation_name is populated in production instead of always None. PATCH aggregates are stamped patch_item via DiagnosticsContext::with_operation_name so they no longer inherit the trailing Replace sub-op's name. This makes the point-vs non-point tail-sampling classification and the db.operation.name span/log attribute correct. SDK metrics: implement the previously-deferred azure.cosmosdb.client.active_instance.count as an opt-in up-down counter (MetricsOptions::with_active_instance_metric); CosmosMetricsHandler records +1 on construction and -1 on Drop so the value tracks live instrumented client instances. Request-scope per-request metrics remain a documented TODO (high cardinality; deferred per task scope). 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
Populates Cosmos operation names for diagnostics and adds opt-in active-client metrics.
Changes:
- Maps driver operations to OpenTelemetry names.
- Propagates names through diagnostics, including PATCH aggregation.
- Adds active-instance metric configuration and lifecycle accounting.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
metrics/options.rs |
Adds the active-instance option. |
metrics/instruments.rs |
Defines the up-down counter. |
metrics/handler.rs |
Emits lifecycle count changes. |
metrics/attributes.rs |
Adds metric constants. |
azure_data_cosmos/CHANGELOG.md |
Updates metrics release notes. |
cosmos_operation.rs |
Maps operation names. |
patch_handler.rs |
Labels aggregated PATCH diagnostics. |
cosmos_driver.rs |
Populates operation names. |
diagnostics_context.rs |
Stores and propagates operation names. |
DIAGNOSTICS-CONTRACT.md |
Documents metric emission. |
azure_data_cosmos_driver/CHANGELOG.md |
Documents driver changes. |
…mapping - span_builder: prefer the caller-facing CosmosOperationContext operation name over the driver DiagnosticsContext (falling back to the driver value), so the tracing span label agrees with the db.operation.name metric and the tail-sampling classifier (both read the op context). Fixes a PATCH that fails during its internal Read being labeled read_item instead of patch_item. - cosmos_operation: map (Query|SqlQuery, Offer) => read_throughput so the user-facing throughput read (located via a query on the offers feed) is labeled, not just the poller's internal by-RID re-read. - Clarify active_instance docs across options/instruments/handler/attributes/ DIAGNOSTICS-CONTRACT: the counter tracks live CosmosMetricsHandler instances (one per client under the intended one-handler-per-client registration). - Note the intentional read_all_items/query_* naming divergence from .NET. - Fix CHANGELOG PR references to Azure#4874; add throughput + span-label tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:99
- This increments the “active client instance” metric when a handler is constructed, even if it is never registered with a client; conversely, one
Arc<CosmosMetricsHandler>can be registered with multiple independently built clients and contributes only 1. Because the public registration API accepts a shareableArc<dyn DiagnosticsHandler>, handler lifetime is not a reliable proxy for client lifetime, so this metric can over- or under-count clients. Tie the increment/decrement to client construction and the shared client-instance lifetime instead.
if handler.options.active_instance_metric_enabled() {
handler
.instruments
.active_instance
.add(1, &Self::active_instance_attributes());
sdk/cosmos/azure_data_cosmos/CHANGELOG.md:8
- The new
with_active_instance_metricpublic API is buried inside the existing #4789 handler entry. Give this distinct public API addition its own concise line so users can identify what #4874 adds without parsing or attributing the entire older feature to both PRs.
- Added the `metrics`-gated `CosmosMetricsHandler` (with `MetricsOptions`), emitting the stable `db.client.operation.duration` histogram plus per-signal opt-in metrics (`with_request_charge_metric`, `with_returned_rows_metric`, and `with_active_instance_metric` — an `azure.cosmosdb.client.active_instance.count` up-down counter that is incremented on handler construction and decremented on drop to track live client instances) and an opt-in extended attribute set (`with_extended_attributes`); a no-op when no meter provider is registered. ([#4789](https://github.com/Azure/azure-sdk-for-rust/pull/4789), [#4874](https://github.com/Azure/azure-sdk-for-rust/pull/4874))
The operation_name plumbing added in this PR grew the diagnostics context builder and driver operation state, tipping six emulator metadata/item query futures and two handler-propagation query futures just over clippy's `large_futures` deny threshold (16384 bytes) on the `--all-features --all-targets` CI Analyze job. Wrap the eight flagged `query_databases`/`query_containers`/ `query_items` futures in `Box::pin` (clippy's own suggestion), heap-allocating them so the awaited state is a pointer. Behavior is unchanged; only the two in_memory_emulator test files are touched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:99
- This increments at handler construction, not when a client is created. The public registration API accepts an
Arc<dyn DiagnosticsHandler>, so an enabled handler can be constructed but never registered, shared by multiple independently built clients, or registered more than once; all of those supported cases make this "active client instance" metric incorrect. Move the +1/-1 accounting to a per-built-client registration/lifetime guard (shared by that client's clones) so each instrumented client contributes exactly once.
if handler.options.active_instance_metric_enabled() {
handler
.instruments
.active_instance
.add(1, &Self::active_instance_attributes());
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2245
- This updated public documentation now says production contexts carry the operation name, but the same API's
is_threshold_violated_fordocumentation at lines 2311-2316 still says production contexts do not carry one and that SDK handlers must supply it. Update that stale section so rustdoc does not present contradictory behavior.
/// `create_item`, or `query_items`. The driver pipeline populates this from
/// [`CosmosOperation::db_operation_name`](crate::models::CosmosOperation::db_operation_name);
/// it is `None` only for operations without a canonical name (query plans,
/// partition-key-range reads, and other internal requests).
sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md:7
- This release note combines the new public method, pipeline behavior, tracing precedence, and PATCH aggregation into a very long implementation-level entry. The changelog guidance requires a concise one-line summary of each public API change; keep the customer-visible
db_operation_name/diagnostics behavior and omit internal tracing details.
- Added `CosmosOperation::db_operation_name`, returning the canonical OpenTelemetry `db.operation.name` (e.g. `read_item`, `query_items`, `execute_batch`) for an operation. The operation pipeline now populates `DiagnosticsContext::operation_name` from it in production (previously always `None`), so `DiagnosticsContext::operation_name()` is populated for callers inspecting diagnostics and supplies the operation name to the emission layer's tail-sampling classifier and tracing span when no SDK-supplied `CosmosOperationContext` is present. The tracing span's operation label prefers the caller-facing `CosmosOperationContext` identity (consistent with the `db.operation.name` metric), and PATCH aggregates report `patch_item` rather than the underlying Replace. ([#4874](https://github.com/Azure/azure-sdk-for-rust/pull/4874))
Resolves the automated review comments on Azure#4874. Operation naming (`cosmos_operation.rs`): a read feed scoped to a single logical partition now maps to the canonical `read_all_items_of_logical_partition` rather than `read_all_items`, and offer (throughput) operations are no longer mapped at all. Semconv has no unscoped `read_throughput`/`replace_throughput` — only the database- and container-scoped variants — and the driver's offer request carries no discriminator to tell them apart, so the scoped names are supplied by the SDK's operation contexts instead (`read_database_throughput`, `replace_database_throughput`, `read_container_throughput`, `replace_container_throughput`). PATCH identity (`patch_handler.rs`): every error exit now stamps `patch_item` on the diagnostics it propagates, so a failed read-modify-write is attributed to the caller-facing operation instead of the underlying Replace. Prior attempts are aggregated when more than one exists; the single-attempt case uses a new `DiagnosticsContext::clone_with_operation_name` so hedge diagnostics survive (`aggregate_sub_operations` drops them). Active-instance metric (`metrics/handler.rs`): the up-down counter was incremented in the handler constructor and decremented in its `Drop`, so it counted handler objects, not clients — one handler shared across N clients reported 1, and a handler built but never registered reported a phantom instance. The +1/-1 now ride a `ClientLifetimeToken` handed out by a new defaulted `DiagnosticsHandler::on_client_created` hook and owned by `ClientContext`, so the count follows live `CosmosClient` instances. Per semconv the counter is keyed on `server.address` (plus `server.port` for a non-default port), not `azure.client.id`; `DIAGNOSTICS-CONTRACT.md` D10 is corrected accordingly. Docs: rewrote the stale `is_threshold_violated_for` doc comment, and split the oversized changelog entries in both crates into focused single-purpose bullets, restoring the Azure#4789 entry the active-instance note had been folded into. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 611a0d9f-360b-4c07-b45f-7dba858f7a76
Resolves conflicts with Azure#4671 (Cosmos binary JSON encoding): - `cosmos_client_builder.rs` / `clients/mod.rs`: `ClientContext` gained a `binary_encoding` field upstream while this branch replaced the struct literal with `ClientContext::new` (which dispatches the new `on_client_created` hook); `new` now takes the resolved `BinaryEncodingOptions`. - `cosmos_operation.rs` and both CHANGELOGs: additive on both sides, kept together. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 611a0d9f-360b-4c07-b45f-7dba858f7a76
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:300
- Every registered
CosmosMetricsHandlerexecutes this+1becauseDiagnosticsHandlerChain::dispatch_client_createdvisits the entire chain. Since repeatedwith_diagnostics_handlercalls are explicitly supported (src/options/client.rs:44-47), one client with two metrics handlers backed by the same meter—or the same handler registered twice—reports2active clients. This contradicts the metric's client-instance semantics and the documented claim that the value is independent of handler count. Deduplicate this registration per logical client/instrument, or enforce a single metrics handler before recording.
counter.add(1, &attributes);
sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md:15
- This tracing-span behavior is implemented in
azure_data_cosmos/src/diagnostics/tracing/span_builder.rs, not in the driver crate. Listing it in the driver changelog misstates what driver-only users receive, while the SDK changelog omits the fix. Move this entry to the SDK changelog's existing Bugs Fixed section.
- The tracing span's operation label now prefers the caller-facing `CosmosOperationContext` identity, matching how the `db.operation.name` metric attribute is resolved. ([#4874](https://github.com/Azure/azure-sdk-for-rust/pull/4874))
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:420
- The embedded second
///makes this generated test documentation read as one malformed line. Split it into two doc-comment lines.
/// Returns the summed value of the `active_instance.count` up-down counter /// from the most recent export, or `None` if the metric was never emitted.
Resolves the additive conflict in the driver CHANGELOG: main added the `PlanOptions` fan-out entry (Azure#4855) and this branch added the `CosmosOperation::db_operation_name` entry (Azure#4874) at the same position under Features Added. Both are kept. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b5586b49-f116-489f-81a4-d51fb2511a71
Picks up the three review points that were raised but left open on Azure#4874. `DiagnosticsHandlerChain::dispatch_client_created` visited every chain entry, so registering the same `Arc<dyn DiagnosticsHandler>` twice (the chain is additive, so that is reachable) recorded two `+1`s for a single client and reported it as two live instances. Client creation is a per-client lifecycle event rather than a per-call dispatch, so the chain now notifies each distinct handler at most once per client, deduplicating by handler identity. Distinct handler objects are still notified independently. Three tests pin the repeated-handler, distinct-handler, and no-token cases. The `DIAGNOSTICS-CONTRACT.md` D7 table claimed the counter was "independent of how many handler objects exist", which overstated the guarantee: two distinct metrics handlers built from the same meter each record their own `+1`, exactly as they each record their own duration and request-charge samples. Both the contract and `MetricsOptions::with_active_instance_metric` now state what is actually guaranteed and point users at one metrics handler per meter. The tracing span operation-label precedence entry was filed under the driver's Bugs Fixed, but that behavior lives in `azure_data_cosmos/src/diagnostics/tracing/span_builder.rs`; listing it in the driver changelog misstates what driver-only consumers receive. Moved to the SDK changelog, with the symptom spelled out. Also splits a doc comment on the `active_instance_value` test helper that had two `///` prefixes collapsed onto one line. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b5586b49-f116-489f-81a4-d51fb2511a71
|
Rebased onto Merge conflictMerged Active-instance count with a repeated handler
Client creation is a per-client lifecycle event rather than a per-call dispatch, so the chain now notifies each distinct handler at most once per client, deduplicating by The related doc claim was also an overreach. Changelog placementThe tracing-span operation-label precedence entry was under the driver's Bugs Fixed, but that behavior lives in Doc commentSplit the Verification
|
Ashley Stanton-Nurse (analogrelay)
left a comment
There was a problem hiding this comment.
Approving, but let's consider a brief discussion offline about what db.operation.name should be for patch elements (read/replace)
Addresses review feedback: rather than pinning at each call site, box the planning future once inside `CosmosDriver::plan_operation`. `plan_operation` holds the whole pipeline-builder state across several await points, making it ~16.4 KB — over the 16384-byte `clippy::large_futures` threshold. The previous fix pinned at the five callers, which only covered the sites that happened to exist and left future callers to rediscover the problem. The body moves to a private `plan_operation_inner`; the public `plan_operation` boxes it, so every caller now awaits a pointer-sized future. This also removes the five call-site `Box::pin` wrappers and a pre-existing redundant one inside `execute_operation`, which would otherwise double-box. Verified against the CI toolchain (rust 1.95.0, 59807616e) with the workspace-wide clippy invocation CI runs: reverting only this change reproduces all five errors at 16432 bytes, and restoring it yields zero. Note that `cargo clippy -p <crate>` does not reproduce the lint, and neither does any Windows host.
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 (3)
sdk/cosmos/azure_data_cosmos_driver/src/driver/pipeline/patch_handler.rs:472
- For locally synthesized failures after a successful Read (missing ETag, body extraction/deserialization, patch evaluation, or serialization),
sourcescontains only successful sub-operation diagnostics. Reusing that context while changing only the name leaves its operation/terminal status successful, soDiagnosticsContext::is_failure()is false andeffective_status()reports 200. The tracing/logging handlers can therefore suppress the failed PATCH and metrics label it as a success. Stamperr.status()onto the replacement diagnostics as well aspatch_item, and add a local-error assertion for the diagnostics status/failure classification.
let stamped = match sources.as_slice() {
[] => return err,
[only] => Arc::new(only.clone_with_operation_name(operation_name)),
many => match DiagnosticsContext::aggregate_sub_operations(many) {
Some(ctx) => Arc::new(ctx.with_operation_name(operation_name)),
sdk/cosmos/azure_data_cosmos/src/diagnostics/handler.rs:172
- The new default hook is swallowed by the existing composable
SamplingLogHandler: that wrapper delegateshandleto its innerDiagnosticsHandlerbut does not override and forwardon_client_created. Consequently, wrapping an active-instance-enabledCosmosMetricsHandlercauses the counter to emit nothing even though the inner handler is registered through the wrapper. Forward this lifecycle hook (and its token) through handler decorators, with a wrapper test.
fn on_client_created(&self, client: &CosmosClientInfo) -> Option<ClientLifetimeToken> {
let _ = client;
None
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/attributes.rs:37
- This description contradicts the implementation: the instrument counts live
CosmosClientinstances, notCosmosMetricsHandlerinstances; one shared handler can count multiple clients. Describe the measured client lifetime instead.
/// Optional up-down counter (instances): number of live
/// [`CosmosMetricsHandler`](super::CosmosMetricsHandler) instances (one per
/// instrumented client, under the intended one-handler-per-client registration).
A PATCH is one user-facing operation implemented as a read followed by a replace. We previously relabeled both halves `patch_item`, which matches the caller's mental model but erases the fact that two round trips happened. Bare `read_item` / `replace_item` would have been worse: they look like standalone point operations the caller never issued. Encode both facts instead. The operation span and the operation metric keep reporting `patch_item`; the two attempts now report `patch_read_item` and `patch_replace_item`, so the read-modify-write decomposition is visible without being mistakable for a caller request. Per-request operation identity did not exist before, so this needed a change at each layer the name has to survive: - `cosmos_operation.rs` — an `is_patch_sub_operation` marker that `db_operation_name` consults for the Read and Replace arms. - `patch_handler.rs` — the sub-operation builders set the marker; the aggregate keeps `patch_item`. - `diagnostics_context.rs` — `RequestDiagnostics` carries an optional `operation_name`. `aggregate_sub_operations` stamps it as it folds the sub-contexts together, and `preserve_request_operation_names` pushes a displaced name down when a context is relabeled, so the single sub-operation error path keeps its identity too. `None` means "same as the context", so non-PATCH diagnostics JSON is byte-identical. - `compaction.rs` — `CompactionKey` includes the operation name. Otherwise a PATCH's read and replace hitting the same endpoint with the same status could collapse into one run whose RU total and duration percentiles blend a read with a write. - `span_builder.rs` — attempt spans prefer the request's own name and fall back to the operation's. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b490ca0-32f6-4909-80c7-e7f77b661c4a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/attributes.rs:37
- This comment still describes handler instances, but the implementation now deliberately counts logical clients and supports one handler shared by multiple clients. Document the client-based series here so this constant does not contradict the public option and handler docs.
/// Optional up-down counter (instances): number of live
/// [`CosmosMetricsHandler`](super::CosmosMetricsHandler) instances (one per
/// instrumented client, under the intended one-handler-per-client registration).
sdk/cosmos/azure_data_cosmos/src/diagnostics/handler.rs:173
- The new default hook is swallowed by the built-in
SamplingLogHandler, even though that wrapper accepts anyArc<dyn DiagnosticsHandler>and delegateshandleto it. Wrapping a lifecycle-aware handler (includingCosmosMetricsHandler) therefore prevents itson_client_createdcallback and the active-instance metric never increments. Add an override onSamplingLogHandlerthat forwards this lifecycle callback to its inner handler.
fn on_client_created(&self, client: &CosmosClientInfo) -> Option<ClientLifetimeToken> {
let _ = client;
None
}
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2076
aggregate_sub_operationsis also used for retries of the same operation byCosmosResponse::with_aggregated_prior_diagnostics(src/models/cosmos_response.rs:197-207). This unconditional assignment therefore populates every request with a redundant name even when all sources are the same operation, contradicting the new accessor contract that the field is set only for multi-operation aggregates and changing ordinary retry diagnostics JSON. Only push source names down when the source contexts have distinct operation names; already-named nested requests can still be preserved.
c.requests.iter().map(|req| {
let mut req = req.clone();
if req.operation_name.is_none() {
req.operation_name = c.operation_name.clone();
}
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/handler.rs:108
- The active-instance semantic convention and the updated diagnostics contract define this metric's attributes as
server.addressplus conditionalserver.port;db.system.nameis currently explicitly deferred in the upstream convention. Emitting it here makes the standard metric use a different attribute schema than documented. Remove this attribute (and its test assertion) until the convention adds it.
let mut attrs = Vec::with_capacity(3);
attrs.push(KeyValue::new(
attributes::ATTR_DB_SYSTEM_NAME,
attributes::DB_SYSTEM_NAME_VALUE,
));
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/compaction.rs:149
- The operation name now separates compaction buckets, but
compacted_rundoes not copy that name into the publicCompactedRunrollup. Consequently PATCH read and replace runs with identical endpoint/status/context remain indistinguishable in serialized diagnostics, so consumers cannot tell which RU and latency statistics belong to which sub-operation—the decomposition this key is intended to preserve. Add an optional operation name toCompactedRunand populate it from the run's first request.
operation_name: req.operation_name().map(str::to_string),
sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs:2813
- Populating the driver name here makes existing public documentation inaccurate:
azure_data_cosmos/src/diagnostics/operation_context.rs:23-26still saysDiagnosticsContextdoes not capture the operation name, anddiagnostics/logging/handler.rs:239-241says production contexts do not carry one. Update those descriptions to explain thatCosmosOperationContextsupplies the caller-facing override/fallback, consistent with the new behavior.
// Populate the canonical `db.operation.name` (e.g. `read_item`,
// `query_items`) so the finalized diagnostics carry it in production —
// feeding the emission layer's span/log attribute and the point-vs.-
// non-point tail-sampling classification. `None` for operations without
// a canonical name leaves it unset, matching prior behavior.
if let Some(operation_name) = operation.db_operation_name() {
diagnostics_builder.set_operation_name(operation_name);
Resolve the azure_data_cosmos_driver CHANGELOG conflict in the 0.7.0 (Unreleased) section by unioning both sides: main's 403 topology-retry budget fix (Azure#4740) alongside this PR's operation_name entries (Azure#4874). All other files auto-merged cleanly (no additional hand resolution needed). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac9ab8a2-54e7-464e-b6f4-078f60dac9cf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (6)
sdk/cosmos/azure_data_cosmos_driver/src/driver/pipeline/patch_handler.rs:476
- For locally synthesized failures after a successful Read (missing ETag, body extraction/deserialization, patch evaluation, or serialization),
err.diagnostics()isNone, sosourcescontains only successful sub-operation contexts. This attaches diagnostics whose operation name ispatch_itembut whose status remains 200. The SDK then reports a successful status metric andDiagnosticsContext::is_failure()returns false, suppressing failure-triggered tracing/logging for an operation that returned an error. Stamp the aggregate's operation status fromerr.status()before attaching it, and cover one of these local-error paths with anis_failure()/effective-status assertion.
let stamped = match sources.as_slice() {
[] => return err,
[only] => Arc::new(only.clone_with_operation_name(operation_name)),
many => match DiagnosticsContext::aggregate_sub_operations(many) {
Some(ctx) => Arc::new(ctx.with_operation_name(operation_name)),
sdk/cosmos/azure_data_cosmos_driver/src/models/cosmos_operation.rs:241
FeedRange::is_logical_partition()also returns true for a partial hierarchical-partition-key prefix, but such a prefix covers multiple logical partitions (feed_range.rs:115-153;azure_data_cosmos/src/feed/query.rs:25-31). This therefore labels a cross-partition prefix read asread_all_items_of_logical_partition. Check that the partition key has all paths from the container's partition-key definition before selecting the singular logical-partition name.
} else if self.targets_logical_partition() {
// `read_all_items(container, partition_key)` narrows the
// feed to one logical partition, which semconv names
// distinctly from the cross-partition read.
"read_all_items_of_logical_partition"
sdk/cosmos/azure_data_cosmos/src/diagnostics/metrics/attributes.rs:37
- This documentation still says the counter counts
CosmosMetricsHandlerinstances, but the new implementation deliberately counts liveCosmosClientregistrations and allows one handler to serve multiple clients. Update the description so generated docs do not reintroduce the handler-lifetime model this PR replaces.
/// Optional up-down counter (instances): number of live
/// [`CosmosMetricsHandler`](super::CosmosMetricsHandler) instances (one per
/// instrumented client, under the intended one-handler-per-client registration).
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/compaction.rs:139
- Adding
operation_nameonly to the private grouping key separates PATCH Read/Replace buckets internally, butCompactedRundoes not expose that discriminator. In the exact case described here, the two public rollup rows can have identical region/endpoint/status/execution-context fields, so consumers cannot tell which RU and latency statistics belong to the Read versus Replace after compaction. Carry the operation name intoCompactedRun(and update its documented grouping fields) so the decomposition remains observable.
operation_name: Option<String>,
sdk/cosmos/azure_data_cosmos_driver/src/models/cosmos_operation.rs:213
patch_read_itemandpatch_replace_itemare not canonical Cosmos DBdb.operation.namevalues. The OpenTelemetry Cosmos DB convention says a listed well-known value MUST be used when applicable, and listsread_itemandreplace_item; thepatch_*variants are not listed. These child requests are actual Read/Replace operations, while their parent already identifies the caller operation aspatch_item. Keep the standard names here and use a separate custom attribute if additional PATCH ownership is needed.
(OperationType::Read, ResourceType::Document) if self.is_patch_sub_operation => {
"patch_read_item"
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2079
- This populates a request-level name for every aggregation, including the dataflow retry path in
CosmosResponse::with_aggregated_prior_diagnostics, where all source contexts and the aggregate have the same operation name. That contradicts the new public contract thatRequestDiagnostics::operation_name()isNoneunless requests from different operations are combined, and unnecessarily changes serialized diagnostics for ordinary retries. Only copy the source name when it differs from the aggregate's inherited name; the later PATCH restamp will still preserve both sub-operation identities.
c.requests.iter().map(|req| {
let mut req = req.clone();
if req.operation_name.is_none() {
req.operation_name = c.operation_name.clone();
}
Keep the tracing assertion aligned with the actual read-modify-write execution sequence instead of sorting away ordering regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5440044f-ae66-4eee-8f44-f06bf90a8e8b
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
sdk/cosmos/azure_data_cosmos_driver/src/models/cosmos_operation.rs:217
patch_read_itemandpatch_replace_itemare not Cosmos DB semantic-convention operation names. The current convention listspatch_item,read_item, andreplace_item, and requires the respective well-known value whenever one applies. These internal requests are still a Read and Replace, so custom names makedb.operation.namenon-canonical and exclude them from tooling that groups the standard operations. Keep the root aggregate aspatch_item, but label these attemptsread_item/replace_item; their parent span already preserves that they belong to PATCH.
(OperationType::Read, ResourceType::Document) if self.is_patch_sub_operation => {
"patch_read_item"
}
(OperationType::Read, ResourceType::Document) => "read_item",
(OperationType::Replace, ResourceType::Document) if self.is_patch_sub_operation => {
"patch_replace_item"
sdk/cosmos/azure_data_cosmos/src/diagnostics/handler.rs:46
Url::port()drops a scheme-default port, so a supported HTTP emulator endpoint such ashttp://localhost:80/producesNonehere. Cosmos semconv treats 443 as the DBMS default, making port 80 conditionally required; this therefore merges emulator series that should carryserver.port=80. Resolve the effective port and omit only Cosmos DB's default 443.
server_port: endpoint.port(),
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/diagnostics_context.rs:2076
- This unconditionally copies each source context's name onto every request, but
aggregate_sub_operationsis also used for same-operation topology retries (models/cosmos_response.rs:197-207). Those non-PATCH contexts now redundantly serializeoperation_nameon every retained attempt, contradicting the new accessor docs that say the field is set only when multiple operations are aggregated. Only preserve a source name when it differs from the aggregate's inherited name;with_operation_namewill still fill the final PATCH sub-operation when the aggregate is restamped.
c.requests.iter().map(|req| {
let mut req = req.clone();
if req.operation_name.is_none() {
req.operation_name = c.operation_name.clone();
}
sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/compaction.rs:149
- Adding
operation_nameonly to the grouping key separates PATCH Read and Replace buckets internally, butCompactedRundoes not carry or serialize that discriminator. When both sub-operations share region, endpoint, status, and execution context, diagnostics expose two otherwise indistinguishable rollup rows, so operators cannot tell which RU/duration statistics belong to the Read versus Replace. Add the issuing operation name toCompactedRunand populate it from the first request.
endpoint: req.endpoint().to_string(),
status: *req.status(),
execution_context: req.execution_context(),
operation_name: req.operation_name().map(str::to_string),
sdk/cosmos/azure_data_cosmos_driver/src/models/cosmos_operation.rs:482
- This public getter cannot return
truefor external callers: the only setter,as_patch_sub_operation, is crate-private, all public constructors initialize the flag to false, and operations are consumed by execution. It therefore adds an unusable public API (and changelog commitment) solely for crate-internal tests. Make the getterpub(crate)and remove it from the public changelog entry.
pub fn is_patch_sub_operation(&self) -> bool {
self.is_patch_sub_operation
Record completion of the reviewer nit follow-up after verifying the existing behavioral commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 49ca1a2f-4e01-44a7-a1a5-959602b42935
Resolve conflicts with the client-lifetime metrics and PATCH operation naming work from Azure#4874 by keeping both sides: - Metrics attributes/instruments/options now carry both the hedged operation counter and the active-instance up-down counter. - `RequestDiagnostics` keeps both internal test helpers (`with_execution_context_for_testing` and `for_testing_with_operation_name`). - `DiagnosticsContext::clone_with_operation_name` (new in main) now carries the hedging detection fields forward. - Changelogs keep both sets of entries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2af76390-9049-48f0-89f2-51f754db60ba
Resolves two conflicts created by Azure#4789 and Azure#4874 landing on main: - `CHANGELOG.md` - main now carries the `CosmosMetricsHandler` bullet this branch had been adding, so the resolution keeps main's wording plus this branch's sentence recording that each histogram declares explicit bucket boundaries, and keeps both bullets Azure#4874 added. - `Cargo.lock` - regenerated rather than hand-merged; `--locked` passes. `instruments.rs` auto-merged. Checked by hand that all three histograms still carry `.with_boundaries(...)` afterwards, since losing one would silently restore the millisecond-scaled defaults and make every latency percentile a constant again. The instrument Azure#4874 adds is an up-down counter, so it needs none. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7c387a4e-befb-4bcf-a800-bed57fec3b5c
Resolve the driver changelog conflict by keeping both this branch's metadata-hedging entries and the operation-naming entries from Azure#4874. The source changes from main merged cleanly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2af76390-9049-48f0-89f2-51f754db60ba
Why
Two enrichment gaps in the merged Cosmos observability layer (#4789):
DiagnosticsContext::operation_namewas never populated in production (so tail-sampling's point-vs-non-point classification and thedb.operation.namespan label were degraded), and theazure.cosmosdb.client.active_instance.countmetric was documented but not emitted.What
operation_nameplumbing (azure_data_cosmos_driver)CosmosOperation::db_operation_name()maps (operation type, resource type, feed scope) to the canonical OTeldb.operation.namevalues —read_item/create_item/replace_item/upsert_item/delete_item/patch_item/query_items/query_change_feed/execute_batch/ container + database ops, andread_all_itemsvsread_all_items_of_logical_partitiondepending on whether the read feed'sFeedRangeis scoped to one logical partition. Unmapped internal ops (query plans, pk-range reads, HEAD, sprocs, DTX) stayNone.read_throughput; it distinguishesread_database_throughputfromread_container_throughput, and an offer request carries no database-vs-container discriminator, so the driver cannot pick correctly. The scoped names are supplied by the SDK'sDatabaseClient/ContainerClientthroughput operations, which do know the scope.DiagnosticsContextBuildergains anoperation_namefield + setter, propagated throughclone_for_hedge_attemptandcomplete()(previously hardcodedNone), set at the single operation-scope chokepoint inexecute_operation_direct.patch_item.active_instance.count(azure_data_cosmos, opt-in)MetricsOptions::with_active_instance_metric, backed by an i64 up-down counter keyed on the account endpoint (server.address, plusserver.portonly for a non-default port) — the semconv attribute set for this instrument.DiagnosticsHandlergains a defaultedon_client_created(&CosmosClientInfo) -> Option<ClientLifetimeToken>hook, dispatched once per client byClientContext::new.CosmosMetricsHandlerrecords the+1there and returns a token whoseDroprecords the-1.ClientContextowns the token and is cloned down into everyDatabaseClient/ContainerClient, so the-1lands when the last client derived from thatCosmosClientgoes away. A handler shared across N clients therefore reports N, and one registered on no client reports nothing.None, so existing handlers are unaffected and it costs nothing when unused.Tracing span label
The span's operation label now prefers the caller-facing
CosmosOperationContextidentity over the driver-recorded name, matching how thedb.operation.namemetric attribute is resolved. Previously a PATCH that failed during its internal read could label the spanread_itemwhile the metric reportedpatch_item.Notes
active_instanceis opt-in.DIAGNOSTICS-CONTRACT.mdis corrected in two places: the D10 note claimed this counter is keyed onazure.client.id, which is a span attribute and would make every series a constant 1; and §10.3.1 now records the throughput-scope split above.azure_core-backdating-trait migration is blocked on core1.2.0being published (shipping crates link published core1.1.0); request-scope (per gateway/replica) metrics remain a documented high-cardinality follow-up.Verification
cargo fmt --checkclean;cargo clippy --all-targetson both crates (SDK withmetrics,distributed_tracing,control_plane,key_auth; driver withfault_injection) — 0 warnings.cargo test -p azure_data_cosmos_driver --lib(2362 pass, incl. thedb_operation_namemapping and PATCH error-path label tests);cargo test -p azure_data_cosmos --lib --doc(179 + 50 pass, incl. the active-instance lifecycle, endpoint-keying, and handler-chain dedup tests).--all-featuresskipped (environmentalopenssl-sys).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com