Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions lib/models/ObjectMD.ts
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, {
Expand Down Expand Up @@ -89,6 +93,10 @@ export type ObjectMDData = {
replicationInfo: ReplicationInfo;
dataStoreName: string;
originOp: string;
traceContext?: {
traceparent?: string;
tracestate?: string;
Comment on lines +97 to +98
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
traceparent?: string;
tracestate?: string;
parentId?: string;
state?: string;

State is a string or a list of possible value ?

Copy link
Copy Markdown
Contributor Author

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/

Copy link
Copy Markdown
Contributor

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 trace

Copy link
Copy Markdown
Contributor Author

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, traceparent is not a plain parentId at all, it is a carefull formatted string, such as traceparent: 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01, which carries a version, a trace ID, a span ID, and flags, including the standard "is sampled" flag. parentId would be outright wrong, but even if we gave it a better name, it would be less precise and more confusing than traceparent, 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.

Copy link
Copy Markdown
Contributor

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

};
microVersionId?: string;
// Deletion flag
// Used for keeping object metadata in the oplog event
Expand Down Expand Up @@ -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) {
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
*
Expand Down
60 changes: 60 additions & 0 deletions lib/storage/metadata/captureTraceContext.ts
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(
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;
}
Comment thread
delthas marked this conversation as resolved.
Comment thread
delthas marked this conversation as resolved.
Comment thread
delthas marked this conversation as resolved.
5 changes: 5 additions & 0 deletions lib/storage/metadata/mongoclient/MongoClientInterface.ts
Comment thread
delthas marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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());
}).catch(err => {
Expand Down Expand Up @@ -1361,6 +1363,7 @@ class MongoClientInterface {
cb: ArsenalCallback<string | void>,
): void {
MongoUtils.serialize(objVal);
applyTraceContext(objVal, captureCurrentTraceContext());
Comment thread
delthas marked this conversation as resolved.
const c = this.getCollection<ObjectMetastoreDocument>(bucketName);
const _params = Object.assign({}, params);
return this.getBucketVFormat(bucketName, log, (err, vFormat?) => {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -2073,6 +2077,7 @@ class MongoClientInterface {
const obj = doc.value;
const objMetadata = new ObjectMD(obj.value);
objMetadata.setOriginOp(originOp);
objMetadata.setTraceContext(captureCurrentTraceContext());
Comment thread
DarkIsDude marked this conversation as resolved.
objMetadata.setDeleted(true);
return next(null, objMetadata.getValue());
}).catch(err => {
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"@azure/identity": "^4.13.0",
"@azure/storage-blob": "^12.31.0",
"@js-sdsl/ordered-set": "^4.4.2",
"@opentelemetry/api": "^1.9.0",
"@scality/hdclient": "^1.3.2",
"@smithy/node-http-handler": "^4.3.0",
"@smithy/protocol-http": "^5.3.5",
Expand Down
53 changes: 53 additions & 0 deletions tests/unit/models/ObjectMD.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,59 @@ describe('ObjectMD class setters/getters', () => {
assert.deepStrictEqual(md.getOriginOp(), 'Copy');
});

it('ObjectMD::set/getTraceContext with valid traceparent', () => {
const tc = {
traceparent: '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
tracestate: 'rojo=00f067aa0ba902b7',
};
md.setTraceContext(tc);
assert.deepStrictEqual(md.getTraceContext(), tc);
});

it('ObjectMD::setTraceContext with undefined clears any existing value', () => {
md.setTraceContext({
traceparent: '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
});
md.setTraceContext(undefined);
assert.strictEqual(md.getTraceContext(), undefined);
assert.strictEqual(md.getValue().traceContext, undefined);
});

it('ObjectMD::setTraceContext without traceparent clears any existing value', () => {
md.setTraceContext({
traceparent: '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
});
md.setTraceContext({ tracestate: 'rojo=00f067aa0ba902b7' });
assert.strictEqual(md.getTraceContext(), undefined);
});

it('ObjectMD reconstructed from existing data clears stale trace context when setTraceContext(undefined) is called', () => {
// Simulates the real hazard: an ObjectMD loaded from storage
// already has a traceContext from a previous write. A new
// operation without an active span must not inherit that stale
// context into its own oplog entry.
const stale = {
traceparent: '00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01',
};
const loaded = new ObjectMD(
new ObjectMD().setTraceContext(stale).getValue(),
);
assert.deepStrictEqual(loaded.getTraceContext(), stale);
loaded.setTraceContext(undefined);
assert.strictEqual(loaded.getTraceContext(), undefined);
assert.strictEqual(loaded.getValue().traceContext, undefined);
});

it('ObjectMD::getValue serializes traceContext only when set', () => {
assert.strictEqual(md.getValue().traceContext, undefined);
md.setTraceContext({
traceparent: '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
});
assert.deepStrictEqual(md.getValue().traceContext, {
traceparent: '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
});
});

it('ObjectMD::set/getAmzRestore', () => {
md.setAmzRestore({
'ongoing-request': false,
Expand Down
83 changes: 83 additions & 0 deletions tests/unit/storage/metadata/captureTraceContext.spec.js
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);
});
});
Loading
Loading