Add span backdating to core tracing traits#4784
Conversation
There was a problem hiding this comment.
Pull request overview
Adds explicit span timestamps to core tracing and OpenTelemetry, but also includes Cosmos retry changes that contradict the stated core-only scope.
Changes:
- Adds
start_span_at,start_span_with_parent_at, andend_at. - Implements and tests OpenTelemetry span backdating.
- Adds bounded Cosmos 410 retry and 503 conversion behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/core/typespec_client_core/src/tracing/mod.rs |
Adds timestamp-aware tracing APIs. |
sdk/core/typespec_client_core/CHANGELOG.md |
Documents tracing APIs. |
sdk/core/azure_core/CHANGELOG.md |
Documents re-exported APIs. |
sdk/core/azure_core_opentelemetry/src/tracer.rs |
Implements backdated span creation. |
sdk/core/azure_core_opentelemetry/src/span.rs |
Implements backdated ending and tests. |
sdk/core/azure_core_opentelemetry/CHANGELOG.md |
Documents OpenTelemetry support. |
sdk/cosmos/azure_data_cosmos/src/retry_policies/mod.rs |
Adds terminal 410-to-503 conversion. |
sdk/cosmos/azure_data_cosmos/src/retry_policies/client_retry_policy.rs |
Adds bounded Gone retries. |
sdk/cosmos/azure_data_cosmos/src/handler/retry_handler.rs |
Applies terminal conversion. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_fault_injection.rs |
Tests surfaced 503 status. |
sdk/cosmos/azure_data_cosmos/CHANGELOG.md |
Documents Cosmos retry behavior. |
161b0b3 to
7f9abef
Compare
heaths
left a comment
There was a problem hiding this comment.
@LarryOsterman what do you think? Seems acceptable, and .NET's StartActivity already has an overload that lets you do this.
Fundamentally I don't have a problem with this idea - we didn't have a need for it earlier, but if we do now, then so be it. My biggest concern is architectural: For better or for worse, we have standardized on the |
⚠ Blocked on an upstream workspace-dependency linkage (not a defect in this PR)This PR is clean and green on a consistent workspace (1 commit, 6 core-crate files, rebased directly onto
The Needs a maintainer call (cc the tracing-trait / workspace-versioning owner, e.g. Heath): the fix is an upstream workspace-versioning change ( |
|
You need to pull the latest upstream main and rebase, and then have |
|
@NaluTripician I'm working on fixing your PR. Turns out there's a few issues with just rebasing and updating the versions because of an unrelated crate. I'm prototyping a few ideas. You could make changes still and commit them, but don't push them yet. After my changes are pushed, you could pull this remote and update with git fetch origin nalutripician/cosmos-diag-ws4c-backdating
git rebase --onto FETCH_HEAD HEAD~ # assuming you made only one subsequent commit with suggested changesSee https://heaths.dev/tips/2019/11/11/rebasing-commits-on-one-topic-branch-onto-another-branch.html for more information. |
|
Opened #4791 to track long-term changes to workspace dependency resolution. |
Add explicit start/end timestamp support to the tracing traits so late-bound (tail-sampled) spans can be reconstructed from an already-completed operation with their original timestamps, rather than being stamped at "now". typespec_client_core (`tracing/mod.rs`): - `Tracer::start_span_at` and `Tracer::start_span_with_parent_at` - `Span::end_at` All three are additive with default implementations that delegate to the existing non-backdating methods, so existing `Tracer`/`Span` implementations keep compiling unchanged (non-breaking, minor-release friendly). `azure_core` re-exports these traits, so its public API gains the methods transitively. azure_core_opentelemetry: - Implement the new methods by mapping to raw OpenTelemetry `SpanBuilder::with_start_time` and `SpanRef::end_with_timestamp`. - Refactor span construction into shared `build_span` / `parent_context` helpers to avoid duplication across the four start_span variants. - Add in-memory-exporter unit tests proving a backdated span (and a backdated parent/child pair) carries the injected PAST timestamps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7f9abef to
6fd91d7
Compare
|
@NaluTripician this is ready. Please make additional requested changes. |
Address review feedback on the span-backdating traits: - Replace `Tracer::start_span_at` / `start_span_with_parent_at` with a future-proof `SpanOptions` struct passed to `start_span_with_options` / `start_span_with_parent_and_options` (analogrelay/heaths). - Use `time::OffsetDateTime` instead of `std::time::SystemTime` for the public timestamp type (LarryOsterman); the OpenTelemetry bridge converts to `SystemTime` at the OTel boundary. - `azure_core` CHANGELOG now copies the `typespec_client_core` entry verbatim with no re-export note (heaths). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@heaths @LarryOsterman @analogrelay — pushed the requested changes in 5288a57:
The stale "blocked on workspace-dependency linkage" warning has been removed from the description — the local Verification (core crates only): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
sdk/core/azure_core_opentelemetry/src/span.rs:319
- Repository test guidance says test function names should not begin with
testunless needed for disambiguation. The#[test]attribute already provides that context, so please rename this toopen_telemetry_span_backdated.
fn test_open_telemetry_span_backdated() {
sdk/core/azure_core_opentelemetry/src/span.rs:352
- Repository test guidance says test function names should not begin with
testunless needed for disambiguation. Please drop the redundant prefix here as well.
fn test_open_telemetry_span_backdated_with_parent() {
heaths
left a comment
There was a problem hiding this comment.
I'm good with this. Thanks! But I want @LarryOsterman to have one last look before merging.
Why
Tail-based (late-bound) sampling decides after an operation completes whether it's
worth emitting a span — e.g. only for slow or failed requests. To do that we must be able
to reconstruct a span for an operation that already happened, stamped with the
timestamps it actually had, not "now".
Today the tracing traits in
typespec_client_corehave no hook for this:Tracer::start_spanbuilds the span at "now" and
Span::end()closes it at "now". This PR adds an explicitstart/end timestamp path so backdated spans aren't a Cosmos-only concern.
This is the upstream sub-task (WS4c) of the Cosmos diagnostics effort. It is independent
of the Cosmos work and touches only core crates — no Cosmos crate is modified.
What
typespec_client_core(src/tracing/mod.rs):SpanOptionsstruct (currently carryingstart_time: Option<OffsetDateTime>).Tracer::start_span_with_options(name, kind, attributes, options)Tracer::start_span_with_parent_and_options(name, kind, attributes, parent, options)Span::end_at(end_time)azure_core:SpanOptionsalongside the existing tracing types.azure_core_opentelemetry:SpanBuilder::with_start_time/SpanRef::end_with_timestampAPIs, converting theOffsetDateTimetoSystemTimeat the OpenTelemetry boundary.build_span/parent_contexthelpers so thespan variants don't duplicate builder logic.
test_open_telemetry_span_backdated,test_open_telemetry_span_backdated_with_parent) that assert the exported span carriesthe injected past timestamps (and that a backdated child is correctly parented).
Trait-design rationale (additive, future-proof)
The change is deliberately additive: all new methods have default implementations
that delegate to the existing non-backdating methods (dropping the timestamp). Consequences:
Tracer/Spanimplementations keep compiling unchanged — adding a defaultedmethod to a trait is a SemVer-minor-compatible change in Rust.
than failing to compile or panicking. The OpenTelemetry bridge overrides the defaults to
honor the timestamps.
Timestamps are carried by a
SpanOptionsstruct rather than as bare method arguments, sofuture options can be added without changing the
Tracermethod signatures (per reviewfeedback from @analogrelay / @heaths).
time::OffsetDateTimeis used as the timestamp typeto match the rest of the Rust SDK, which standardizes on the
timecrate (per reviewfeedback from @LarryOsterman); the OpenTelemetry bridge converts it to
SystemTimewherethe OTel APIs require it.
Verification
Run for the touched core crates (
typespec_client_core,azure_core,azure_core_opentelemetry):cargo fmt— cleancargo clippy(lib/tests) — no warningscargo test—azure_core_opentelemetry18 lib tests (incl. the 2 new backdating tests)plus its doctests and integration tests;
typespec_client_core143 lib tests anddoctests (incl. the new
SpanOptionsdoctest)Scope guardrails
azure_data_cosmos/ driver untouched).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com