Address valid review comments
Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
This commit is contained in:
@@ -129,13 +129,14 @@ The context returned by `propagation.extract` and `context.active` is an opaque
|
||||
|
||||
## Reading Baggage
|
||||
|
||||
Use `propagation.getActiveBaggage()` to read baggage entries from the currently active context. This is useful for forwarding caller-set metadata onto your spans — for example, a request ID, tenant identifier, or feature-flag context that the caller propagated via baggage:
|
||||
Use `propagation.getActiveBaggage()` to read baggage entries from the currently active context. This is useful for forwarding caller-set metadata onto your spans — for example, a request ID, tenant identifier, or feature-flag context that the caller propagated via baggage. The baggage is exposed as a flat list of entries — iterate through them to find the keys you care about:
|
||||
|
||||
```ts
|
||||
const baggage = tracing.propagation.getActiveBaggage();
|
||||
const tenantId = baggage?.getEntry('app.tenant.id')?.value;
|
||||
if (tenantId) {
|
||||
span.setAttribute('app.tenant.id', tenantId);
|
||||
for (const [key, entry] of baggage?.getAllEntries() ?? []) {
|
||||
if (key === 'app.tenant.id') {
|
||||
span.setAttribute('app.tenant.id', entry.value);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
@@ -143,12 +144,11 @@ Use `propagation.getBaggage(ctx)` when you already hold a specific context handl
|
||||
|
||||
The returned object exposes:
|
||||
|
||||
| Method | Description |
|
||||
| ----------------- | -------------------------------------------------------- |
|
||||
| `getEntry(key)` | Returns `{ value: string }` for the key, or `undefined`. |
|
||||
| `getAllEntries()` | Returns all entries as `[key, { value }][]`. |
|
||||
| Method | Description |
|
||||
| ----------------- | -------------------------------------------- |
|
||||
| `getAllEntries()` | Returns all entries as `[key, { value }][]`. |
|
||||
|
||||
Both calls return `undefined` when no baggage is present.
|
||||
Both calls return `undefined` when no baggage is present. Single-key lookups are intentionally not provided — baggage is meant for bridging caller metadata onto spans or metrics, not as a general-purpose key-value store.
|
||||
|
||||
## Principal Enrichment
|
||||
|
||||
|
||||
@@ -89,6 +89,13 @@ export class DefaultTracingService implements TracingService {
|
||||
|
||||
readonly context: TracingServiceContextAPI = {
|
||||
active: () => fromOtelContext(otelContext.active()),
|
||||
// `otelContext.with` is synchronous: it activates `ctx`, invokes `fn`,
|
||||
// then restores the previous active context before this call returns.
|
||||
// When `fn` is async, the AsyncLocalStorage context manager installed
|
||||
// by the OTel SDK is what keeps `ctx` active across the callback's
|
||||
// `await`s. If no context manager is registered (e.g. in a test that
|
||||
// does not wire up the OTel SDK) the `await` continuations will run
|
||||
// outside `ctx`.
|
||||
with: async <T>(
|
||||
ctx: TracingServiceContext,
|
||||
fn: () => T | Promise<T>,
|
||||
|
||||
@@ -130,6 +130,12 @@ export interface MockedTracingServicePropagationAPI
|
||||
* `mockReturnValue` / `mockImplementation`, which takes precedence over
|
||||
* the default behaviour.
|
||||
*
|
||||
* Unlike the real `DefaultTracingService`, the mock's `startActiveSpan`
|
||||
* does **not** resolve `options.credentials` from `options.request` via
|
||||
* `httpAuth`. Tests that need principal-derived span attributes should
|
||||
* supply `options.credentials` directly on the span options, or assert
|
||||
* on the raw `options` captured by `startActiveSpan.mock.calls`.
|
||||
*
|
||||
* @alpha
|
||||
*/
|
||||
export interface TracingServiceMock extends TracingService {
|
||||
|
||||
@@ -1055,6 +1055,37 @@ describe('McpService', () => {
|
||||
expect(options?.attributes).not.toHaveProperty('gen_ai.agent.id');
|
||||
});
|
||||
|
||||
it('threads baggage end-to-end from a propagated baggage header through context.with into the tool span', async () => {
|
||||
const tracing = tracingServiceMock.mock();
|
||||
// Simulate what the routers do on incoming requests: extract context
|
||||
// from headers and run the handler with that context active.
|
||||
const ctx = tracing.propagation.extract(tracing.context.active(), {
|
||||
baggage: 'gen_ai.conversation.id=conv-end-to-end',
|
||||
});
|
||||
await tracing.context.with(ctx, () => invokeMockAction({ tracing }));
|
||||
|
||||
const [, options] = tracing.startActiveSpan.mock.calls[0];
|
||||
expect(options?.attributes?.['gen_ai.conversation.id']).toBe(
|
||||
'conv-end-to-end',
|
||||
);
|
||||
});
|
||||
|
||||
it('truncates overlong baggage values before stamping them on the span', async () => {
|
||||
const tracing = tracingServiceMock.mock();
|
||||
const longValue = 'a'.repeat(1024);
|
||||
tracing.propagation.getActiveBaggage.mockReturnValue({
|
||||
getAllEntries: () => [['gen_ai.conversation.id', { value: longValue }]],
|
||||
});
|
||||
|
||||
await invokeMockAction({ tracing });
|
||||
|
||||
const [, options] = tracing.startActiveSpan.mock.calls[0];
|
||||
const recorded = options?.attributes?.['gen_ai.conversation.id'];
|
||||
expect(typeof recorded).toBe('string');
|
||||
expect((recorded as string).length).toBe(256);
|
||||
expect(recorded).toBe('a'.repeat(256));
|
||||
});
|
||||
|
||||
it('includes tool arguments in the span options and sets the structured action output as the result attribute when captureToolPayloads is true', async () => {
|
||||
const tracing = tracingServiceMock.mock();
|
||||
await invokeMockAction({ tracing, captureToolPayloads: true });
|
||||
|
||||
@@ -54,6 +54,12 @@ const PROPAGATED_BAGGAGE_ATTRIBUTES: ReadonlySet<string> = new Set([
|
||||
'gen_ai.request.model',
|
||||
]);
|
||||
|
||||
// Cap each forwarded baggage value before it lands on a span attribute.
|
||||
// Baggage values are caller-controlled strings of unbounded length;
|
||||
// allowlisting keys protects against arbitrary attribute names but not
|
||||
// against pathologically large values inflating exported span sizes.
|
||||
const BAGGAGE_ATTRIBUTE_VALUE_MAX_LENGTH = 256;
|
||||
|
||||
function baggageAttributes(
|
||||
tracingService: TracingService,
|
||||
): Record<string, string> {
|
||||
@@ -62,7 +68,7 @@ function baggageAttributes(
|
||||
const attrs: Record<string, string> = {};
|
||||
for (const [key, entry] of baggage.getAllEntries()) {
|
||||
if (PROPAGATED_BAGGAGE_ATTRIBUTES.has(key)) {
|
||||
attrs[key] = entry.value;
|
||||
attrs[key] = entry.value.slice(0, BAGGAGE_ATTRIBUTE_VALUE_MAX_LENGTH);
|
||||
}
|
||||
}
|
||||
return attrs;
|
||||
@@ -219,6 +225,11 @@ export class McpService {
|
||||
}
|
||||
|
||||
// Re-attribute the span to the plugin that owns the action.
|
||||
// This runs after the span has started, so head-based samplers
|
||||
// still see the default `mcp-actions` value when deciding
|
||||
// whether to record the span. The pluginId is only known after
|
||||
// resolving the action via `actions.list`, so the reattribution
|
||||
// is unavoidable.
|
||||
span.setAttribute('backstage.plugin.id', action.pluginId);
|
||||
|
||||
const { output } = await this.actions.invoke({
|
||||
|
||||
Reference in New Issue
Block a user