-
Notifications
You must be signed in to change notification settings - Fork 20
feat(ARSN-572): Propagate W3C trace context to MongoDB metadata writes #2611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development/8.3
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| import * as crypto from 'crypto'; | ||
| import * as constants from '../constants'; | ||
| import { | ||
| applyTraceContext, | ||
| TraceContextCarrier, | ||
| } from '../storage/metadata/captureTraceContext'; | ||
| import * as VersionIDUtils from '../versioning/VersionID'; | ||
| import { VersioningConstants } from '../versioning/constants'; | ||
| import ObjectMDLocation, { | ||
|
|
@@ -89,6 +93,10 @@ export type ObjectMDData = { | |
| replicationInfo: ReplicationInfo; | ||
| dataStoreName: string; | ||
| originOp: string; | ||
| traceContext?: { | ||
| traceparent?: string; | ||
| tracestate?: string; | ||
| }; | ||
| microVersionId?: string; | ||
| // Deletion flag | ||
| // Used for keeping object metadata in the oplog event | ||
|
|
@@ -1442,6 +1450,29 @@ export default class ObjectMD { | |
| return this._data.originOp; | ||
| } | ||
|
|
||
| /** | ||
| * Attach a W3C trace context to the metadata so it ends up in | ||
| * the MongoDB oplog and downstream consumers can continue the trace. | ||
| * Always reflects the current write: passing undefined (or a value | ||
| * without traceparent) clears any previously-set traceContext so that | ||
| * a stale context does not get carried forward from an existing | ||
| * ObjectMD loaded from storage. | ||
| * @param tc - W3C trace context carrier | ||
| * @return itself | ||
| */ | ||
| setTraceContext(tc?: TraceContextCarrier) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this is correct.
In addition (and maybe this becomes irrelevant with the above point, or maybe not): I know ObjectMD is mostly written with simple accessors, but this is causing issues. Would be better to allow passing the trace context at the proper point in the lifecycle of the object, and introduce the parameter at that specific point. For exemple, if trace should be passed whenever we make a new MD op, this should be a second parameter of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, looking below it is applied in mongo client : so correct indeed, but then the problem is more related to abstraction. Setting the trace context SHOULD NOT be done by "users" of ObjectMD (cloudserver and backbeat mostly), but only by metadata backend also it will not be present for other backends -esp. metadata- : should we consider handling this in |
||
| applyTraceContext(this._data, tc); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the trace context attached to the metadata, if any. | ||
| * @return W3C trace context carrier or undefined | ||
| */ | ||
| getTraceContext() { | ||
| return this._data.traceContext; | ||
| } | ||
|
|
||
| /** | ||
| * Returns metadata object | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { context, propagation, trace } from '@opentelemetry/api'; | ||
|
|
||
| export interface TraceContextCarrier { | ||
| traceparent?: string; | ||
| tracestate?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Set or clear the `traceContext` field on a metadata-shaped object. | ||
| * Used by every site that stamps trace context onto a write — both raw | ||
| * ObjectMDData manipulation and the ObjectMD model setter — so the | ||
| * set/clear semantics live in exactly one place. Always reflects the | ||
| * current write: passing undefined (or a value without `traceparent`) | ||
| * clears any previously-set context so a stale value is not carried | ||
| * forward from a loaded ObjectMD. | ||
| * | ||
| * Generic over `data` so callers can use either an ObjectMD's `_data` | ||
| * or a raw `ObjectMDData` without type gymnastics. | ||
| */ | ||
| export function applyTraceContext( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function is always called with |
||
| data: { traceContext?: TraceContextCarrier }, | ||
| tc?: TraceContextCarrier, | ||
| ): void { | ||
| if (tc && tc.traceparent) { | ||
| data.traceContext = tc; | ||
| } else { | ||
| delete data.traceContext; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Capture the currently-active OTEL trace context as a plain object | ||
| * suitable for storing alongside metadata (ends up in the MongoDB oplog). | ||
| * Returns undefined when no trace is active (e.g., OTEL not enabled, | ||
| * or called outside a traced request). | ||
| */ | ||
| export function captureCurrentTraceContext(): | ||
| { traceparent: string; tracestate?: string } | undefined { | ||
| const ctx = context.active(); | ||
| const span = trace.getSpan(ctx); | ||
| if (!span) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const carrier: Record<string, string> = {}; | ||
| propagation.inject(ctx, carrier, { | ||
| set: (c, k, v) => { c[k] = v; }, | ||
| }); | ||
| if (!carrier.traceparent) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const out: { traceparent: string; tracestate?: string } = { | ||
| traceparent: carrier.traceparent, | ||
| }; | ||
| if (carrier.tracestate) { | ||
| out.tracestate = carrier.tracestate; | ||
| } | ||
| return out; | ||
| } | ||
|
delthas marked this conversation as resolved.
delthas marked this conversation as resolved.
delthas marked this conversation as resolved.
|
||
|
delthas marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import { ErrorLike, reshapeExceptionError } from '../../../errorUtils'; | |
| import errors, { ArsenalError, errorInstances } from '../../../errors'; | ||
| import BucketInfo, { BucketMetadata, Capabilities } from '../../../models/BucketInfo'; | ||
| import ObjectMD, { ObjectMDData } from '../../../models/ObjectMD'; | ||
| import { applyTraceContext, captureCurrentTraceContext } from '../captureTraceContext'; | ||
| import * as jsutil from '../../../jsutil'; | ||
| import { ArsenalCallback, NestedOmit } from '../../../types'; | ||
|
|
||
|
|
@@ -1284,6 +1285,7 @@ class MongoClientInterface { | |
| const obj = doc.value; | ||
| const objMetadata = new ObjectMD(obj); | ||
| objMetadata.setOriginOp(params.originOp); | ||
| objMetadata.setTraceContext(captureCurrentTraceContext()); | ||
| objMetadata.setDeleted(true); | ||
| return next(null, objMetadata.getValue()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
not related to this PR, let's not fix it yet ; but please create a ticket to look at this |
||
| }).catch(err => { | ||
|
|
@@ -1361,6 +1363,7 @@ class MongoClientInterface { | |
| cb: ArsenalCallback<string | void>, | ||
| ): void { | ||
| MongoUtils.serialize(objVal); | ||
| applyTraceContext(objVal, captureCurrentTraceContext()); | ||
|
delthas marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we always serialize objet before writing it, should we not apply traceContext in |
||
| const c = this.getCollection<ObjectMetastoreDocument>(bucketName); | ||
| const _params = Object.assign({}, params); | ||
| return this.getBucketVFormat(bucketName, log, (err, vFormat?) => { | ||
|
|
@@ -1635,6 +1638,7 @@ class MongoClientInterface { | |
| const masterKey = formatMasterKey(objName, vFormat); | ||
| MongoUtils.serialize(objVal); | ||
| objVal.originOp = 's3:ObjectRemoved:Delete'; | ||
| applyTraceContext(objVal, captureCurrentTraceContext()); | ||
| c.findOneAndReplace({ | ||
| '_id': masterKey, | ||
| 'value.isPHD': true, | ||
|
|
@@ -2073,6 +2077,7 @@ class MongoClientInterface { | |
| const obj = doc.value; | ||
| const objMetadata = new ObjectMD(obj.value); | ||
| objMetadata.setOriginOp(originOp); | ||
| objMetadata.setTraceContext(captureCurrentTraceContext()); | ||
|
DarkIsDude marked this conversation as resolved.
|
||
| objMetadata.setDeleted(true); | ||
| return next(null, objMetadata.getValue()); | ||
| }).catch(err => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| 'use strict'; | ||
|
|
||
| const assert = require('assert'); | ||
|
|
||
| // Mock @opentelemetry/api so we can drive captureCurrentTraceContext | ||
| // without needing a registered SDK or propagator. The api package is a | ||
| // runtime dep of arsenal but we can stub the small surface it consumes. | ||
| const mockActive = jest.fn(); | ||
| const mockGetSpan = jest.fn(); | ||
| const mockInject = jest.fn(); | ||
|
|
||
| jest.mock('@opentelemetry/api', () => ({ | ||
| context: { active: mockActive }, | ||
| trace: { getSpan: mockGetSpan }, | ||
| propagation: { inject: mockInject }, | ||
| })); | ||
|
|
||
| const { | ||
| captureCurrentTraceContext, | ||
| } = require('../../../../lib/storage/metadata/captureTraceContext'); | ||
|
|
||
| describe('captureCurrentTraceContext', () => { | ||
| beforeEach(() => { | ||
| mockActive.mockReset(); | ||
| mockGetSpan.mockReset(); | ||
| mockInject.mockReset(); | ||
| mockActive.mockReturnValue({ tag: 'mock-active-context' }); | ||
| }); | ||
|
|
||
| it('returns undefined when no span is active', () => { | ||
| mockGetSpan.mockReturnValue(undefined); | ||
| assert.strictEqual(captureCurrentTraceContext(), undefined); | ||
| // No injection should be attempted when there is no active span. | ||
| assert.strictEqual(mockInject.mock.calls.length, 0); | ||
| }); | ||
|
|
||
| it('returns { traceparent } when active span yields a traceparent', () => { | ||
| mockGetSpan.mockReturnValue({ /* opaque span object */ }); | ||
| mockInject.mockImplementation((ctx, carrier, setter) => { | ||
| setter.set(carrier, 'traceparent', | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01'); | ||
| }); | ||
|
|
||
| assert.deepStrictEqual(captureCurrentTraceContext(), { | ||
| traceparent: | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01', | ||
| }); | ||
|
|
||
| // Verify inject was given the active context so propagation | ||
| // sees the right state, not a freshly-constructed empty one. | ||
| assert.strictEqual(mockInject.mock.calls.length, 1); | ||
| const [ctxArg, carrierArg] = mockInject.mock.calls[0]; | ||
| assert.deepStrictEqual(ctxArg, { tag: 'mock-active-context' }); | ||
| assert.strictEqual(typeof carrierArg, 'object'); | ||
| }); | ||
|
|
||
| it('returns both traceparent and tracestate when both are present', () => { | ||
| mockGetSpan.mockReturnValue({}); | ||
| mockInject.mockImplementation((ctx, carrier, setter) => { | ||
| setter.set(carrier, 'traceparent', | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01'); | ||
| setter.set(carrier, 'tracestate', 'rojo=00f067aa0ba902b7'); | ||
| }); | ||
|
|
||
| assert.deepStrictEqual(captureCurrentTraceContext(), { | ||
| traceparent: | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01', | ||
| tracestate: 'rojo=00f067aa0ba902b7', | ||
| }); | ||
| }); | ||
|
|
||
| it('returns undefined when propagation injects no traceparent', () => { | ||
| // Defensive case: a misbehaving propagator that exposes only | ||
| // unrelated headers. We must not return a partial / invalid | ||
| // trace context. | ||
| mockGetSpan.mockReturnValue({}); | ||
| mockInject.mockImplementation((ctx, carrier, setter) => { | ||
| setter.set(carrier, 'baggage', 'unrelated=true'); | ||
| }); | ||
|
|
||
| assert.strictEqual(captureCurrentTraceContext(), undefined); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State is a string or a list of possible value ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those names are standard, come from a W3C spec: https://www.w3.org/TR/trace-context/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delthas indeed, I saw that we use them in several place in the code but when stored in mongo, under the traceContext structure, it don't make sense anymore to keep
traceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd heavily lean towards keeping the standard names, as they have specific formats, with meanings specified in the specification. Specifically,
traceparentis not a plainparentIdat all, it is a carefull formatted string, such astraceparent: 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01, which carries a version, a trace ID, a span ID, and flags, including the standard "is sampled" flag.parentIdwould be outright wrong, but even if we gave it a better name, it would be less precise and more confusing thantraceparent, which is exactly the format the consumer would use. For example, see my backbeat code that recreates an OTEL span from saved trace context: tracecontext and traceparent are the field names used by the library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you prefer to keep it, then let's go. It still don't make sense for me but I'm fine to move forward