feat(ARSN-572): Propagate W3C trace context to MongoDB metadata writes#2611
feat(ARSN-572): Propagate W3C trace context to MongoDB metadata writes#2611delthas wants to merge 1 commit intodevelopment/8.3from
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
LGTM |
delthas
force-pushed
the
improvement/ARSN-572/trace-context
branch
from
d42f30b to
01693cd
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (tc && tc.traceparent) { | ||
| this._data.traceContext = tc; | ||
| } |
There was a problem hiding this comment.
When this method is called on an ObjectMD constructed from an existing document (as in putObjectNoVerWithOplogUpdate and internalDeleteObject), passing undefined leaves any stale traceContext from the prior write intact. A delete with no active span would carry forward the PUT's trace context to the oplog entry, misleading downstream consumers.
| if (tc && tc.traceparent) { | |
| this._data.traceContext = tc; | |
| } | |
| if (tc && tc.traceparent) { | |
| this._data.traceContext = tc; | |
| } else { | |
| delete this._data.traceContext; | |
| } |
— Claude Code
| const tc = captureCurrentTraceContext(); | ||
| if (tc) { | ||
| objVal.traceContext = tc; | ||
| } |
There was a problem hiding this comment.
Same stale carry-forward issue: if captureCurrentTraceContext() returns undefined, any existing objVal.traceContext from the prior metadata persists into the oplog entry.
| const tc = captureCurrentTraceContext(); | |
| if (tc) { | |
| objVal.traceContext = tc; | |
| } | |
| const tc = captureCurrentTraceContext(); | |
| if (tc) { | |
| objVal.traceContext = tc; | |
| } else { | |
| delete objVal.traceContext; | |
| } |
— Claude Code
Review by Claude Code |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.3 #2611 +/- ##
===================================================
- Coverage 73.47% 73.46% -0.01%
===================================================
Files 222 223 +1
Lines 18175 18210 +35
Branches 3785 3770 -15
===================================================
+ Hits 13354 13378 +24
- Misses 4816 4827 +11
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
delthas
force-pushed
the
improvement/ARSN-572/trace-context
branch
from
01693cd to
ca456c9
Compare
| out.tracestate = carrier.tracestate; | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
No unit tests for this function. It is the core new logic (OTEL API interaction) and has zero test coverage — the ObjectMD tests only cover the model setter/getter, not the capture logic itself.
Key behaviors to cover:
- Returns undefined when no span is active (OTEL not enabled)
- Returns correct traceparent/tracestate when a span exists
- Returns undefined when propagation.inject yields no traceparent
These are straightforward to test by mocking the @opentelemetry/api exports (context, trace, propagation).
— Claude Code
Review by Claude Code |
delthas
force-pushed
the
improvement/ARSN-572/trace-context
branch
from
ca456c9 to
a30bce8
Compare
| out.tracestate = carrier.tracestate; | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
captureCurrentTraceContext has no unit tests. While the ObjectMD setter/getter is well covered, this function is the core integration point with the OTEL API. A test that registers a mock propagator and verifies the output (and the undefined path when no span is active) would catch regressions if the @opentelemetry/api contract changes.
— Claude Code
|
LGTM — clean design. One minor gap: |
Add a traceContext field to ObjectMD carrying the currently-active OTEL trace context (W3C traceparent/tracestate). Inject it automatically at the three metadata write chokepoints where originOp is set today: internalPutObject, repair, and internalDeleteObject. The value ends up in the MongoDB oplog; downstream consumers (backbeat, sorbet) can extract it to continue the trace across the async boundary, closing the loop on end-to-end tracing for flows that cross the oplog. When OTEL is not active on the caller (no SDK, or request outside a traced context), captureCurrentTraceContext returns undefined, setTraceContext no-ops, and the field stays absent — zero cost. Only adds @opentelemetry/api as a runtime dependency (the API-only surface package, becomes a no-op when no SDK is registered). Issue: ARSN-572
delthas
force-pushed
the
improvement/ARSN-572/trace-context
branch
from
a30bce8 to
39233c8
Compare
| out.tracestate = carrier.tracestate; | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
No unit tests for this function. It contains non-trivial logic (active-span check, propagation injection, traceparent/tracestate filtering) that should be exercised independently of the ObjectMD model tests. Mocking @opentelemetry/api (or using its built-in no-op behavior) would let you cover:
- No active span → returns undefined
- Active span with traceparent → returns { traceparent }
- Active span with traceparent + tracestate → returns both
- Active span but propagation yields no traceparent → returns undefined
— Claude Code
|
2 participants
Not human-reviewed yet. Not asking for reviews at the moment!
Context
Cloudserver recently gained OpenTelemetry tracing (CLDSRV-884, cloudserver PR scality/cloudserver#6140). That gives us a single end-to-end trace per S3 request spanning cloudserver → vault → hdproxy → hyperiod.
But many state mutations in the S3 stack are written to MongoDB and consumed asynchronously by worker services via the oplog:
Today the oplog entry carries no trace context, so async work performed hours or days after the S3 request cannot be tied back to the request that caused it. A user's
POST …?restoreproduces a trace that ends at the metadata write; the actual cold-tier retrieval by Sorbet is invisible from the original trace.Solution
Stamp a W3C trace context (
traceparent/tracestate) on every mutating metadata write, auto-derived from the currently-active OTEL context.The field lives on
ObjectMDDatanext tooriginOp:A new helper
captureCurrentTraceContext()reads the active OTEL context via@opentelemetry/apiand serializes it viapropagation.inject(). It's invoked at the threeMongoClientInterfacesites that currently setoriginOp:internalPutObjectrepairinternalDeleteObjectConsumers (backbeat, sorbet — out of scope for this PR) will later:
value.traceContext.traceparentfrom the oplog entrypropagation.extract()it to reconstruct a parent contextcontext.with(extractedCtx, ...)This closes the loop on end-to-end tracing across the async boundary.
Design decisions
1. Auto-inject at the arsenal write boundary (vs. param plumbing).
The alternative was to thread a new
traceContextparameter through the ~15 cloudserver call sites that already setoriginOp. Rejected because:@opentelemetry/context-async-hooks(loaded by the OTEL SDK at cloudserver startup) already propagates the request context through arsenal's async call chain for free. Re-reading it at the mongo write is idiomatic OTEL.originOpout of params and attaches it to theObjectMD.originOpeasy to forget on new API endpoints today.2. Only
@opentelemetry/apias a runtime dep (~10 KB, no SDK).The api package is specifically designed to be safe for libraries to depend on. When no SDK is registered (today's state for anyone not opting into cloudserver OTEL),
context.active()returns a no-op context,trace.getSpan()returns undefined, andcaptureCurrentTraceContext()returnsundefined.setTraceContext(undefined)no-ops; the field stays absent in the write. Zero runtime cost when OTEL is off.3. Optional field, no schema migration.
traceContextis absent by default — consistent with other optional ObjectMD fields likelegalHold,retentionDate. No default-value change in the ObjectMDData construction. Existing consumers that don't understand the field simply ignore it; fully backward compatible.4. No-op when no traceparent is present.
setTraceContext(undefined)andsetTraceContext({ tracestate: 'x' })(no traceparent) both no-op — the field only appears when the caller had an active trace. This prevents junk entries from any edge cases in the OTEL API.Interaction with async flows (cold restore as example)
Cold storage restore exercises both a sync and an async metadata write:
ObjectRestore:Post(sync, user-initiated viaPOST /bucket/key?restore) — cloudserver setsoriginOp; arsenal now also stampstraceContextpointing to the user's request trace. ✅ObjectRestore:Completed(async, Sorbet-triggered via cloudserver's/_/backbeat/*route) — behavior depends on Sorbet:traceparenton its HTTP call (ideally extracted from step 1's storedtraceContext): arsenal picks it up viacontext.with(remoteContext, ...)in cloudserver'sserver.js, and the Completed write ties back to the original user restore. ✅traceparent(today's state): safely no-ops — Completed write has notraceContext, no regression. ✅Same pattern applies to lifecycle expiration/transition, CRR replication, and GC.
Verification
Unit tests
tests/unit/models/ObjectMD.spec.js— 4 new tests covering:set/getTraceContextsetTraceContext(undefined)is a no-opsetTraceContext({ tracestate: ... })without traceparent is a no-opgetValue()serializestraceContextonly when setAll 128 ObjectMD tests pass (was 124 + 4 new).
End-to-end (follow-up on a test cluster)
ENABLE_OTEL=trueo.value.traceContext.traceparentshould be present, with characters 3-35 matching the Jaeger trace IDENABLE_OTEL=false→ field should be absentOut of scope (follow-ups)
value.traceContext.traceparentfrom oplog entries, extract & wrap processingtraceparenton its callbacks to cloudserver's backbeat routeoriginOp. Parallel but separate change if we want trace context on bucket writesSize
@opentelemetry/api(~10 KB, API-only)Issue: ARSN-572