From 86bac7b956c6b320379bd7e141892ea591dbc14c Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Wed, 2 Sep 2026 09:56:32 -0400 Subject: [PATCH 01/22] feat(node): beforeSpanSend hook and per-span limits Adds a scrubbing/drop hook on finished spans, caps user attributes, events and attribute-value length per span, and records exception stacktraces. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0138vKReknFeXLbvMyUX7fPZ --- .changeset/node-before-span-send.md | 7 + .changeset/node-exception-stacktrace.md | 7 + .changeset/node-span-limits.md | 7 + packages/core/src/index.ts | 3 + packages/core/src/traces/index.spec.ts | 734 ++++++++++++++++++ packages/core/src/traces/index.ts | 229 +++++- packages/core/src/traces/live-spans.spec.ts | 4 + packages/core/src/traces/otlp.spec.ts | 4 + packages/core/src/traces/otlp.ts | 11 + packages/core/src/traces/span.spec.ts | 410 +++++++++- packages/core/src/traces/span.ts | 440 ++++++++++- packages/core/src/traces/types.ts | 17 +- .../src/__tests__/traces-defaults.spec.ts | 37 + packages/node/src/__tests__/traces.spec.ts | 120 +++ packages/node/src/exports.ts | 2 + packages/node/src/traces-defaults.ts | 27 +- packages/types/src/index.ts | 2 + packages/types/src/traces.ts | 103 ++- 18 files changed, 2120 insertions(+), 44 deletions(-) create mode 100644 .changeset/node-before-span-send.md create mode 100644 .changeset/node-exception-stacktrace.md create mode 100644 .changeset/node-span-limits.md diff --git a/.changeset/node-before-span-send.md b/.changeset/node-before-span-send.md new file mode 100644 index 0000000000..44a95b6da2 --- /dev/null +++ b/.changeset/node-before-span-send.md @@ -0,0 +1,7 @@ +--- +'posthog-node': minor +'@posthog/core': minor +'@posthog/types': minor +--- + +Add a `traces.beforeSpanSend` hook to scrub attributes on a finished span, or return `null` to drop it. diff --git a/.changeset/node-exception-stacktrace.md b/.changeset/node-exception-stacktrace.md new file mode 100644 index 0000000000..efe7d6d27f --- /dev/null +++ b/.changeset/node-exception-stacktrace.md @@ -0,0 +1,7 @@ +--- +'posthog-node': minor +'@posthog/core': minor +'@posthog/types': minor +--- + +Record `exception.stacktrace` on spans: `recordException` and a throwing `withSpan` callback now attach the stack alongside `exception.type` and `exception.message`. Stacks are attached by default and carry your server's file paths; remove the attribute in `traces.beforeSpanSend` if you'd rather they didn't leave the process. The value is bounded by `traces.maxAttributeValueLength` like any other attribute. diff --git a/.changeset/node-span-limits.md b/.changeset/node-span-limits.md new file mode 100644 index 0000000000..9d5fe27d45 --- /dev/null +++ b/.changeset/node-span-limits.md @@ -0,0 +1,7 @@ +--- +'posthog-node': minor +'@posthog/core': minor +'@posthog/types': minor +--- + +Cap spans at 128 user attributes, 128 events, and 8192 characters per string attribute value, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. The character bound applies to every string a value contains, including the ones nested inside arrays and objects, and to status messages and resource attributes as well. Once a span has spent its event cap, a small reserve stays available to `exception` events, so a span that fills its events and then throws still carries the exception. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f8b93ec090..72396f0ba6 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -92,6 +92,9 @@ export { PostHogTraces } from './traces' export { SyncSpanContextManager } from './traces/context' export { NOOP_SPAN, inertSpan } from './traces/span' export type { ResolvedTracesConfig, SpanContextManager, TraceSdkContext } from './traces/types' +// The `beforeSpanSend` shapes come straight from @posthog/types: hooks see the +// public record, not core's internal one, which also carries `traceState`. +export type { SpanRecord, BeforeSpanSendFn } from '@posthog/types' // Same barrel convention as logs and metrics for the user-facing tracing types. export type { Span, diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 1af123fe3b..4da67639b0 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -6,6 +6,7 @@ import type { OtlpTracesPayload, ResolvedTracesConfig, SendTracesBatchOutcome, + SpanRecord, TraceSdkContext, } from './types' import type { Logger } from '../types' @@ -18,6 +19,10 @@ const resolveForTest = (partial?: Partial): ResolvedTraces flushIntervalMs: 5000, maxExportBatchSize: 512, maxQueueSize: 2048, + beforeSpanSend: [], + maxAttributesPerSpan: 128, + maxEventsPerSpan: 128, + maxAttributeValueLength: 8192, maxLiveSpans: 10000, maxSpanAgeMs: 3600000, ...partial, @@ -332,6 +337,7 @@ describe('PostHogTraces', () => { attributes: [ { key: 'exception.type', value: { stringValue: 'TypeError' } }, { key: 'exception.message', value: { stringValue: 'boom' } }, + { key: 'exception.stacktrace', value: { stringValue: expect.stringContaining('TypeError: boom') } }, ], }) }) @@ -570,6 +576,720 @@ describe('PostHogTraces', () => { }) }) + describe('beforeSpanSend', () => { + const endOneSpan = (beforeSpanSend: any): PostHogTraces => { + const traces = createTraces({ beforeSpanSend: [beforeSpanSend].flat() }) + traces.startSpan('checkout', { attributes: { userId: 42 } }).end() + return traces + } + + it('drops a span when the hook returns null', async () => { + await endOneSpan(() => null).flush() + expect(sentSpans()).toHaveLength(0) + }) + + it('drops the span when the hook throws', async () => { + await endOneSpan(() => { + throw new Error('scrubber broke') + }).flush() + + expect(sentSpans()).toHaveLength(0) + expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('beforeSpanSend failed'), expect.anything()) + }) + + it('counts a span the hook dropped', async () => { + await endOneSpan(() => null).flush() + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('beforeSpanSend dropped it')) + }) + + it('counts a span dropped because the hook threw', async () => { + // A permanently broken scrubber otherwise drops every span with the drop + // counter reading zero. + await endOneSpan(() => { + throw new Error('scrubber broke') + }).flush() + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('beforeSpanSend failed')) + }) + + it('hands the hook plain values, not the OTLP encoding', () => { + const seen: unknown[] = [] + endOneSpan((span: SpanRecord) => { + seen.push(span.attributes.userId) + return span + }) + + expect(seen).toEqual([42]) + }) + + it('keeps the original ids when a hook rewrites them', async () => { + const traces = createTraces({ + beforeSpanSend: [ + (span: any) => { + span.traceId = '0'.repeat(32) + span.spanId = '1'.repeat(16) + return span + }, + ], + }) + const started = traces.startSpan('checkout') + const originalTraceId = started.traceparent()!.split('-')[1] + started.end() + await traces.flush() + + const [span] = sentSpans() + expect(span.traceId).toBe(originalTraceId) + expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('identity field')) + }) + + it('exports a span whose record the hook froze', async () => { + // A defensive hook may freeze what it returns. Assigning to a frozen + // property throws even when the value is the one already there, so the + // post-hook pass works on a copy — otherwise every span the hook saw is + // dropped by the fail-closed branch, with only a debug line to say so. + const traces = createTraces({ + beforeSpanSend: [(span: SpanRecord) => Object.freeze({ ...span, attributes: { route: '/checkout' } })], + }) + const span = traces.startSpan('checkout') + + expect(() => span.end()).not.toThrow() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['route']) + }) + + it('exports a span whose attributes the hook froze', async () => { + const traces = createTraces({ + maxAttributesPerSpan: 1, + beforeSpanSend: [ + (span: SpanRecord) => ({ ...span, attributes: Object.freeze({ route: '/checkout', extra: 1 }) as never }), + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['route']) + expect(sent.droppedAttributesCount).toBe(1) + }) + + it('rejects a timestamp the server could not decode', async () => { + const instance = createMockInstance() + const traces = createTraces( + { beforeSpanSend: [(span: SpanRecord) => ({ ...span, startTime: span.startTime * 1e6 })] }, + instance + ) + traces.startSpan('poison').end() + await traces.flush() + + const [span] = sentSpans(instance) + expect(span.startTimeUnixNano.length).toBeLessThanOrEqual(19) + }) + + it('keeps tracestate a rebuilding hook would have dropped', async () => { + const instance = createMockInstance() + const traces = createTraces( + { beforeSpanSend: [(span: SpanRecord) => ({ ...span, traceState: undefined }) as SpanRecord] }, + instance + ) + traces.startSpan('child', { parent: `00-${TRACE_ID}-${REMOTE_SPAN_ID}-01`, tracestate: 'vendor=abc' }).end() + await traces.flush() + + expect(sentSpans(instance)[0].traceState).toBe('vendor=abc') + }) + + it('runs hooks left to right and stops at the first null', async () => { + const order: string[] = [] + await endOneSpan([ + (span: SpanRecord) => { + order.push('first') + return span + }, + () => { + order.push('second') + return null + }, + (span: SpanRecord) => { + order.push('third') + return span + }, + ]).flush() + + expect(order).toEqual(['first', 'second']) + expect(sentSpans()).toHaveLength(0) + }) + + it('exports the edits a hook made', async () => { + await endOneSpan((span: SpanRecord) => { + delete span.attributes.userId + span.name = 'redacted' + return span + }).flush() + + const [span] = sentSpans() + expect(span.name).toBe('redacted') + expect(span.attributes?.find((attribute) => attribute.key === 'userId')).toBeUndefined() + }) + }) + + describe('beforeSpanSend validity', () => { + it('sanitises an event the hook pushed without a timestamp', async () => { + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + span.events.push({ name: 'audited' } as never) + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + const [event] = sentSpans()[0].events! + expect(event.name).toBe('audited') + expect(event.timeUnixNano).toMatch(/^\d+$/) + }) + + it('clamps an out-of-range timestamp on a hook-supplied event', async () => { + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + span.events.push({ name: 'audited', timestamp: -1 } as never) + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()[0].events![0].timeUnixNano).toMatch(/^\d+$/) + }) + + it('bounds a status message the hook rewrote', async () => { + const traces = createTraces({ + maxAttributeValueLength: 4, + beforeSpanSend: [ + (span) => { + span.status = { code: 'error', message: 'abcdefgh' } + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()[0].status).toEqual({ code: 2, message: 'abcd' }) + }) + + it('keeps the original status when the hook writes an unknown code', async () => { + // An unknown code maps to nothing and encodes as an empty status object, + // which silently loses an error the span really had. + const traces = createTraces({ + beforeSpanSend: [(span) => ({ ...span, status: { code: 'ERROR' as never, message: 'boom' } })], + }) + const span = traces.startSpan('checkout') + span.setStatus('error', 'boom') + span.end() + await traces.flush() + + expect(sentSpans()[0].status).toEqual({ code: 2, message: 'boom' }) + }) + + it('ignores a dropped count the hook invented', async () => { + const traces = createTraces({ + beforeSpanSend: [(span) => ({ ...span, droppedAttributesCount: 'lots' as never })], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()[0].droppedAttributesCount).toBeUndefined() + }) + + it('lets a hook scrub the auto-context keys', async () => { + // The exemption is from the count cap only. A hook is the documented + // scrubbing point, so it has to be able to remove the join keys as well. + context = { distinctId: 'user-1', sessionId: 'session-1' } + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + delete span.attributes.posthogDistinctId + delete span.attributes.sessionId + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()[0].attributes).toBeUndefined() + }) + + it('keeps the exception event when the hook pushes past the event cap', async () => { + // The re-apply used to slice to the first `maxEvents`, and an exception + // event is the last thing on a span that threw — exactly what a slice cuts. + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + span.events.push({ name: 'audited', timestamp: Date.now() }) + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step') + span.recordException(new Error('boom')) + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.events!.map((event) => event.name)).toEqual(['step', 'exception']) + expect(sent.droppedEventsCount).toBe(1) + expect(sent.status).toEqual({ code: 2, message: 'boom' }) + }) + + it('keeps the original status when the hook mutates the code in place', async () => { + // The hook is documented as editing the record in place, so snapshotting a + // reference to `status` would restore the mutation onto itself. + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + ;(span.status as { code: string }).code = 'ERROR' + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.setStatus('error', 'boom') + span.end() + await traces.flush() + + expect(sentSpans()[0].status).toEqual({ code: 2, message: 'boom' }) + }) + + it('exports a span whose record is a class instance with prototype getters', async () => { + // A spread copies own properties only, so `events` behind a prototype + // getter arrived undefined and the fail-closed branch ate every span. + class Wrapped { + constructor(private readonly _inner: SpanRecord) {} + get traceId(): string { + return this._inner.traceId + } + get spanId(): string { + return this._inner.spanId + } + get name(): string { + return this._inner.name + } + get kind(): SpanRecord['kind'] { + return this._inner.kind + } + get attributes(): SpanRecord['attributes'] { + return this._inner.attributes + } + get events(): SpanRecord['events'] { + return this._inner.events + } + get startTime(): number { + return this._inner.startTime + } + get endTime(): number { + return this._inner.endTime + } + } + const traces = createTraces({ + beforeSpanSend: [(span: SpanRecord) => new Wrapped(span) as unknown as SpanRecord], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans().map((span) => span.name)).toEqual(['checkout']) + }) + + it('survives a hook that leaves a hole in the events array', async () => { + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + span.events.length = 2 + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step') + span.end() + await traces.flush() + + expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step']) + }) + + it('keeps the span-side dropped count when the hook overwrites the counter', async () => { + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + ;(span as unknown as { droppedEventsCount: unknown }).droppedEventsCount = 'lots' + span.events.push({ name: 'audited', timestamp: Date.now() }) + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step-0') + span.addEvent('step-1') + span.end() + await traces.flush() + + // One dropped at the span, one by the post-hook re-apply. + expect(sentSpans()[0].droppedEventsCount).toBe(2) + }) + + it('drops only the event the hook made unreadable', async () => { + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + span.events = [span.events[0], null as never, span.events[1]] + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step-0') + span.addEvent('step-1') + span.end() + await traces.flush() + + expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0', 'step-1']) + }) + + it.each([ + ['attributes replaced with null', (span: SpanRecord) => ({ ...span, attributes: null as never })], + ['attributes replaced with an array', (span: SpanRecord) => ({ ...span, attributes: ['a'] as never })], + ['events replaced with null', (span: SpanRecord) => ({ ...span, events: null as never })], + ['an async hook returning a promise', (span: SpanRecord) => Promise.resolve(span) as never], + ])('drops the span when the hook returns %s', async (_label, beforeSpanSend) => { + // Repairing these would export a nameless span carrying no join keys. + const traces = createTraces({ beforeSpanSend: [beforeSpanSend] }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()).toHaveLength(0) + }) + + it('exports the span when the hook status message refuses to stringify', async () => { + // The encoder downstream only marks the field, so coercing here must not + // be the thing that costs the span. + const traces = createTraces({ + maxAttributeValueLength: 4, + beforeSpanSend: [ + (span) => { + span.status = { + code: 'error', + message: { + toString() { + throw new Error('nope') + }, + } as never, + } + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()).toHaveLength(1) + expect(sentSpans()[0].status?.code).toBe(2) + }) + + it('bounds a non-string status message the hook wrote', async () => { + const traces = createTraces({ + maxAttributeValueLength: 4, + beforeSpanSend: [ + (span) => { + span.status = { code: 'error', message: { toString: () => 'abcdefgh' } as never } + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()[0].status).toEqual({ code: 2, message: 'abcd' }) + }) + + it('does not spend cap budget on a value the hook blanked', async () => { + const traces = createTraces({ + maxAttributesPerSpan: 2, + beforeSpanSend: [ + (span) => { + span.attributes.secret = null + span.attributes.scrubbed = true + return span + }, + ], + }) + traces.startSpan('checkout', { attributes: { secret: 'sk-live', route: '/checkout' } }).end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['route', 'scrubbed']) + expect(sent.droppedAttributesCount).toBeUndefined() + }) + }) + + describe('span limits', () => { + it('keeps the earliest attributes and counts the rest', async () => { + const traces = createTraces({ maxAttributesPerSpan: 3 }) + const span = traces.startSpan('checkout') + for (let i = 0; i < 5; i++) { + span.setAttribute(`key-${i}`, i) + } + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['key-0', 'key-1', 'key-2']) + expect(sent.droppedAttributesCount).toBe(2) + }) + + it('re-applies the attribute cap to what beforeSpanSend added', async () => { + const traces = createTraces({ + maxAttributesPerSpan: 2, + beforeSpanSend: [ + (span) => { + for (let i = 0; i < 5; i++) { + span.attributes[`added-${i}`] = i + } + return span + }, + ], + }) + traces.startSpan('checkout', { attributes: { kept: true } }).end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['kept', 'added-0']) + expect(sent.droppedAttributesCount).toBe(4) + }) + + it('re-applies the event cap to what beforeSpanSend added', async () => { + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + span.events.push({ name: 'added', timestamp: span.startTime }) + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('original') + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.events!.map((event) => event.name)).toEqual(['original']) + expect(sent.droppedEventsCount).toBe(1) + }) + + it('keeps the auto-context keys when beforeSpanSend pushes past the cap', async () => { + context = { distinctId: 'alice', sessionId: 'session-1' } + const traces = createTraces({ + maxAttributesPerSpan: 1, + beforeSpanSend: [ + (span) => { + span.attributes.late = true + return span + }, + ], + }) + traces.startSpan('checkout', { attributes: { early: true } }).end() + await traces.flush() + + const keys = sentSpans()[0].attributes!.map((attribute) => attribute.key) + expect(keys).toEqual(expect.arrayContaining(['posthogDistinctId', 'sessionId', 'early'])) + expect(keys).not.toContain('late') + }) + + it('re-applies the value bound to what beforeSpanSend wrote', async () => { + const traces = createTraces({ + maxAttributeValueLength: 8, + beforeSpanSend: [ + (span) => { + span.attributes.enriched = 'y'.repeat(5000) + span.events.push({ name: 'added', timestamp: span.startTime, attributes: { blob: 'z'.repeat(5000) } }) + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.find((attribute) => attribute.key === 'enriched')!.value).toEqual({ + stringValue: 'yyyyyyyy', + }) + expect(sent.events!.at(-1)!.attributes!.find((attribute) => attribute.key === 'blob')!.value).toEqual({ + stringValue: 'zzzzzzzz', + }) + }) + + it('does not invent a dropped count when beforeSpanSend only removes', async () => { + const traces = createTraces({ + maxAttributesPerSpan: 5, + beforeSpanSend: [ + (span) => { + delete span.attributes.secret + return span + }, + ], + }) + traces.startSpan('checkout', { attributes: { secret: 'x', kept: true } }).end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['kept']) + expect(sent.droppedAttributesCount).toBeUndefined() + }) + + it('never evicts the auto-context keys', async () => { + context = { distinctId: 'alice', sessionId: 'session-1' } + const traces = createTraces({ maxAttributesPerSpan: 1 }) + const span = traces.startSpan('checkout') + for (let i = 0; i < 5; i++) { + span.setAttribute(`key-${i}`, i) + } + span.end() + await traces.flush() + + const keys = sentSpans()[0].attributes!.map((attribute) => attribute.key) + expect(keys).toEqual(expect.arrayContaining(['posthogDistinctId', 'sessionId', 'key-0'])) + expect(keys).not.toContain('key-1') + }) + + it('caps events and counts the rest', async () => { + const traces = createTraces({ maxEventsPerSpan: 2 }) + const span = traces.startSpan('checkout') + for (let i = 0; i < 4; i++) { + span.addEvent(`event-${i}`) + } + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.events!.map((event) => event.name)).toEqual(['event-0', 'event-1']) + expect(sent.droppedEventsCount).toBe(2) + }) + + it('lets a caller overwrite an attribute it already set while at the cap', async () => { + const traces = createTraces({ maxAttributesPerSpan: 1 }) + const span = traces.startSpan('checkout') + span.setAttribute('plan', 'free') + span.setAttribute('plan', 'pro') + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes).toEqual([{ key: 'plan', value: { stringValue: 'pro' } }]) + expect(sent.droppedAttributesCount).toBeUndefined() + }) + + it('omits the counters when nothing was dropped', async () => { + const traces = createTraces() + traces.startSpan('checkout', { attributes: { plan: 'pro' } }).end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent).not.toHaveProperty('droppedAttributesCount') + expect(sent).not.toHaveProperty('droppedEventsCount') + }) + + it('counts a parsed __proto__ key against the cap instead of smuggling it through', async () => { + // JSON.parse produces an own `__proto__` key; a plain object store would + // swap its prototype and leak every nested key past the cap. + const traces = createTraces({ maxAttributesPerSpan: 2 }) + const parsed = JSON.parse('{"__proto__": {"leaked": 1}, "orderId": "abc"}') + traces.startSpan('checkout', { attributes: parsed }).end() + await traces.flush() + + const keys = sentSpans()[0].attributes!.map((attribute) => attribute.key) + expect(keys).not.toContain('leaked') + expect(keys).toContain('orderId') + }) + + it('does not let reserved property names bypass the cap', async () => { + const traces = createTraces({ maxAttributesPerSpan: 1 }) + const span = traces.startSpan('checkout') + span.setAttribute('kept', 1) + span.setAttribute('toString', 'nope') + span.setAttribute('constructor', 'nope') + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['kept']) + expect(sent.droppedAttributesCount).toBe(2) + }) + + it('does not spend cap budget on values that are dropped at encode time', async () => { + const traces = createTraces({ maxAttributesPerSpan: 2 }) + const span = traces.startSpan('checkout') + span.setAttribute('skipped-a', undefined) + span.setAttribute('skipped-b', null) + span.setAttribute('orderId', 'abc-123') + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['orderId']) + expect(sent.droppedAttributesCount).toBeUndefined() + }) + + it('still caps a key first seen with an optional value', async () => { + const traces = createTraces({ maxAttributesPerSpan: 2 }) + const span = traces.startSpan('checkout') + for (let i = 0; i < 5; i++) { + span.setAttribute(`field-${i}`, undefined) + } + for (let i = 0; i < 5; i++) { + span.setAttribute(`field-${i}`, i) + } + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes).toHaveLength(2) + expect(sent.droppedAttributesCount).toBe(3) + }) + + it('clears a key that is set back to null', async () => { + const traces = createTraces({ maxAttributesPerSpan: 2 }) + const span = traces.startSpan('checkout') + span.setAttribute('orderId', 'abc-123') + span.setAttribute('orderId', null) + span.setAttribute('a', 1) + span.setAttribute('b', 2) + span.end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['a', 'b']) + expect(sent.droppedAttributesCount).toBeUndefined() + }) + + it('caps attributes supplied at start', async () => { + const traces = createTraces({ maxAttributesPerSpan: 2 }) + traces.startSpan('checkout', { attributes: { a: 1, b: 2, c: 3 } }).end() + await traces.flush() + + const [sent] = sentSpans() + expect(sent.attributes!.map((attribute) => attribute.key)).toEqual(['a', 'b']) + expect(sent.droppedAttributesCount).toBe(1) + }) + }) + describe('export', () => { it('flushes when the queue reaches the batch size', async () => { const traces = createTraces({ maxExportBatchSize: 2 }) @@ -1057,6 +1777,20 @@ describe('PostHogTraces', () => { expect(sentSpans().map((span) => span.name)).toEqual(['checkout']) }) + it('bounds a long resource attribute value', async () => { + // Resource attributes are caller-supplied like span attributes, and they + // ride on every batch rather than on one span. + const traces = createTraces({ + maxAttributeValueLength: 4, + resourceAttributes: { 'host.name': 'abcdefgh' } as never, + }) + traces.startSpan('checkout').end() + await traces.flush() + + const resource = sentPayloads()[0].resourceSpans[0].resource!.attributes + expect(resource.find((attribute) => attribute.key === 'host.name')?.value).toEqual({ stringValue: 'abcd' }) + }) + it('does not throw on a Date-like object with no Date slot', () => { const traces = createTraces() const fakeDate = Object.create(Date.prototype) diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 0281556118..fa00e87cb9 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -4,14 +4,30 @@ import type { OtlpSpan, ResolvedTracesConfig, SpanContextManager, + SpanEventRecord, SpanRecord, TraceSdkContext, TracesHost, } from './types' -import { NOOP_SPAN, PostHogSpan, describeError, inertSpan, monotonicNow } from './span' +import { + NOOP_SPAN, + PostHogSpan, + applySpanLimits, + describeError, + inertSpan, + monotonicNow, + truncateAttributes, +} from './span' import { newSpanId, newTraceId } from './ids' import { parseTraceparent, sanitizeTracestate } from './traceparent' -import { assignUserAttributes, resolveStartTime, sanitizeName } from './sanitize' +import { + assignUserAttributes, + clampEndTime, + resolveStartTime, + resolveSuppliedTime, + sanitizeName, + toEpochMs, +} from './sanitize' import { buildOtlpSpan, buildOtlpTracesPayload, buildTracesResourceAttributes } from './otlp' import { isPromise, safeSetTimeout } from '../utils' @@ -49,6 +65,34 @@ function looksLikeSpan(value: unknown): boolean { } } +interface SpanIdentity { + traceId: string + spanId: string + parentSpanId?: string + traceState?: string +} + +/** + * Whether a `beforeSpanSend` return value still carries the two collections the + * rest of the pipeline reads. An array is rejected for `attributes`: it would + * encode as `{ "0": ... }` rather than fail. + */ +function isSpanRecordShape(record: SpanRecord): boolean { + return ( + !!record.attributes && + typeof record.attributes === 'object' && + !Array.isArray(record.attributes) && + Array.isArray(record.events) + ) +} + +/** Writes `value` onto `record` only when it isn't already there. */ +function restoreField(record: SpanIdentity, field: K, value: SpanIdentity[K]): void { + if (record[field] !== value) { + record[field] = value + } +} + interface ParentContext { traceId: string parentSpanId?: string @@ -141,6 +185,8 @@ export class PostHogTraces { // so a backdated `startTime` neither ages a span early nor exempts it. this._liveSpans.set(spanId, clockNow()) + const autoAttributes = this._autoContextAttributes() + return new PostHogSpan( { traceId: parent?.traceId ?? newTraceId(), @@ -150,11 +196,15 @@ export class PostHogTraces { name: sanitizeName(name, 'Span name', this._logger), kind: options?.kind ?? 'internal', // Auto-context first so user-supplied attributes win on collision. - attributes: assignUserAttributes(this._autoContextAttributes(), options?.attributes), + attributes: assignUserAttributes({ ...autoAttributes }, options?.attributes), + autoAttributeKeys: Object.keys(autoAttributes), + maxAttributes: this._config.maxAttributesPerSpan, + maxEvents: this._config.maxEventsPerSpan, + maxAttributeValueLength: this._config.maxAttributeValueLength, startTime, backdated: startTime !== now, }, - (record) => this._onSpanEnd(record), + (record, autoKeys) => this._onSpanEnd(record, autoKeys), this._logger ) } @@ -333,8 +383,12 @@ export class PostHogTraces { if (!(span instanceof PostHogSpan)) { return } - const { type, message } = describeError(error) - span.addEvent('exception', { 'exception.type': type, 'exception.message': message }) + const { type, message, stack } = describeError(error) + span.addEvent('exception', { + 'exception.type': type, + 'exception.message': message, + ...(stack && { 'exception.stacktrace': stack }), + }) if (!span.statusIsExplicitlyOk) { span.setStatus('error', message) } @@ -361,9 +415,9 @@ export class PostHogTraces { } } - private _onSpanEnd(record: SpanRecord): void { - // Deleted before any other gate, so an opted-out span still returns its slot. - if (!this._liveSpans.delete(record.spanId)) { + private _onSpanEnd(incoming: SpanRecord, autoKeys: ReadonlySet): void { + // Deleted before any other gate, so a span dropped later still returns its slot. + if (!this._liveSpans.delete(incoming.spanId)) { // Evicted for age while live: never exported, and already counted as a drop. return } @@ -374,6 +428,11 @@ export class PostHogTraces { return } + const record = this._runBeforeSpanSend(incoming, autoKeys) + if (!record) { + return + } + if (this._queue.length >= this._config.maxQueueSize) { // Drop the incoming span, not queued ones: those are completed parents whose // children may already have shipped. @@ -401,6 +460,149 @@ export class PostHogTraces { } } + /** + * Runs the `beforeSpanSend` chain, returning the span to enqueue or `null` to + * drop it. + * + * A throwing hook drops the span: the hook is the documented scrubbing point, + * so a broken scrubber must not let the unscrubbed record through. Identity + * fields are restored afterwards, since rewriting them orphans shipped children. + */ + private _runBeforeSpanSend(record: SpanRecord, autoKeys: ReadonlySet): SpanRecord | null { + if (!this._config.beforeSpanSend.length) { + return record + } + + // Snapshotted before any hook runs: a hook that mutates in place would + // otherwise leave nothing to restore from. + const identity = { + traceId: record.traceId, + spanId: record.spanId, + parentSpanId: record.parentSpanId, + traceState: record.traceState, + } + const originalTimes = { startTime: record.startTime, endTime: record.endTime } + // Snapshotted with the rest: the hook mutates the record in place, so reading + // these back afterwards reads whatever the hook left there. + const originalDropped = { + attributes: record.droppedAttributesCount, + events: record.droppedEventsCount, + } + // Copied, not referenced: the hook is documented as mutating the record in + // place, and a reference would restore the mutation onto itself. + const originalStatus = record.status && { ...record.status } + let current = record + try { + for (const hook of this._config.beforeSpanSend) { + const result = hook(current) + if (!result) { + this._recordDrop(1, 'beforeSpanSend dropped it') + return null + } + current = this._keepSpanIdentity(result, identity) + } + + // Rebuilt field by field before anything below writes to it. The hook's + // return value may be frozen, where every write here would throw, or a + // class instance whose fields are prototype getters a spread would miss. + // Naming them also bounds what can reach the wire. + current = { + traceId: current.traceId, + spanId: current.spanId, + parentSpanId: current.parentSpanId, + traceState: current.traceState, + name: current.name, + kind: current.kind, + status: current.status, + attributes: current.attributes, + events: current.events, + startTime: current.startTime, + endTime: current.endTime, + // Taken from the span, not from the hook's return value: these are SDK + // bookkeeping that no public type declares, so a hook overwriting them + // must not erase what the span actually dropped. + droppedAttributesCount: originalDropped.attributes, + droppedEventsCount: originalDropped.events, + } + // A value missing either collection is not a span record — an `async` + // hook returns a Promise, truthy and `undefined` for every field. Filling + // the gaps in would export a nameless span carrying no person or session. + if (!isSpanRecordShape(current)) { + this._logger.debug('beforeSpanSend did not return a span record; dropping the span') + this._recordDrop(1, 'beforeSpanSend returned an unusable record') + return null + } + + // Re-applied to whatever the hook returned: one undecodable timestamp 400s + // the whole request, taking unrelated spans with it. + current.name = sanitizeName(current.name, 'Span name', this._logger) + // A status the hook rewrote never went through `setStatus`. An unknown code + // encodes as an empty status object, which loses an error the span really had. + if (current.status && current.status.code !== 'ok' && current.status.code !== 'error') { + this._logger.debug('beforeSpanSend set an unknown span status; keeping the original') + current.status = originalStatus + } + current.startTime = toEpochMs(current.startTime) ?? originalTimes.startTime + current.endTime = clampEndTime(toEpochMs(current.endTime) ?? originalTimes.endTime, current.startTime) + // Events a hook pushed never went through `addEvent`, so they carry no + // sanitised name or timestamp; an unvalidated one encodes as `NaN000NaN` + // and the ingestion service refuses the whole batch. + const sanitizedEvents: SpanEventRecord[] = [] + for (const event of current.events) { + try { + sanitizedEvents.push({ + ...event, + name: sanitizeName(event.name, 'Span event name', this._logger), + timestamp: resolveSuppliedTime(event.timestamp, current.startTime, 'event timestamp', this._logger), + }) + } catch { + // A hook can leave a `null` in the array or a throwing accessor on an + // event. That costs the event; the rest of the span still ships. + this._logger.debug('beforeSpanSend left an unreadable span event; dropping it') + } + } + current.events = sanitizedEvents + applySpanLimits( + current, + autoKeys, + this._config.maxAttributesPerSpan, + this._config.maxEventsPerSpan, + this._config.maxAttributeValueLength + ) + return current + } catch (error) { + // Covers the hook and everything done to its return value: a frozen or + // hostile record must not throw out of `end()` into application code. + this._logger.debug('beforeSpanSend failed; dropping the span rather than exporting it unscrubbed', error) + this._recordDrop(1, 'beforeSpanSend failed') + return null + } + } + + /** + * Restores the fields a hook must not change. Runs per hook so a later hook in + * the chain cannot sample on an id an earlier one forged. + */ + private _keepSpanIdentity(hooked: SpanRecord, original: SpanIdentity): SpanRecord { + if ( + hooked.traceId !== original.traceId || + hooked.spanId !== original.spanId || + hooked.parentSpanId !== original.parentSpanId + ) { + this._logger.debug('beforeSpanSend changed a span identity field; keeping the original ids') + } + // Only the fields that actually differ are written back. Assigning a value + // to a frozen property throws even when it is the value already there, and + // a hook that freezes the record it returns would otherwise drop every span. + restoreField(hooked, 'traceId', original.traceId) + restoreField(hooked, 'spanId', original.spanId) + restoreField(hooked, 'parentSpanId', original.parentSpanId) + // A hook that rebuilds the record instead of spreading it would otherwise + // drop tracestate, which is not part of the record the hook is handed. + restoreField(hooked, 'traceState', original.traceState) + return hooked + } + private _recordDrop(count: number, reason: string): void { this._droppedSinceWarning += count this._dropReasons.add(reason) @@ -457,10 +659,11 @@ export class PostHogTraces { return discarded } - const resourceAttributes = buildTracesResourceAttributes( - this._config, - this._instance.getLibraryId(), - this._instance.getLibraryVersion() + // Bounded like span attributes: resource attributes are caller-supplied too, + // and they ride on every batch rather than on one span. + const resourceAttributes = truncateAttributes( + buildTracesResourceAttributes(this._config, this._instance.getLibraryId(), this._instance.getLibraryVersion()), + this._config.maxAttributeValueLength ) const scopeName = this._instance.getLibraryId() const scopeVersion = this._instance.getLibraryVersion() diff --git a/packages/core/src/traces/live-spans.spec.ts b/packages/core/src/traces/live-spans.spec.ts index d4b780e7fe..e752072a7a 100644 --- a/packages/core/src/traces/live-spans.spec.ts +++ b/packages/core/src/traces/live-spans.spec.ts @@ -20,6 +20,10 @@ describe('live spans', () => { maxQueueSize: 2048, maxLiveSpans: 10000, maxSpanAgeMs: 3600000, + beforeSpanSend: [], + maxAttributesPerSpan: 128, + maxEventsPerSpan: 128, + maxAttributeValueLength: 8192, } const createTraces = (): PostHogTraces => diff --git a/packages/core/src/traces/otlp.spec.ts b/packages/core/src/traces/otlp.spec.ts index 798cf33242..f25f57280d 100644 --- a/packages/core/src/traces/otlp.spec.ts +++ b/packages/core/src/traces/otlp.spec.ts @@ -118,6 +118,10 @@ describe('OTLP span encoding', () => { flushIntervalMs: 5000, maxExportBatchSize: 512, maxQueueSize: 2048, + beforeSpanSend: [], + maxAttributesPerSpan: 128, + maxEventsPerSpan: 128, + maxAttributeValueLength: 8192, maxLiveSpans: 10000, maxSpanAgeMs: 3600000, ...partial, diff --git a/packages/core/src/traces/otlp.ts b/packages/core/src/traces/otlp.ts index 75d66721c1..b010c08752 100644 --- a/packages/core/src/traces/otlp.ts +++ b/packages/core/src/traces/otlp.ts @@ -9,6 +9,7 @@ import type { } from '@posthog/types' import type { Logger } from '../types' import type { ResolvedTracesConfig, SpanRecord } from './types' +import { nonNegativeCount } from './span' import { toOtlpKeyValueList } from '../utils/otlp-any-value' import { UNSERIALIZABLE_VALUE, sanitizeString } from '../utils/json-utils' import { buildOtlpResourceAttributes } from '../utils/otlp-resource' @@ -112,6 +113,16 @@ export function buildOtlpSpan(record: SpanRecord, logger?: Logger): OtlpSpan { if (record.events.length) { span.events = record.events.map((event) => toOtlpEvent(event, logger)) } + // Coerced: a `beforeSpanSend` hook can write anything onto the record, and a + // non-integer here is refused for the whole request. + const droppedAttributes = nonNegativeCount(record.droppedAttributesCount) + if (droppedAttributes) { + span.droppedAttributesCount = droppedAttributes + } + const droppedEvents = nonNegativeCount(record.droppedEventsCount) + if (droppedEvents) { + span.droppedEventsCount = droppedEvents + } if (record.status) { span.status = { code: SPAN_STATUS_TO_OTLP[record.status.code], diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index c980c5f423..8997614802 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -21,6 +21,10 @@ describe('PostHogSpan', () => { attributes: {}, startTime: Date.now(), backdated: false, + autoAttributeKeys: [], + maxAttributes: 128, + maxEvents: 128, + maxAttributeValueLength: 8192, ...init, }, (record) => ended.push(record), @@ -166,6 +170,76 @@ describe('PostHogSpan', () => { expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('OK')) }) + describe('exception events past the event cap', () => { + const fillEvents = (span: PostHogSpan, count: number): void => { + for (let i = 0; i < count; i++) { + span.addEvent(`step-${i}`) + } + } + + it('records an exception on a span that has filled its events', () => { + // Without its own budget the exception event arrives last, hits the cap and + // is dropped, leaving a span marked `error` with no record of why. + const span = createSpan({ maxEvents: 2 }) + fillEvents(span, 3) + span.recordException(new Error('boom')) + span.end() + + expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1', 'exception']) + expect(ended[0].events[2].attributes?.['exception.message']).toBe('boom') + expect(ended[0].status).toEqual({ code: 'error', message: 'boom' }) + expect(ended[0].droppedEventsCount).toBe(1) + }) + + it('spends an ordinary slot on an exception while the cap has room', () => { + // The reserve is a fallback past the cap, not a smaller budget exceptions + // are confined to: with room to spare, exceptions are ordinary events. + const span = createSpan({ maxEvents: 128 }) + for (let i = 0; i < 20; i++) { + span.recordException(new Error(`boom-${i}`)) + } + span.end() + + expect(ended[0].events).toHaveLength(20) + expect(ended[0].droppedEventsCount).toBeUndefined() + }) + + it('bounds the reserve so recordException cannot grow a span without limit', () => { + const span = createSpan({ maxEvents: 1 }) + for (let i = 0; i < 7; i++) { + span.recordException(new Error(`boom-${i}`)) + } + span.end() + + // One ordinary slot, then the reserve of four. + expect(ended[0].events).toHaveLength(5) + expect(ended[0].droppedEventsCount).toBe(2) + }) + + it('does not let the reserve rescue ordinary events', () => { + const span = createSpan({ maxEvents: 2 }) + fillEvents(span, 5) + span.end() + + expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1']) + expect(ended[0].droppedEventsCount).toBe(3) + }) + }) + + describe('attribute store hygiene', () => { + it('does not copy a polluted Object.prototype key into the span', () => { + ;(Object.prototype as any).polluted = 'yes' + try { + const span = createSpan({ attributes: { real: 1 } }) + span.end() + + expect(Object.keys(ended[0].attributes)).toEqual(['real']) + } finally { + delete (Object.prototype as any).polluted + } + }) + }) + describe('poison attributes', () => { const withThrowingGetter = (): any => { const attributes: any = { ok: 1 } @@ -224,10 +298,271 @@ describe('PostHogSpan', () => { expect(ended[0].events).toEqual([ expect.objectContaining({ name: 'exception', - attributes: { 'exception.type': 'TypeError', 'exception.message': 'boom' }, + attributes: expect.objectContaining({ 'exception.type': 'TypeError', 'exception.message': 'boom' }), }), ]) }) + + it('attaches the stack as exception.stacktrace', () => { + const span = createSpan() + span.recordException(new TypeError('boom')) + span.end() + + const stack = ended[0].events[0].attributes?.['exception.stacktrace'] + expect(stack).toEqual(expect.stringContaining('TypeError: boom')) + }) + + it('bounds the stack by maxAttributeValueLength', () => { + const span = createSpan({ maxAttributeValueLength: 40 }) + const error = new Error('boom') + error.stack = `Error: boom\n${' at somewhere deep\n'.repeat(500)}` + + span.recordException(error) + span.end() + + expect(ended[0].events[0].attributes?.['exception.stacktrace']).toHaveLength(40) + }) + + it('records an exception with no stack without inventing one', () => { + const span = createSpan() + span.recordException('just a string') + span.end() + + expect(ended[0].events[0].attributes).not.toHaveProperty('exception.stacktrace') + }) + }) + + describe('maxAttributeValueLength', () => { + it('truncates a long string attribute', () => { + const span = createSpan({ maxAttributeValueLength: 10 }) + span.setAttribute('payload', 'x'.repeat(5000)) + span.end() + + expect(ended[0].attributes.payload).toBe('xxxxxxxxxx') + }) + + it('truncates the strings inside an array attribute, and leaves other types alone', () => { + const span = createSpan({ maxAttributeValueLength: 3 }) + span.setAttributes({ tags: ['abcdef', 'ab'], count: 1234567, flag: true }) + span.end() + + expect(ended[0].attributes).toMatchObject({ tags: ['abc', 'ab'], count: 1234567, flag: true }) + }) + + it('truncates strings nested inside an object value', () => { + // `setAttribute('payload', { body: res.body })` is the natural way to attach + // a response, and an unbounded one is what pushes a span past the endpoint. + const span = createSpan({ maxAttributeValueLength: 4 }) + span.setAttribute('payload', { body: 'abcdefgh', status: 200 }) + span.end() + + expect(ended[0].attributes.payload).toEqual({ body: 'abcd', status: 200 }) + }) + + it('truncates strings nested inside an array value', () => { + const span = createSpan({ maxAttributeValueLength: 4 }) + span.setAttribute('rows', [{ body: 'abcdefgh' }, ['abcdefgh']]) + span.end() + + expect(ended[0].attributes.rows).toEqual([{ body: 'abcd' }, ['abcd']]) + }) + + it('terminates on a self-referencing value', () => { + const cyclic: any = { body: 'abcdefgh' } + cyclic.self = cyclic + const span = createSpan({ maxAttributeValueLength: 4 }) + + expect(() => { + span.setAttribute('payload', cyclic) + span.end() + }).not.toThrow() + }) + + it('charges a throwing accessor to its own key, still bounding its siblings', () => { + // A lazy ORM relation next to a large field is the shape that matters: if + // the throw abandons the whole walk, the large field ships at full length. + const span = createSpan({ maxAttributeValueLength: 4 }) + const hostile = { + ok: 'abcdefgh', + get boom() { + throw new Error('getter exploded') + }, + } + + expect(() => { + span.setAttribute('payload', hostile as any) + span.end() + }).not.toThrow() + + const payload = ended[0].attributes.payload as Record + expect(payload.ok).toBe('abcd') + expect(payload.boom).toBe('[Unserializable]') + }) + + it('does not walk into a value whose toJSON redacts it', () => { + // Copying the object's own keys would hand the encoder a plain object it + // no longer recognises as self-describing, putting the internals of a + // value that redacts itself on the wire. + class Redacted { + constructor(public secret: string) {} + toJSON(): null { + return null + } + } + const span = createSpan({ maxAttributeValueLength: 10 }) + + span.setAttribute('payload', { inner: new Redacted('S'.repeat(50)) } as any) + span.end() + + expect((ended[0].attributes.payload as any).inner).toBeInstanceOf(Redacted) + }) + + it('keeps a toJSON that resolves to nothing as the object that defines it', () => { + // Replacing it would spend a cap slot on an attribute encoding to nothing, + // and would drop an invalid Date the encoder still describes. + const ghost = { toJSON: () => undefined } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('ghost', ghost as any) + span.end() + + expect(ended[0].attributes.ghost).toBe(ghost) + }) + + it('bounds event attributes too', () => { + const span = createSpan({ maxAttributeValueLength: 4 }) + span.addEvent('cache.miss', { key: 'abcdefgh' }) + span.end() + + expect(ended[0].events[0].attributes).toEqual({ key: 'abcd' }) + }) + + it('walks a value that points at itself twice only once', () => { + // Depth alone is not a bound. Two back-references at each level cost + // 2 ** 20 visits, which is a quarter-second inside the caller's own + // `setAttribute` call, and a third reference is minutes. + let reads = 0 + const cyclic: any = { + get body() { + reads++ + return 'abcdefgh' + }, + } + cyclic.self1 = cyclic + cyclic.self2 = cyclic + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('payload', cyclic) + span.end() + + expect(reads).toBe(1) + }) + + it('bounds the nodes an acyclic value costs, not just its depth', () => { + // Siblings sharing a subtree are not a cycle, so the ancestor set does not + // catch them: 3 ** 12 visits without a node budget. + let reads = 0 + let level: any = { + get body() { + reads++ + return 'abcdefgh' + }, + } + for (let i = 0; i < 12; i++) { + level = { a: level, b: level, c: level } + } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('payload', level) + span.end() + + expect(reads).toBeLessThanOrEqual(10_000) + }) + + it('bounds the value a toJSON produces, which is what the encoder puts on the wire', () => { + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('doc', { toJSON: () => 'abcdefgh' } as any) + span.end() + + expect(ended[0].attributes.doc).toBe('abcd') + }) + + it('resolves toJSON exactly once, so a second call cannot dodge the bound', () => { + // Returning the original object when nothing needed shortening left the + // encoder to call toJSON again — a value that answered differently the + // second time reached the wire unbounded. + let calls = 0 + const doc = { + toJSON: () => { + calls++ + return calls === 1 ? 'ab' : 'x'.repeat(4000) + }, + } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('doc', doc as any) + span.end() + + expect(calls).toBe(1) + expect(ended[0].attributes.doc).toBe('ab') + }) + + it('bounds a string that follows a large collection', () => { + // The traversal budget is spent on containers, not leaves: a big array + // used to exhaust it and leave every later string at full length. + const span = createSpan({ maxAttributeValueLength: 8 }) + + span.setAttribute('payload', { + rows: Array.from({ length: 20000 }, (_, index) => index), + html: 'X'.repeat(50000), + }) + span.end() + + expect((ended[0].attributes.payload as any).html).toHaveLength(8) + }) + + it('keeps a nested __proto__ key as an ordinary entry', () => { + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('payload', JSON.parse('{"__proto__": {"body": "abcdefgh"}}')) + span.end() + + const payload = ended[0].attributes.payload as Record + expect(Object.keys(payload)).toEqual(['__proto__']) + expect(Object.getOwnPropertyDescriptor(payload, '__proto__')?.value).toEqual({ body: 'abcd' }) + }) + + it('bounds a status message', () => { + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setStatus('error', 'abcdefgh') + span.end() + + expect(ended[0].status).toEqual({ code: 'error', message: 'abcd' }) + }) + + it('bounds the status message recordException sets, like the event attribute', () => { + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.recordException(new Error('abcdefgh')) + span.end() + + expect(ended[0].status?.message).toBe('abcd') + expect(ended[0].events[0].attributes?.['exception.message']).toBe('abcd') + }) + + it('bounds an SDK-attached value, which is exempt from the count cap only', () => { + const span = createSpan({ + maxAttributeValueLength: 4, + attributes: { posthogDistinctId: 'user-12345' }, + autoAttributeKeys: ['posthogDistinctId'], + }) + span.setAttribute('posthogDistinctId', 'user-12345') + span.end() + + expect(ended[0].attributes.posthogDistinctId).toBe('user') + }) }) describe('timestamps', () => { @@ -339,6 +674,61 @@ describe('NoopSpan', () => { }) }) +describe('attribute store', () => { + it('hands out a record whose attributes behave like an ordinary object', () => { + const ended: SpanRecord[] = [] + const span = new PostHogSpan( + { + traceId: TRACE_ID, + spanId: SPAN_ID, + name: 'checkout', + kind: 'internal', + attributes: { plan: 'pro' }, + startTime: Date.now(), + backdated: false, + autoAttributeKeys: [], + maxAttributes: 128, + maxEvents: 128, + maxAttributeValueLength: 8192, + }, + (record) => ended.push(record) + ) + span.end() + + const { attributes } = ended[0] + expect(Object.getPrototypeOf(attributes)).toBe(Object.prototype) + expect(Object.prototype.hasOwnProperty.call(attributes, 'plan')).toBe(true) + expect(() => JSON.stringify(attributes)).not.toThrow() + }) + + it('keeps a parsed __proto__ key as an ordinary attribute', () => { + const ended: SpanRecord[] = [] + const span = new PostHogSpan( + { + traceId: TRACE_ID, + spanId: SPAN_ID, + name: 'checkout', + kind: 'internal', + attributes: JSON.parse('{"__proto__": {"leaked": 1}, "orderId": "abc"}'), + startTime: Date.now(), + backdated: false, + autoAttributeKeys: [], + maxAttributes: 128, + maxEvents: 128, + maxAttributeValueLength: 8192, + }, + (record) => ended.push(record) + ) + span.end() + + const keys: string[] = [] + for (const key in ended[0].attributes) { + keys.push(key) + } + expect(keys).not.toContain('leaked') + }) +}) + describe('describeError', () => { it.each([ ['an Error', new Error('boom'), { type: 'Error', message: 'boom' }], @@ -347,7 +737,23 @@ describe('describeError', () => { ['an object with a message', { name: 'CustomError', message: 'oops' }, { type: 'CustomError', message: 'oops' }], ['an object without a name', { message: 'oops' }, { type: 'Object', message: 'oops' }], ])('describes %s', (_name, error, expected) => { - expect(describeError(error)).toEqual(expected) + expect(describeError(error)).toMatchObject(expected) + }) + + it('carries the stack where the thrown value has one, and nothing where it does not', () => { + expect(describeError(new Error('boom')).stack).toEqual(expect.stringContaining('Error: boom')) + expect(describeError('just a string').stack).toBeUndefined() + expect(describeError({ message: 'oops' }).stack).toBeUndefined() + }) + + it('survives a throwing stack accessor', () => { + const hostile = { + message: 'oops', + get stack() { + throw new Error('nope') + }, + } + expect(describeError(hostile)).toEqual({ type: 'Object', message: 'oops' }) }) it('describes a thrown primitive', () => { diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 92828f7037..a75ae1c0a4 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -3,7 +3,13 @@ import type { Logger } from '../types' import type { SpanEventRecord, SpanRecord } from './types' import { formatTraceparent, normalizeTraceparent, sanitizeTracestate } from './traceparent' import { assignUserAttributes, clampEndTime, resolveSuppliedTime, sanitizeName } from './sanitize' -import { isError } from '../utils' +import { isArray, isError, isNullish } from '../utils' +import { + MAX_JSON_SAFE_VALUE_DEPTH, + MAX_JSON_SAFE_VALUE_ITEMS, + MAX_JSON_SAFE_VALUE_NODES, + UNSERIALIZABLE_VALUE, +} from '../utils/json-utils' /** * A monotonic millisecond reading where the platform has one, so an NTP @@ -26,6 +32,11 @@ export interface SpanInit { startTime: number /** True when the caller supplied an explicit `startTime`. */ backdated: boolean + /** Keys the SDK attached itself. Exempt from the attribute cap and never evicted. */ + autoAttributeKeys: string[] + maxAttributes: number + maxEvents: number + maxAttributeValueLength: number } export class PostHogSpan implements Span { @@ -43,10 +54,19 @@ export class PostHogSpan implements Span { private _events: SpanEventRecord[] = [] private _status?: { code: SpanStatusCode; message?: string } private _ended = false + private readonly _autoKeys: Set + private readonly _maxAttributes: number + private readonly _maxEvents: number + private readonly _maxAttributeValueLength: number + private _userAttributeCount = 0 + private _userEventCount = 0 + private _exceptionEventCount = 0 + private _droppedAttributes = 0 + private _droppedEvents = 0 constructor( init: SpanInit, - private readonly _onEnd: (record: SpanRecord) => void, + private readonly _onEnd: (record: SpanRecord, autoKeys: ReadonlySet) => void, private readonly _logger?: Logger ) { this._traceId = init.traceId @@ -55,7 +75,19 @@ export class PostHogSpan implements Span { this._traceState = init.traceState this._name = init.name this._kind = init.kind - this._attributes = init.attributes + this._autoKeys = new Set(init.autoAttributeKeys) + this._maxAttributes = init.maxAttributes + this._maxEvents = init.maxEvents + this._maxAttributeValueLength = init.maxAttributeValueLength + // Null-prototype: a `__proto__` key would otherwise swap this object's prototype + // instead of becoming an entry, and `toString` and friends would read as + // already-present. + this._attributes = Object.create(null) as SpanAttributes + // Object.keys, not for...in: the latter walks the prototype chain, so a + // polluted `Object.prototype` key would become an attribute of every span. + for (const key of Object.keys(init.attributes)) { + this._writeAttribute(key, init.attributes[key]) + } this._startTime = init.startTime this._startMono = init.backdated ? undefined : monotonicNow() } @@ -83,27 +115,89 @@ export class PostHogSpan implements Span { return true } + /** + * Writes an attribute unless the span is already at its user-attribute cap. + * + * Overwriting a key already on the span always succeeds — the cap counts + * distinct user keys, not writes — and SDK-attached keys never count toward + * it, so a span at the cap still carries its person and session ids. + */ + private _writeAttribute(key: string, value: SpanAttributeValue): void { + // Nullish removes the key rather than occupying it: storing one would spend no + // budget and make every later write to that key free, exceeding the cap. + if (isNullish(value)) { + if (key in this._attributes && !this._autoKeys.has(key)) { + this._userAttributeCount-- + } + delete this._attributes[key] + return + } + // The cap is checked before the value is bounded: walking a value the span is + // about to drop is the dominant cost of a span that overflows its cap. + if (!this._autoKeys.has(key) && !(key in this._attributes)) { + if (this._userAttributeCount >= this._maxAttributes) { + this._droppedAttributes++ + return + } + this._userAttributeCount++ + } + this._attributes[key] = truncateAttributeValue(value, this._maxAttributeValueLength) + } + setAttribute(key: string, value: SpanAttributeValue): this { if (this._mutable('setAttribute')) { - Object.defineProperty(this._attributes, key, { value, enumerable: true, writable: true, configurable: true }) + this._writeAttribute(key, value) } return this } setAttributes(attributes: SpanAttributes): this { if (this._mutable('setAttributes')) { - assignUserAttributes(this._attributes, attributes) + // Read through the shared guard first — own enumerable keys only, and a + // throwing getter costs its own key — then write each through the cap. + const safe: SpanAttributes = assignUserAttributes({}, attributes) + for (const key of Object.keys(safe)) { + this._writeAttribute(key, safe[key]) + } } return this } + /** + * Reserves a slot for an event, or refuses when the span is full. + * + * Every event takes an ordinary slot while the cap has room, exception events + * included — the reserve is what an exception falls back on once the cap is + * spent, not a smaller budget it is confined to. + */ + private _claimEventSlot(name: string): boolean { + if (this._userEventCount < this._maxEvents) { + this._userEventCount++ + return true + } + if (isExceptionEvent(name) && this._exceptionEventCount < MAX_EXCEPTION_EVENTS_PER_SPAN) { + this._exceptionEventCount++ + return true + } + return false + } + addEvent(name: string, attributes?: SpanAttributes, timestamp?: SpanTimeInput): this { if (this._mutable('addEvent')) { + // Sanitised before the bucket check, so the name deciding the bucket is the + // one that ends up on the record. + const eventName = sanitizeName(name, 'Span event name', this._logger) + if (!this._claimEventSlot(eventName)) { + this._droppedEvents++ + return this + } this._events.push({ - name: sanitizeName(name, 'Span event name', this._logger), + name: eventName, timestamp: resolveSuppliedTime(timestamp, this._now(), 'event timestamp', this._logger), // Copied so a caller reusing one object across events can't mutate a recorded one. - ...(attributes && { attributes: assignUserAttributes({}, attributes) }), + ...(attributes && { + attributes: truncateAttributes(assignUserAttributes({}, attributes), this._maxAttributeValueLength), + }), }) } return this @@ -115,7 +209,12 @@ export class PostHogSpan implements Span { this._logger?.debug(`Ignoring unknown span status "${String(status)}"; expected "ok" or "error"`) return this } - this._status = { code: status, ...(message && { message }) } + // Bounded like an attribute value: a status message is one more string the + // caller controls, and one large enough takes the span past the body limit. + this._status = { + code: status, + ...(message && { message: truncateString(message, this._maxAttributeValueLength) }), + } } return this } @@ -129,10 +228,11 @@ export class PostHogSpan implements Span { if (!this._mutable('recordException')) { return this } - const { type, message } = describeError(error) - this.addEvent('exception', { + const { type, message, stack } = describeError(error) + this.addEvent(EXCEPTION_EVENT_NAME, { 'exception.type': type, 'exception.message': message, + ...(stack && { 'exception.stacktrace': stack }), }) // recordException is itself an explicit call, so it follows last-write-wins // rather than deferring to an earlier `ok`. @@ -169,20 +269,148 @@ export class PostHogSpan implements Span { const derived = this._now() const resolved = resolveSuppliedTime(endTime, derived, 'end time', this._logger) - this._onEnd({ - traceId: this._traceId, - spanId: this._spanId, - ...(this._parentSpanId && { parentSpanId: this._parentSpanId }), - ...(this._traceState && { traceState: this._traceState }), - name: this._name, - kind: this._kind, - ...(this._status && { status: this._status }), - attributes: this._attributes, - events: this._events, - startTime: this._startTime, - endTime: clampEndTime(resolved, this._startTime), + this._onEnd( + { + traceId: this._traceId, + spanId: this._spanId, + ...(this._parentSpanId && { parentSpanId: this._parentSpanId }), + ...(this._traceState && { traceState: this._traceState }), + name: this._name, + kind: this._kind, + ...(this._status && { status: this._status }), + // Copied out with an ordinary prototype: the store is null-prototype, but a + // record handed to user code should behave like a normal object. + attributes: { ...this._attributes }, + events: this._events, + startTime: this._startTime, + endTime: clampEndTime(resolved, this._startTime), + ...(this._droppedAttributes && { droppedAttributesCount: this._droppedAttributes }), + ...(this._droppedEvents && { droppedEventsCount: this._droppedEvents }), + }, + this._autoKeys + ) + } +} + +const EXCEPTION_EVENT_NAME = 'exception' + +/** + * How many `exception` events may sit past the event cap. + * + * A span that fills its events and then throws would otherwise lose the only + * record of why it failed — the span you most want to read. Fixed and small + * rather than configurable: this is a safety margin, not a tuning knob, and + * four covers a catch-retry-fail loop without letting `recordException` grow a + * span without limit. + */ +const MAX_EXCEPTION_EVENTS_PER_SPAN = 4 + +/** + * Identified by name, the same trade-off the attribute exemption makes: a + * caller who names their own event `exception` gets the exemption too. + */ +function isExceptionEvent(name: string): boolean { + return name === EXCEPTION_EVENT_NAME +} + +/** A value as its string form, or the encoder's marker when it refuses to produce one. */ +function safeString(value: unknown): string { + try { + return typeof value === 'string' ? value : String(value) + } catch { + return UNSERIALIZABLE_VALUE + } +} + +/** A caller-visible counter read back as a number, or 0 for anything else. */ +export function nonNegativeCount(value: unknown): number { + return typeof value === 'number' && Number.isFinite(value) && value > 0 ? Math.floor(value) : 0 +} + +/** + * Re-applies the per-span caps to a record a `beforeSpanSend` hook has already + * seen. The hook writes to the plain record, not through the span's own guarded + * writer, so an enriching hook would otherwise push a span past the cap it was + * trimmed to and back into the 413 path the cap exists to avoid. + * + * Earliest-set entries win, matching the span-side rule; SDK-attached keys are + * exempt. Counts add to whatever the span already dropped. + */ +export function applySpanLimits( + record: SpanRecord, + autoKeys: ReadonlySet, + maxAttributes: number, + maxEvents: number, + maxAttributeValueLength: number +): void { + let kept = 0 + let droppedAttributes = 0 + // Built fresh rather than edited in place: a hook is free to return a record + // whose attributes it froze, and a `delete` on one throws. + const attributes: SpanAttributes = {} + for (const key of Object.keys(record.attributes)) { + const value = record.attributes[key] + // Matches `_writeAttribute`: the encoder drops these, so a hook that blanks a + // value rather than deleting the key must not evict a real attribute. + if (isNullish(value)) { + continue + } + if (!autoKeys.has(key)) { + if (kept >= maxAttributes) { + droppedAttributes++ + continue + } + kept++ + } + Object.defineProperty(attributes, key, { + value: truncateAttributeValue(value, maxAttributeValueLength), + enumerable: true, + writable: true, + configurable: true, }) } + record.attributes = attributes + if (droppedAttributes) { + // Coerced, not trusted: a hook can put anything in the counter, and a + // non-number there would erase the count the span itself accumulated. + record.droppedAttributesCount = nonNegativeCount(record.droppedAttributesCount) + droppedAttributes + } + + // Walked in order rather than sliced: an exception event is the last thing on + // a span that threw, so a plain slice would cut off the exemption the writer + // just granted. A hook can also append events or rewrite their attributes, + // neither of which goes through `addEvent`. + let keptEvents = 0 + let keptExceptions = 0 + let droppedEvents = 0 + const events: SpanEventRecord[] = [] + for (const event of record.events) { + if (keptEvents < maxEvents) { + keptEvents++ + } else if (isExceptionEvent(event.name) && keptExceptions < MAX_EXCEPTION_EVENTS_PER_SPAN) { + keptExceptions++ + } else { + droppedEvents++ + continue + } + if (event.attributes) { + event.attributes = truncateAttributes({ ...event.attributes }, maxAttributeValueLength) + } + events.push(event) + } + record.events = events + if (droppedEvents) { + record.droppedEventsCount = nonNegativeCount(record.droppedEventsCount) + droppedEvents + } + if (record.status?.message) { + // Coerced first: a non-string would reach the encoder to be stringified at + // full length. Guarded, because a throwing `toString` here would cost the + // span, where the encoder downstream only marks the field. + record.status = { + ...record.status, + message: truncateString(safeString(record.status.message), maxAttributeValueLength), + } + } } /** @@ -247,6 +475,21 @@ export class PassThroughSpan extends NoopSpan { } } +/** + * The `stack` of whatever was thrown, as OTel's `exception.stacktrace`. Reads + * the property behind its own guard: a getter on a hostile object throws, and a + * thrown string has no stack at all. The value is bounded like any other + * attribute, by `maxAttributeValueLength`. + */ +function readStack(error: unknown): { stack?: string } { + try { + const stack = (error as { stack?: unknown }).stack + return typeof stack === 'string' && stack ? { stack } : {} + } catch { + return {} + } +} + /** * The handle to return when a span cannot be recorded: a pass-through when the * caller supplied a usable `parent` header, the shared no-op otherwise. @@ -259,14 +502,161 @@ export function inertSpan(options?: { parent?: unknown; tracestate?: unknown }): return new PassThroughSpan(traceparent, sanitizeTracestate(options?.tracestate)) } +/** Mirrors `encodeAnyValue`'s traversal budget so the two walks cost the same. */ +interface TruncateState { + /** Containers on the current path, so a back-reference stops the walk. */ + ancestors: WeakSet + remainingNodes: number +} + +/** + * Bounds every string reachable from an attribute value to `maxLength` + * characters, including the strings nested inside arrays and objects. Numbers + * and booleans are bounded already. + * + * An unbounded value is the one thing the per-span caps do not stop: a single + * multi-MB attribute makes the whole span too large for the ingestion endpoint, + * and the 413 path then drops that span whole. `setAttribute('payload', { body })` + * is the usual way one arrives, so the bound has to reach inside the value. + * + * Returns the value it was given when nothing needed shortening, so the common + * case allocates nothing. + */ +function truncateString(value: string, maxLength: number): string { + return value.length > maxLength ? value.slice(0, maxLength) : value +} + +export function truncateAttributeValue(value: SpanAttributeValue, maxLength: number): SpanAttributeValue { + return truncateValue(value, maxLength, { ancestors: new WeakSet(), remainingNodes: MAX_JSON_SAFE_VALUE_NODES }, 0) +} + +/** + * Walks under the same depth cap, node budget and ancestor set as + * `encodeAnyValue`. Depth alone does not bound this: a value whose children + * point back at their siblings costs `fanout ** depth` visits, which is minutes + * of synchronous work inside the caller's own `setAttribute` call. + * + * A value the walk cannot bound is returned as it came. The encoder walks it + * again under its own budget and marks whatever it finds there. + */ +function truncateValue( + value: SpanAttributeValue, + maxLength: number, + state: TruncateState, + depth: number +): SpanAttributeValue { + // A string is a leaf and costs no traversal, so it is bounded before the + // budget is consulted. Charging it would let one large collection spend the + // budget and leave every string after it on the wire at full length. + if (typeof value === 'string') { + return truncateString(value, maxLength) + } + if (value === null || typeof value !== 'object') { + return value + } + if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH || state.ancestors.has(value)) { + return value + } + state.remainingNodes-- + state.ancestors.add(value) + try { + // The representation the value defines for itself is what the encoder puts + // on the wire, so it is what has to be bounded — a `toJSON` returning a + // megabyte of text is invisible to a walk over the object's own keys. + const resolved = resolveToJson(value) + if (resolved.selfDescribed) { + // Resolving to nothing is the value's answer. Walking its keys anyway + // would build a plain object the encoder no longer treats as + // self-describing, putting the internals of a redacted value on the wire. + return isNullish(resolved.value) ? value : truncateValue(resolved.value, maxLength, state, depth + 1) + } + let changed = false + if (isArray(value)) { + // Only the items the encoder will emit are walked; it stops at the same + // cap, so bounding the rest is work spent on values that never ship. + const walked = Math.min(value.length, MAX_JSON_SAFE_VALUE_ITEMS) + let boundedItems: SpanAttributeValue[] | undefined + for (let index = 0; index < walked; index++) { + const item = value[index] + const boundedItem = truncateValue(item, maxLength, state, depth + 1) + if (boundedItem !== item) { + // Copied lazily, so an array that needed nothing allocates nothing. + boundedItems = boundedItems ?? value.slice() + boundedItems[index] = boundedItem + } + } + return boundedItems ?? value + } + const bounded: SpanAttributes = {} + for (const key of Object.keys(value)) { + let boundedItem: SpanAttributeValue + try { + // Read once: re-reading to compare would run a getter a second time. + const item = (value as SpanAttributes)[key] + boundedItem = truncateValue(item, maxLength, state, depth + 1) + changed = changed || boundedItem !== item + } catch { + // A throwing accessor costs its own key. Reaching the walk's own catch + // would abandon the whole value unbounded, which is how a lazy ORM + // relation next to a large field puts that field on the wire whole. + boundedItem = UNSERIALIZABLE_VALUE + changed = true + } + // defineProperty, not assignment: a nested `__proto__` key would otherwise + // swap the copy's prototype and vanish. + Object.defineProperty(bounded, key, { + value: boundedItem, + enumerable: true, + writable: true, + configurable: true, + }) + } + return changed ? bounded : value + } catch { + // Whatever is left — a hostile `Object.keys`, a `slice` that throws — costs + // this value its bound rather than the span. Per-key reads are guarded + // above, so a single bad property does not reach here. + return value + } finally { + // Siblings pointing at the same object are duplication, not a cycle. + state.ancestors.delete(value) + } +} + +/** + * The value's own serialized form. `selfDescribed` is false when it defines no + * `toJSON`, or when reading one throws — both fall through to the plain walk, + * as they do in the encoder. + */ +function resolveToJson(value: object): { selfDescribed: boolean; value?: SpanAttributeValue } { + try { + const toJSON = (value as { toJSON?: unknown }).toJSON + if (typeof toJSON === 'function') { + return { selfDescribed: true, value: toJSON.call(value) as SpanAttributeValue } + } + } catch { + // Falls through to the plain walk. + } + return { selfDescribed: false } +} + +/** `truncateAttributeValue` across an attribute bag, in place. */ +export function truncateAttributes(attributes: SpanAttributes, maxLength: number): SpanAttributes { + for (const key of Object.keys(attributes)) { + attributes[key] = truncateAttributeValue(attributes[key], maxLength) + } + return attributes +} + /** * Extracts the OTel `exception.type` / `exception.message` pair from whatever was * thrown. Anything can be thrown in JS, so non-Errors are described by type. */ -export function describeError(error: unknown): { type: string; message: string } { +export function describeError(error: unknown): { type: string; message: string; stack?: string } { try { + const stack = readStack(error) if (isError(error)) { - return { type: error.name || 'Error', message: error.message || '' } + return { type: error.name || 'Error', message: error.message || '', ...stack } } if (typeof error === 'string') { return { type: 'string', message: error } @@ -274,7 +664,7 @@ export function describeError(error: unknown): { type: string; message: string } if (error && typeof error === 'object') { const maybe = error as { name?: unknown; message?: unknown } if (typeof maybe.message === 'string') { - return { type: typeof maybe.name === 'string' ? maybe.name : 'Object', message: maybe.message } + return { type: typeof maybe.name === 'string' ? maybe.name : 'Object', message: maybe.message, ...stack } } } return { type: typeof error, message: String(error) } diff --git a/packages/core/src/traces/types.ts b/packages/core/src/traces/types.ts index 6551d0bcac..2852ff62a3 100644 --- a/packages/core/src/traces/types.ts +++ b/packages/core/src/traces/types.ts @@ -7,6 +7,7 @@ export type { SpanTimeInput, StartSpanOptions, TracesConfig, + BeforeSpanSendFn, OtlpSpan, OtlpSpanEvent, OtlpSpanKeyValue, @@ -14,7 +15,15 @@ export type { OtlpTracesPayload, } from '@posthog/types' -import type { OtlpTracesPayload, Span, SpanAttributes, SpanKind, SpanStatusCode, TracesConfig } from '@posthog/types' +import type { + BeforeSpanSendFn, + OtlpTracesPayload, + Span, + SpanAttributes, + SpanKind, + SpanStatusCode, + TracesConfig, +} from '@posthog/types' /** Same tagged outcome shape as `SendLogsBatchOutcome` — one policy for all three signals. */ export type SendTracesBatchOutcome = @@ -72,6 +81,8 @@ export interface SpanRecord { /** ms epoch. */ startTime: number endTime: number + droppedAttributesCount?: number + droppedEventsCount?: number } /** @@ -98,6 +109,10 @@ export interface ResolvedTracesConfig extends TracesConfig { * dropped rather than queued ones, whose children may already have shipped. */ maxQueueSize: number + beforeSpanSend: BeforeSpanSendFn[] + maxAttributesPerSpan: number + maxEventsPerSpan: number + maxAttributeValueLength: number /** Bound on spans started but not yet ended. At the bound `startSpan` returns a no-op handle. */ maxLiveSpans: number /** How long a span may stay live before it stops being accounted for and can never export. */ diff --git a/packages/node/src/__tests__/traces-defaults.spec.ts b/packages/node/src/__tests__/traces-defaults.spec.ts index b767799afa..43d1d33224 100644 --- a/packages/node/src/__tests__/traces-defaults.spec.ts +++ b/packages/node/src/__tests__/traces-defaults.spec.ts @@ -1,16 +1,38 @@ import { resolveTracesConfig } from '../traces-defaults' describe('resolveTracesConfig', () => { + it.each([ + ['zero', 0], + ['negative', -1], + ['not a number', NaN], + ])('falls back to the default per-span caps when given %s', (_label, value) => { + const resolved = resolveTracesConfig({ + maxAttributesPerSpan: value, + maxEventsPerSpan: value, + maxAttributeValueLength: value, + }) + expect(resolved.maxAttributesPerSpan).toBe(128) + expect(resolved.maxEventsPerSpan).toBe(128) + expect(resolved.maxAttributeValueLength).toBe(8192) + }) + it('applies the documented defaults', () => { expect(resolveTracesConfig(undefined)).toMatchObject({ flushIntervalMs: 5000, maxExportBatchSize: 512, maxQueueSize: 2048, + maxAttributesPerSpan: 128, + maxEventsPerSpan: 128, + maxAttributeValueLength: 8192, maxLiveSpans: 10_000, maxSpanAgeMs: 3_600_000, }) }) + it('honours an explicit attribute value bound', () => { + expect(resolveTracesConfig({ maxAttributeValueLength: 256 }).maxAttributeValueLength).toBe(256) + }) + it('honours explicit live-span bounds', () => { expect(resolveTracesConfig({ maxLiveSpans: 50, maxSpanAgeMs: 30_000 })).toMatchObject({ maxLiveSpans: 50, @@ -135,6 +157,21 @@ describe('resourceAttributes guarding', () => { expect(resolved.resourceAttributes).toEqual({ region: 'us' }) }) + it('ignores a beforeSpanSend entry that is not a function', () => { + // A plain-JS caller passing the wrong shape would otherwise have every span + // dropped by a hook that throws on call, with tracing silently off. + const scrub = (span: any): any => span + const resolved = resolveTracesConfig({ beforeSpanSend: ['not a function', scrub] as never }) + + expect(resolved.beforeSpanSend).toEqual([scrub]) + }) + + it('resolves to no hooks when beforeSpanSend is the wrong type entirely', () => { + const resolved = resolveTracesConfig({ beforeSpanSend: { scrub: true } as never }) + + expect(resolved.beforeSpanSend).toEqual([]) + }) + it('does not throw when an identity accessor throws', () => { const hostile = {} Object.defineProperty(hostile, 'service.name', { diff --git a/packages/node/src/__tests__/traces.spec.ts b/packages/node/src/__tests__/traces.spec.ts index dd37b982ee..684e399718 100644 --- a/packages/node/src/__tests__/traces.spec.ts +++ b/packages/node/src/__tests__/traces.spec.ts @@ -31,6 +31,9 @@ describe('PostHog traces', () => { const attributeOf = (span: OtlpSpan, key: string): any => span.attributes?.find((a) => a.key === key)?.value + const attributeOfEvent = (event: NonNullable[number], key: string): any => + event.attributes?.find((a: any) => a.key === key)?.value + // Traces run their own flush cycle; this advances it without calling flush(). const DEFAULT_TRACES_FLUSH_INTERVAL_MS = 5000 const flushTraces = async (): Promise => { @@ -319,6 +322,36 @@ describe('PostHog traces', () => { const [span] = sentSpans() expect(span.status).toEqual({ code: 2, message: 'boom' }) expect(span.events?.[0].name).toBe('exception') + expect(attributeOfEvent(span.events![0], 'exception.stacktrace')).toEqual({ + stringValue: expect.stringContaining('TypeError: boom'), + }) + }) + + it('bounds a stack by maxAttributeValueLength, and beforeSpanSend can scrub it', async () => { + const scrubbed = createClient({ + traces: { + serviceName: 'checkout-api', + maxAttributeValueLength: 64, + beforeSpanSend: (span: any) => { + for (const event of span.events) { + if (event.attributes?.['exception.stacktrace']) { + event.attributes['exception.stacktrace'] = '[redacted]' + } + } + return span + }, + }, + }) + + expect(() => + scrubbed.withSpan('job', () => { + throw new TypeError('boom') + }) + ).toThrow('boom') + await scrubbed.shutdown() + + const [span] = sentSpans() + expect(attributeOfEvent(span.events![0], 'exception.stacktrace')).toEqual({ stringValue: '[redacted]' }) }) }) @@ -491,6 +524,93 @@ describe('PostHog traces', () => { }) }) + describe('beforeSpanSend', () => { + it('scrubs attributes before they leave the process', async () => { + const client = createClient({ + traces: { + serviceName: 'svc', + beforeSpanSend: (span: any) => { + delete span.attributes.password + return span + }, + }, + }) + client.startSpan('login', { attributes: { password: 'hunter2', ok: true } }).end() + await client.shutdown() + + const [span] = sentSpans() + expect(span.attributes?.find((a) => a.key === 'password')).toBeUndefined() + expect(span.attributes?.find((a) => a.key === 'ok')).toBeDefined() + }) + + it('runs an array of hooks through the client option', async () => { + const client = createClient({ + traces: { + serviceName: 'svc', + beforeSpanSend: [ + (span: any) => { + span.attributes.first = true + return span + }, + (span: any) => { + span.attributes.second = true + return span + }, + ], + }, + }) + client.startSpan('checkout').end() + await client.shutdown() + + const keys = sentSpans()[0].attributes?.map((a) => a.key) + expect(keys).toEqual(expect.arrayContaining(['first', 'second'])) + }) + + it('drops a span the hook rejects', async () => { + const client = createClient({ + traces: { + serviceName: 'svc', + beforeSpanSend: (span: any) => (span.attributes['http.route'] === '/health' ? null : span), + }, + }) + client.startSpan('GET /health', { attributes: { 'http.route': '/health' } }).end() + client.startSpan('GET /orders', { attributes: { 'http.route': '/orders' } }).end() + await client.shutdown() + + expect(sentSpans().map((s) => s.name)).toEqual(['GET /orders']) + }) + }) + + describe('span limits', () => { + it('caps attributes and reports how many were dropped', async () => { + const client = createClient({ traces: { serviceName: 'svc', maxAttributesPerSpan: 2 } }) + const span = client.startSpan('checkout') + span.setAttribute('a', 1) + span.setAttribute('b', 2) + span.setAttribute('c', 3) + span.end() + await client.shutdown() + + const [sent] = sentSpans() + expect(sent.attributes?.map((a) => a.key)).toEqual(['a', 'b']) + expect(sent.droppedAttributesCount).toBe(1) + }) + + it('defaults to the OpenTelemetry cap of 128', async () => { + const client = createClient({ traces: { serviceName: 'svc' } }) + const span = client.startSpan('checkout') + for (let i = 0; i < 130; i++) { + span.setAttribute(`key-${i}`, i) + } + span.end() + await client.shutdown() + + const [sent] = sentSpans() + expect(sent.attributes).toHaveLength(128) + expect(sent.droppedAttributesCount).toBe(2) + }) + }) + describe('shutdown', () => { it('is bounded by the shutdown timeout when the transport hangs', async () => { const client = createClient() diff --git a/packages/node/src/exports.ts b/packages/node/src/exports.ts index 23e4897a93..35950883a4 100644 --- a/packages/node/src/exports.ts +++ b/packages/node/src/exports.ts @@ -24,6 +24,8 @@ export type { SpanStatusCode, SpanTimeInput, StartSpanOptions, + SpanRecord, + BeforeSpanSendFn, TracesConfig, } from '@posthog/core' diff --git a/packages/node/src/traces-defaults.ts b/packages/node/src/traces-defaults.ts index f4982505bc..3623d7b45e 100644 --- a/packages/node/src/traces-defaults.ts +++ b/packages/node/src/traces-defaults.ts @@ -1,11 +1,20 @@ import { assignUserAttributes } from '@posthog/core' -import type { ResolvedTracesConfig, TracesConfig } from '@posthog/core' +import type { BeforeSpanSendFn, ResolvedTracesConfig, TracesConfig } from '@posthog/core' // OpenTelemetry's BatchSpanProcessor defaults, which sit comfortably under the // server's 2 MB body cap. const DEFAULT_FLUSH_INTERVAL_MS = 5000 const DEFAULT_MAX_EXPORT_BATCH_SIZE = 512 const DEFAULT_MAX_QUEUE_SIZE = 2048 +// OpenTelemetry's per-span defaults. +const DEFAULT_MAX_ATTRIBUTES_PER_SPAN = 128 +const DEFAULT_MAX_EVENTS_PER_SPAN = 128 +// OpenTelemetry leaves the value length unlimited, which is what lets one +// multi-MB attribute make a span too large for the endpoint to accept — and an +// oversized span is dropped whole. 8 KB holds a deep stack trace and any +// realistic header, query string or payload excerpt, and keeps a span at the +// attribute cap under 1 MB, comfortably inside the 2 MB body cap. +const DEFAULT_MAX_ATTRIBUTE_VALUE_LENGTH = 8192 // Live-span bounds. A server can legitimately hold thousands of spans open at // once, and refusing a legitimate span is worse than tolerating a leak, so the @@ -53,6 +62,18 @@ function withUsableIdentityKeys(attributes: TracesConfig['resourceAttributes']): } } +/** + * Keeps only the callable hooks. Anything else is dropped rather than called: an + * untyped caller passing the wrong shape would otherwise have every span dropped + * by a hook that throws, leaving tracing silently off. + */ +function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend']): BeforeSpanSendFn[] { + if (!beforeSpanSend) { + return [] + } + return [beforeSpanSend].flat().filter((hook): hook is BeforeSpanSendFn => typeof hook === 'function') +} + /** * Resolves the public `traces` config into the shape core `PostHogTraces` consumes. * OTLP resource attributes take precedence over the named fields, matching the @@ -75,6 +96,10 @@ export function resolveTracesConfig( serviceVersion: (resourceAttributes?.['service.version'] as string | undefined) ?? config?.serviceVersion, environment: (resourceAttributes?.['deployment.environment'] as string | undefined) ?? config?.environment, resourceAttributes, + beforeSpanSend: resolveBeforeSpanSend(config?.beforeSpanSend), + maxAttributesPerSpan: positiveInteger(config?.maxAttributesPerSpan, DEFAULT_MAX_ATTRIBUTES_PER_SPAN), + maxEventsPerSpan: positiveInteger(config?.maxEventsPerSpan, DEFAULT_MAX_EVENTS_PER_SPAN), + maxAttributeValueLength: positiveInteger(config?.maxAttributeValueLength, DEFAULT_MAX_ATTRIBUTE_VALUE_LENGTH), flushIntervalMs: positiveInteger(config?.flushIntervalMs, DEFAULT_FLUSH_INTERVAL_MS), maxExportBatchSize, // Never below the flush trigger, or the depth-based flush could never fire. diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 04e2974cce..4d550f07f0 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -138,6 +138,8 @@ export type { SpanTimeInput, StartSpanOptions, Span, + SpanRecord, + BeforeSpanSendFn, TracesConfig, OtlpSpanKeyValue, OtlpSpanEvent, diff --git a/packages/types/src/traces.ts b/packages/types/src/traces.ts index aedc80ee63..d3ce38d6b5 100644 --- a/packages/types/src/traces.ts +++ b/packages/types/src/traces.ts @@ -133,8 +133,10 @@ export interface Span { /** * Record an exception on the span: sets status `error` and attaches an - * `exception` event carrying `exception.type` and `exception.message`. - * Does not end the span. + * `exception` event carrying `exception.type`, `exception.message` and, + * where the thrown value has one, `exception.stacktrace`. The stack is + * truncated to `maxAttributeValueLength` like any other attribute value, + * and `beforeSpanSend` sees it before it is exported. Does not end the span. */ recordException(error: unknown): this @@ -172,6 +174,37 @@ export interface Span { end(endTime?: SpanTimeInput): void } +/** + * A completed span as `beforeSpanSend` sees it: plain values, not the OTLP wire + * encoding — `userId: 42` reads as `42`, not `{ intValue: "42" }`. + * + * @experimental Subject to change in a minor release. + */ +export interface SpanRecord { + /** Assignment is ignored with a debug warning: rewriting ids orphans children that already shipped. */ + readonly traceId: string + readonly spanId: string + readonly parentSpanId?: string + name: string + kind: SpanKind + status?: { code: SpanStatusCode; message?: string } + attributes: SpanAttributes + events: { name: string; timestamp: number; attributes?: SpanAttributes }[] + /** Millisecond epoch. */ + startTime: number + endTime: number +} + +/** + * Inspects, edits or drops a finished span. Return `null` to drop it. + * + * The hook runs synchronously as part of `end()`; a returned promise is not + * awaited and the span is dropped. + * + * @experimental Subject to change in a minor release. + */ +export type BeforeSpanSendFn = (span: SpanRecord) => SpanRecord | null + /** * Configuration for distributed tracing, passed as the `traces` client option. * Tracing stays off until this object is supplied. @@ -237,6 +270,68 @@ export interface TracesConfig { */ maxQueueSize?: number + /** + * Runs on every finished span before it is queued. Edit the span in place, + * or return `null` to drop it. An array runs left to right, and the first + * hook to return `null` stops the chain. + * + * This is the place to scrub sensitive attributes, so a hook that throws + * drops the span rather than exporting an unscrubbed one. + * + * @example Drop health checks and redact a header + * ```ts + * traces: { + * beforeSpanSend: (span) => { + * if (span.attributes['http.route'] === '/health') return null + * delete span.attributes['http.request.header.authorization'] + * return span + * }, + * } + * ``` + */ + beforeSpanSend?: BeforeSpanSendFn | BeforeSpanSendFn[] + + /** + * Maximum user-supplied attributes on a single span. Attributes the SDK + * attaches itself — `posthogDistinctId`, `sessionId` and friends — are + * exempt and are never evicted, because they are what links a span to a + * person and a session. + * + * On overflow the earliest-set attributes are kept and later ones are + * dropped, with the number dropped reported on the exported span. + * + * @default 128 + */ + maxAttributesPerSpan?: number + + /** + * Maximum events on a single span. On overflow the earliest events are kept + * and later ones are dropped, with the number dropped reported on the + * exported span. + * + * Once the cap is spent, a small reserve is still available to `exception` + * events, so a span that fills its events and then throws still carries the + * exception rather than only an `error` status. Below the cap an exception + * is an ordinary event and spends an ordinary slot. + * + * @default 128 + */ + maxEventsPerSpan?: number + + /** + * Maximum length of a string attribute value. Longer values are truncated, + * and the bound reaches every string the value contains, including the ones + * nested inside arrays and objects. It applies to span attributes, event + * attributes, status messages and resource attributes alike — including + * `exception.stacktrace`. + * + * The bound is what keeps one large value from making a span too large for + * the ingestion endpoint, which drops an oversized span whole. + * + * @default 8192 + */ + maxAttributeValueLength?: number + /** * Bound on how many spans may be live (started but not ended) at once. At * the bound `startSpan` returns an inert handle, so code that leaks spans @@ -300,6 +395,10 @@ export interface OtlpSpan { status?: OtlpSpanStatus /** W3C trace flags in the low byte; the sampled bit is always set. */ flags?: number + /** User attributes dropped by `maxAttributesPerSpan`. Omitted when none were. */ + droppedAttributesCount?: number + /** Events dropped by `maxEventsPerSpan`. Omitted when none were. */ + droppedEventsCount?: number } export interface OtlpTracesPayload { From dad18df7ca3ddfd30a62c09a2a5b7e3228694095 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 10:41:58 -0400 Subject: [PATCH 02/22] fix(traces): take the propagation fields from the span, not the hook Restoring traceFlags and parentIsRemote onto a hook's return value throws when the hook freezes it, dropping the span. Read them from the pre-hook snapshot in the rebuild, the same treatment the dropped counts already get. --- packages/core/src/traces/index.spec.ts | 14 +++++++ packages/core/src/traces/index.ts | 57 ++++++++++++++------------ 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 1425ef0640..60072edfd8 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -743,6 +743,20 @@ describe('PostHogTraces', () => { expect(sentSpans(instance)[0].flags).toBe(0x300) }) + it('keeps them when the rebuilding hook also freezes what it returns', async () => { + // Restoring these onto the returned record would throw here, and a throwing + // hook drops the span. + const instance = createMockInstance() + const traces = createTraces( + { beforeSpanSend: [(span: SpanRecord) => Object.freeze({ ...span }) as SpanRecord] }, + instance + ) + traces.startSpan('child', { parent: `00-${TRACE_ID}-${REMOTE_SPAN_ID}-00` }).end() + await traces.flush() + + expect(sentSpans(instance).map((s) => s.flags)).toEqual([0x300]) + }) + it('runs hooks left to right and stops at the first null', async () => { const order: string[] = [] await endOneSpan([ diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 9f9ba3ad7a..b969e57466 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -1,4 +1,4 @@ -import type { Span, SpanAttributes, StartSpanOptions } from '@posthog/types' +import type { Span, SpanAttributes, SpanRecord as HookSpanRecord, StartSpanOptions } from '@posthog/types' import type { Logger } from '../types' import type { OtlpSpan, @@ -70,8 +70,6 @@ interface SpanIdentity { spanId: string parentSpanId?: string traceState?: string - traceFlags: string - parentIsRemote: boolean } /** @@ -489,8 +487,6 @@ export class PostHogTraces { spanId: record.spanId, parentSpanId: record.parentSpanId, traceState: record.traceState, - traceFlags: record.traceFlags, - parentIsRemote: record.parentIsRemote, } const originalTimes = { startTime: record.startTime, endTime: record.endTime } // Snapshotted with the rest: the hook mutates the record in place, so reading @@ -499,20 +495,26 @@ export class PostHogTraces { attributes: record.droppedAttributesCount, events: record.droppedEventsCount, } + // Read here rather than restored onto the hook's return value: writing them + // back would throw on a frozen record, and neither is on the record a hook + // is handed, so a rebuilding hook always arrives without them. + const originalPropagation = { + traceFlags: record.traceFlags, + parentIsRemote: record.parentIsRemote, + } // Copied, not referenced: the hook is documented as mutating the record in // place, and a reference would restore the mutation onto itself. const originalStatus = record.status && { ...record.status } + let hooked: HookSpanRecord = record let current = record try { for (const hook of this._config.beforeSpanSend) { - const result = hook(current) + const result = hook(hooked) if (!result) { this._recordDrop(1, 'beforeSpanSend dropped it') return null } - // A hook is handed the public record, which carries neither the trace - // flags nor the parent's remoteness; `_keepSpanIdentity` puts both back. - current = this._keepSpanIdentity(result as SpanRecord, identity) + hooked = this._keepSpanIdentity(result, identity) } // Rebuilt field by field before anything below writes to it. The hook's @@ -520,19 +522,24 @@ export class PostHogTraces { // class instance whose fields are prototype getters a spread would miss. // Naming them also bounds what can reach the wire. current = { - traceId: current.traceId, - spanId: current.spanId, - parentSpanId: current.parentSpanId, - traceState: current.traceState, - traceFlags: current.traceFlags, - parentIsRemote: current.parentIsRemote, - name: current.name, - kind: current.kind, - status: current.status, - attributes: current.attributes, - events: current.events, - startTime: current.startTime, - endTime: current.endTime, + traceId: hooked.traceId, + spanId: hooked.spanId, + parentSpanId: hooked.parentSpanId, + // From the snapshot: `_keepSpanIdentity` has already put it back on the + // record, but no public type declares it, so it cannot be read off one. + traceState: identity.traceState, + name: hooked.name, + kind: hooked.kind, + status: hooked.status, + attributes: hooked.attributes, + events: hooked.events, + startTime: hooked.startTime, + endTime: hooked.endTime, + // Taken from the span for the same reason as the dropped counts: no + // public type declares them, so a rebuilding hook returns without them + // and a `?? fallback` here would export a sampled-out trace as sampled. + traceFlags: originalPropagation.traceFlags, + parentIsRemote: originalPropagation.parentIsRemote, // Taken from the span, not from the hook's return value: these are SDK // bookkeeping that no public type declares, so a hook overwriting them // must not erase what the span actually dropped. @@ -598,7 +605,7 @@ export class PostHogTraces { * Restores the fields a hook must not change. Runs per hook so a later hook in * the chain cannot sample on an id an earlier one forged. */ - private _keepSpanIdentity(hooked: SpanRecord, original: SpanIdentity): SpanRecord { + private _keepSpanIdentity(hooked: HookSpanRecord, original: SpanIdentity): HookSpanRecord { if ( hooked.traceId !== original.traceId || hooked.spanId !== original.spanId || @@ -615,10 +622,6 @@ export class PostHogTraces { // A hook that rebuilds the record instead of spreading it would otherwise // drop tracestate, which is not part of the record the hook is handed. restoreField(hooked, 'traceState', original.traceState) - // Same reasoning: the sampled flag and the parent's remoteness are the - // caller's, and neither is on the record a hook sees. - restoreField(hooked, 'traceFlags', original.traceFlags) - restoreField(hooked, 'parentIsRemote', original.parentIsRemote) return hooked } From 268bb81aea45330099ae8ba3ef0e08e6348773b4 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 10:45:27 -0400 Subject: [PATCH 03/22] refactor(traces): derive the internal span record from the hook-visible one The two declarations repeated every shared field. Declaring only what a hook may not rewrite keeps the halves from drifting; no behaviour change. --- packages/core/src/traces/types.ts | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/packages/core/src/traces/types.ts b/packages/core/src/traces/types.ts index 8371f6bb4e..9a8192afa3 100644 --- a/packages/core/src/traces/types.ts +++ b/packages/core/src/traces/types.ts @@ -21,6 +21,7 @@ import type { Span, SpanAttributes, SpanKind, + SpanRecord as HookSpanRecord, SpanStatusCode, TracesConfig, } from '@posthog/types' @@ -68,23 +69,18 @@ export interface SpanEventRecord { * A completed span in plain, pre-encoding form: strings for kind and status, a * plain attribute map, ms-epoch timestamps. */ -export interface SpanRecord { - traceId: string - spanId: string - parentSpanId?: string +/** + * A finished span as the SDK carries it, which is the hook-visible record plus + * the fields no hook may rewrite. Declaring only the additions keeps the shared + * half from drifting; a field added here rather than to the public record is a + * field `beforeSpanSend` cannot see, and so cannot corrupt. + */ +export interface SpanRecord extends HookSpanRecord { traceState?: string /** The W3C trace-flags byte this span propagates, e.g. `01` sampled. */ traceFlags: string /** True when the parent came from a `traceparent` header rather than a local handle. */ parentIsRemote: boolean - name: string - kind: SpanKind - status?: { code: SpanStatusCode; message?: string } - attributes: SpanAttributes - events: SpanEventRecord[] - /** ms epoch. */ - startTime: number - endTime: number droppedAttributesCount?: number droppedEventsCount?: number } From 308c686c67f2b8dedd37b0a0f989f0dd21f461c7 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 10:56:36 -0400 Subject: [PATCH 04/22] fix(traces): close four gaps in the per-span attribute bound - a back-reference is handed back as the encoder's marker, not the raw ancestor, which a copied parent let the encoder walk one level unbounded - arrays are built from the walked range rather than sliced, so an accessor past the encoder cap cannot cost every item its bound - the key walk stops where the encoder stops emitting, instead of running every getter on a wide object - bounded values are always copied, so a caller mutating what it passed to setAttribute cannot change what ships Also warn when a non-function beforeSpanSend entry is dropped: it is the redaction point, and silently filtering nothing ships what it would remove. --- packages/core/src/traces/span.spec.ts | 73 +++++++++++++++++++ packages/core/src/traces/span.ts | 53 ++++++++++---- .../src/__tests__/traces-defaults.spec.ts | 18 +++++ packages/node/src/client.ts | 2 +- packages/node/src/traces-defaults.ts | 24 ++++-- 5 files changed, 149 insertions(+), 21 deletions(-) diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 299e5828a8..6880ccc65c 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -3,6 +3,7 @@ import type { SpanInit } from './span' import type { SpanRecord } from './types' import type { Logger } from '../types' import { createMockLogger } from '@/testing' +import { MAX_JSON_SAFE_VALUE_ITEMS } from '../utils/json-utils' const TRACE_ID = '4bf92f3577b34da6a3ce929d0e0e4736' const SPAN_ID = '00f067aa0ba902b7' @@ -552,6 +553,78 @@ describe('PostHogSpan', () => { expect(ended[0].events[0].attributes?.['exception.message']).toBe('abcd') }) + it('replaces a back-reference with the marker rather than the value itself', () => { + // Handing the raw ancestor back left it inside a copied parent, where the + // encoder's own cycle detection no longer recognised it and walked one + // more level of its strings at full length. + const cyclic: any = { body: 'abcdefgh' } + cyclic.self = cyclic + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('payload', cyclic) + span.end() + + expect(ended[0].attributes.payload).toEqual({ body: 'abcd', self: '[Circular]' }) + }) + + it('still bounds an array whose accessor past the encoder cap throws', () => { + // `slice()` read the whole array to copy it, so one throwing accessor + // beyond the encoder's cap cost every item in range its bound. + const rows: unknown[] = ['abcdefgh'] + for (let index = 1; index < MAX_JSON_SAFE_VALUE_ITEMS + 200; index++) { + rows.push('x') + } + Object.defineProperty(rows, MAX_JSON_SAFE_VALUE_ITEMS + 100, { + get: () => { + throw new Error('lazy relation') + }, + enumerable: true, + configurable: true, + }) + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('rows', rows as any) + span.end() + + expect((ended[0].attributes.rows as unknown[])[0]).toBe('abcd') + }) + + it('stops reading keys where the encoder stops emitting them', () => { + // Every key was read even though the encoder emits at most the cap, so a + // wide object charged `setAttribute` for getters that never ship. + let reads = 0 + const wide: Record = {} + for (let index = 0; index < MAX_JSON_SAFE_VALUE_ITEMS * 2; index++) { + Object.defineProperty(wide, `k${index}`, { + get: () => { + reads++ + return 'v' + }, + enumerable: true, + configurable: true, + }) + } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('payload', wide) + span.end() + + expect(reads).toBe(MAX_JSON_SAFE_VALUE_ITEMS) + }) + + it('copies a nested value the caller goes on to mutate', () => { + // A value that needed no truncation was attached as it came, so the span + // held caller-owned state and shipped whatever it was changed to. + const nested = { body: 'ok' } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('payload', { nested }) + nested.body = 'abcdefgh' + span.end() + + expect(ended[0].attributes.payload).toEqual({ nested: { body: 'ok' } }) + }) + it('bounds an SDK-attached value, which is exempt from the count cap only', () => { const span = createSpan({ maxAttributeValueLength: 4, diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 28db8f92e2..e753d00c21 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -5,6 +5,7 @@ import { formatTraceparent, normalizeTraceparent, sanitizeTracestate, TRACE_FLAG import { assignUserAttributes, clampEndTime, resolveSuppliedTime, sanitizeName } from './sanitize' import { isArray, isError, isNullish } from '../utils' import { + CIRCULAR_VALUE, MAX_JSON_SAFE_VALUE_DEPTH, MAX_JSON_SAFE_VALUE_ITEMS, MAX_JSON_SAFE_VALUE_NODES, @@ -570,7 +571,17 @@ function truncateValue( if (value === null || typeof value !== 'object') { return value } - if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH || state.ancestors.has(value)) { + if (state.ancestors.has(value)) { + // The marker the encoder would produce, not the value itself. Handing the + // raw ancestor back puts it inside a *copied* parent, where the encoder's + // own cycle detection no longer recognises it and walks one more level of + // its strings at full length. + return CIRCULAR_VALUE + } + // Unlike a cycle, these two are bounded by the encoder as well: it stops at + // the same depth and charges every value, strings included, so its budget is + // spent no later than this one's. + if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH) { return value } state.remainingNodes-- @@ -586,37 +597,49 @@ function truncateValue( // self-describing, putting the internals of a redacted value on the wire. return isNullish(resolved.value) ? value : truncateValue(resolved.value, maxLength, state, depth + 1) } - let changed = false if (isArray(value)) { // Only the items the encoder will emit are walked; it stops at the same // cap, so bounding the rest is work spent on values that never ship. const walked = Math.min(value.length, MAX_JSON_SAFE_VALUE_ITEMS) - let boundedItems: SpanAttributeValue[] | undefined + // Accumulated rather than copied from the value: `slice()` reads every + // element, accessors past the cap included, and one of those throwing + // would reach the outer catch and cost the whole array its bound. + const boundedItems: SpanAttributeValue[] = [] for (let index = 0; index < walked; index++) { - const item = value[index] - const boundedItem = truncateValue(item, maxLength, state, depth + 1) - if (boundedItem !== item) { - // Copied lazily, so an array that needed nothing allocates nothing. - boundedItems = boundedItems ?? value.slice() - boundedItems[index] = boundedItem + try { + boundedItems.push(truncateValue(value[index], maxLength, state, depth + 1)) + } catch { + // A throwing accessor costs its own item, as it does in the encoder. + boundedItems.push(UNSERIALIZABLE_VALUE) } } - return boundedItems ?? value + // Carried so the encoder still marks what it cut. + if (value.length > walked) { + boundedItems.length = value.length + } + return boundedItems } const bounded: SpanAttributes = {} + // Counted the way the encoder counts, so the walk stops where its output + // does: a key it skips costs no slot, and reading past the last one it can + // emit is getter work on values that never ship. + let emittable = 0 for (const key of Object.keys(value)) { + if (emittable >= MAX_JSON_SAFE_VALUE_ITEMS) { + break + } let boundedItem: SpanAttributeValue try { // Read once: re-reading to compare would run a getter a second time. - const item = (value as SpanAttributes)[key] - boundedItem = truncateValue(item, maxLength, state, depth + 1) - changed = changed || boundedItem !== item + boundedItem = truncateValue((value as SpanAttributes)[key], maxLength, state, depth + 1) } catch { // A throwing accessor costs its own key. Reaching the walk's own catch // would abandon the whole value unbounded, which is how a lazy ORM // relation next to a large field puts that field on the wire whole. boundedItem = UNSERIALIZABLE_VALUE - changed = true + } + if (key && !isNullish(boundedItem)) { + emittable++ } // defineProperty, not assignment: a nested `__proto__` key would otherwise // swap the copy's prototype and vanish. @@ -627,7 +650,7 @@ function truncateValue( configurable: true, }) } - return changed ? bounded : value + return bounded } catch { // Whatever is left — a hostile `Object.keys`, a `slice` that throws — costs // this value its bound rather than the span. Per-key reads are guarded diff --git a/packages/node/src/__tests__/traces-defaults.spec.ts b/packages/node/src/__tests__/traces-defaults.spec.ts index 43d1d33224..57df1fe5aa 100644 --- a/packages/node/src/__tests__/traces-defaults.spec.ts +++ b/packages/node/src/__tests__/traces-defaults.spec.ts @@ -1,3 +1,4 @@ +import { createMockLogger } from '@posthog/core/testing' import { resolveTracesConfig } from '../traces-defaults' describe('resolveTracesConfig', () => { @@ -166,6 +167,23 @@ describe('resourceAttributes guarding', () => { expect(resolved.beforeSpanSend).toEqual([scrub]) }) + it('warns about a dropped hook, since the redaction it was configured for is gone', () => { + const logger = createMockLogger() + const scrub = (span: any): any => span + + resolveTracesConfig({ beforeSpanSend: ['not a function', scrub] as never }, undefined, logger) + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('ignoring 1 of 2 entries')) + }) + + it('stays quiet when every hook is callable', () => { + const logger = createMockLogger() + + resolveTracesConfig({ beforeSpanSend: [(span: any): any => span] }, undefined, logger) + + expect(logger.warn).not.toHaveBeenCalled() + }) + it('resolves to no hooks when beforeSpanSend is the wrong type entirely', () => { const resolved = resolveTracesConfig({ beforeSpanSend: { scrub: true } as never }) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 08cb08b189..5dd090024e 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -662,7 +662,7 @@ export abstract class PostHogBackendClient extends PostHogCoreStateless implemen if (!this._traces) { this._traces = new PostHogTraces( this, - resolveTracesConfig(this.options.traces, this.hostResourceAttributes()), + resolveTracesConfig(this.options.traces, this.hostResourceAttributes(), this._logger), this._logger, () => this._tracingContext(), this._spanContextManager, diff --git a/packages/node/src/traces-defaults.ts b/packages/node/src/traces-defaults.ts index 3623d7b45e..ce96656f87 100644 --- a/packages/node/src/traces-defaults.ts +++ b/packages/node/src/traces-defaults.ts @@ -1,5 +1,5 @@ import { assignUserAttributes } from '@posthog/core' -import type { BeforeSpanSendFn, ResolvedTracesConfig, TracesConfig } from '@posthog/core' +import type { BeforeSpanSendFn, Logger, ResolvedTracesConfig, TracesConfig } from '@posthog/core' // OpenTelemetry's BatchSpanProcessor defaults, which sit comfortably under the // server's 2 MB body cap. @@ -66,12 +66,25 @@ function withUsableIdentityKeys(attributes: TracesConfig['resourceAttributes']): * Keeps only the callable hooks. Anything else is dropped rather than called: an * untyped caller passing the wrong shape would otherwise have every span dropped * by a hook that throws, leaving tracing silently off. + * + * Dropping one is warned about rather than thrown on. `beforeSpanSend` is where + * redaction lives, so a configuration that silently filters nothing ships the + * values it was meant to remove — but a client constructor that throws takes the + * application down with it, which is the worse of the two. */ -function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend']): BeforeSpanSendFn[] { +function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend'], logger?: Logger): BeforeSpanSendFn[] { if (!beforeSpanSend) { return [] } - return [beforeSpanSend].flat().filter((hook): hook is BeforeSpanSendFn => typeof hook === 'function') + const supplied = [beforeSpanSend].flat() + const hooks = supplied.filter((hook): hook is BeforeSpanSendFn => typeof hook === 'function') + if (hooks.length !== supplied.length) { + logger?.warn( + `beforeSpanSend: ignoring ${supplied.length - hooks.length} of ${supplied.length} entries that are not functions. ` + + 'Spans export without them, so whatever they were redacting is not redacted.' + ) + } + return hooks } /** @@ -82,7 +95,8 @@ function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend']): */ export function resolveTracesConfig( config: TracesConfig | undefined, - hostResourceAttributes?: Record + hostResourceAttributes?: Record, + logger?: Logger ): ResolvedTracesConfig { // Copied key by key rather than spread: a throwing accessor on a user-supplied // attribute would otherwise escape the first `startSpan`. @@ -96,7 +110,7 @@ export function resolveTracesConfig( serviceVersion: (resourceAttributes?.['service.version'] as string | undefined) ?? config?.serviceVersion, environment: (resourceAttributes?.['deployment.environment'] as string | undefined) ?? config?.environment, resourceAttributes, - beforeSpanSend: resolveBeforeSpanSend(config?.beforeSpanSend), + beforeSpanSend: resolveBeforeSpanSend(config?.beforeSpanSend, logger), maxAttributesPerSpan: positiveInteger(config?.maxAttributesPerSpan, DEFAULT_MAX_ATTRIBUTES_PER_SPAN), maxEventsPerSpan: positiveInteger(config?.maxEventsPerSpan, DEFAULT_MAX_EVENTS_PER_SPAN), maxAttributeValueLength: positiveInteger(config?.maxAttributeValueLength, DEFAULT_MAX_ATTRIBUTE_VALUE_LENGTH), From d7959f681d7ffb4569c679f2cd70440c18955634 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 11:22:18 -0400 Subject: [PATCH 05/22] fix(traces): keep a span whose hook froze a rebuilt record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restoring the identity onto the hook's return value throws when the hook freezes it, and a throwing hook drops the span — so a rebuilding, freezing scrubber lost every span carrying a parent id while roots still exported. The rebuild takes all four identity fields from the pre-hook snapshot, and the write-back that lets a later hook in the chain read true ids is now best-effort. A forged id is ignored with the documented debug warning instead of costing the span. Also leave a Date unbounded, matching the encoder's own Date branch: the value walk turned it into an ISO string and then cut it, shipping a corrupted timestamp at a low maxAttributeValueLength. --- packages/core/src/traces/index.spec.ts | 50 ++++++++++++++++++++++++++ packages/core/src/traces/index.ts | 31 ++++++++++------ packages/core/src/traces/span.spec.ts | 11 ++++++ packages/core/src/traces/span.ts | 6 ++++ 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 60072edfd8..c1e190e8fc 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -757,6 +757,56 @@ describe('PostHogTraces', () => { expect(sentSpans(instance).map((s) => s.flags)).toEqual([0x300]) }) + it('ignores a forged identity from a frozen hook rather than dropping the span', async () => { + // Writing the id back onto a frozen return throws, and a throwing hook + // drops the span, so forging plus freezing used to lose every span. + const instance = createMockInstance() + const traces = createTraces( + { beforeSpanSend: [(span: SpanRecord) => Object.freeze({ ...span, traceId: '0'.repeat(32) }) as SpanRecord] }, + instance + ) + traces.startSpan('forged').end() + await traces.flush() + + expect(sentSpans(instance)).toHaveLength(1) + expect(sentSpans(instance)[0].traceId).not.toBe('0'.repeat(32)) + expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('keeping the original ids')) + }) + + it('exports a child span whose record a frozen hook rebuilt without the parent id', async () => { + // The shape that loses children but keeps roots: a rebuilt record has no + // parentSpanId to match, so restoring it wrote to a frozen object. + const instance = createMockInstance() + const traces = createTraces( + { + beforeSpanSend: [ + (span: SpanRecord) => + Object.freeze({ + traceId: span.traceId, + spanId: span.spanId, + name: span.name, + kind: span.kind, + attributes: span.attributes, + events: span.events, + startTime: span.startTime, + endTime: span.endTime, + }) as SpanRecord, + ], + }, + instance + ) + const root = traces.startSpan('root') + traces.startSpan('child', { parent: root }).end() + root.end() + await traces.flush() + + expect( + sentSpans(instance) + .map((s) => s.name) + .sort() + ).toEqual(['child', 'root']) + }) + it('runs hooks left to right and stops at the first null', async () => { const order: string[] = [] await endOneSpan([ diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index b969e57466..1fc23e6ebf 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -522,11 +522,13 @@ export class PostHogTraces { // class instance whose fields are prototype getters a spread would miss. // Naming them also bounds what can reach the wire. current = { - traceId: hooked.traceId, - spanId: hooked.spanId, - parentSpanId: hooked.parentSpanId, - // From the snapshot: `_keepSpanIdentity` has already put it back on the - // record, but no public type declares it, so it cannot be read off one. + // All four from the snapshot, never from the hook's return value. A hook + // that forges an id has it ignored, which is the documented behaviour, + // and one that also freezes what it returns keeps its span: writing the + // id back onto a frozen object throws, and a throw here drops the span. + traceId: identity.traceId, + spanId: identity.spanId, + parentSpanId: identity.parentSpanId, traceState: identity.traceState, name: hooked.name, kind: hooked.kind, @@ -616,12 +618,19 @@ export class PostHogTraces { // Only the fields that actually differ are written back. Assigning a value // to a frozen property throws even when it is the value already there, and // a hook that freezes the record it returns would otherwise drop every span. - restoreField(hooked, 'traceId', original.traceId) - restoreField(hooked, 'spanId', original.spanId) - restoreField(hooked, 'parentSpanId', original.parentSpanId) - // A hook that rebuilds the record instead of spreading it would otherwise - // drop tracestate, which is not part of the record the hook is handed. - restoreField(hooked, 'traceState', original.traceState) + // Best-effort, for the next hook in the chain only: the record this builds + // is not what gets exported. A frozen return refuses every write, and the + // span must survive that. + try { + restoreField(hooked, 'traceId', original.traceId) + restoreField(hooked, 'spanId', original.spanId) + restoreField(hooked, 'parentSpanId', original.parentSpanId) + // A hook that rebuilds the record instead of spreading it would otherwise + // drop tracestate, which is not part of the record the hook is handed. + restoreField(hooked, 'traceState', original.traceState) + } catch { + // Frozen. The rebuild reads the identity from the snapshot regardless. + } return hooked } diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 6880ccc65c..2e95ea337e 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -625,6 +625,17 @@ describe('PostHogSpan', () => { expect(ended[0].attributes.payload).toEqual({ nested: { body: 'ok' } }) }) + it('leaves a Date whole rather than truncating its timestamp', () => { + // The encoder emits a Date from its own branch ahead of any `toJSON`, so + // bounding it here shipped a cut-off timestamp instead of a shorter one. + const span = createSpan({ maxAttributeValueLength: 10 }) + + span.setAttribute('when', new Date('2020-01-02T03:04:05.000Z') as never) + span.end() + + expect(ended[0].attributes.when).toEqual(new Date('2020-01-02T03:04:05.000Z')) + }) + it('bounds an SDK-attached value, which is exempt from the count cap only', () => { const span = createSpan({ maxAttributeValueLength: 4, diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index e753d00c21..29a8bfd279 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -587,6 +587,12 @@ function truncateValue( state.remainingNodes-- state.ancestors.add(value) try { + // A Date is emitted by the encoder from its own branch, ahead of any + // `toJSON` probe, so bounding it here would ship a truncated timestamp + // rather than a shorter one. + if (value instanceof Date) { + return value + } // The representation the value defines for itself is what the encoder puts // on the wire, so it is what has to be bounded — a `toJSON` returning a // megabyte of text is invisible to a walk over the object's own keys. From 7c484b7d45c082d5d8f2c0aa8dccf6c25d60d9d8 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 11:33:45 -0400 Subject: [PATCH 06/22] fix(traces): report an inert beforeSpanSend entry at critical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit posthog-node gates every level except critical behind debug: true, so the warning an operator most needs — a redaction hook that is silently doing nothing — never reached them in the default configuration. --- packages/node/src/__tests__/traces-defaults.spec.ts | 5 +++-- packages/node/src/traces-defaults.ts | 8 +++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/node/src/__tests__/traces-defaults.spec.ts b/packages/node/src/__tests__/traces-defaults.spec.ts index 57df1fe5aa..7f38deb076 100644 --- a/packages/node/src/__tests__/traces-defaults.spec.ts +++ b/packages/node/src/__tests__/traces-defaults.spec.ts @@ -173,7 +173,8 @@ describe('resourceAttributes guarding', () => { resolveTracesConfig({ beforeSpanSend: ['not a function', scrub] as never }, undefined, logger) - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('ignoring 1 of 2 entries')) + // `critical`, not `warn`: every other level is gated behind `debug: true`. + expect(logger.critical).toHaveBeenCalledWith(expect.stringContaining('ignoring 1 of 2 entries')) }) it('stays quiet when every hook is callable', () => { @@ -181,7 +182,7 @@ describe('resourceAttributes guarding', () => { resolveTracesConfig({ beforeSpanSend: [(span: any): any => span] }, undefined, logger) - expect(logger.warn).not.toHaveBeenCalled() + expect(logger.critical).not.toHaveBeenCalled() }) it('resolves to no hooks when beforeSpanSend is the wrong type entirely', () => { diff --git a/packages/node/src/traces-defaults.ts b/packages/node/src/traces-defaults.ts index ce96656f87..8362c3bf28 100644 --- a/packages/node/src/traces-defaults.ts +++ b/packages/node/src/traces-defaults.ts @@ -67,10 +67,12 @@ function withUsableIdentityKeys(attributes: TracesConfig['resourceAttributes']): * untyped caller passing the wrong shape would otherwise have every span dropped * by a hook that throws, leaving tracing silently off. * - * Dropping one is warned about rather than thrown on. `beforeSpanSend` is where + * Dropping one is reported rather than thrown on. `beforeSpanSend` is where * redaction lives, so a configuration that silently filters nothing ships the * values it was meant to remove — but a client constructor that throws takes the - * application down with it, which is the worse of the two. + * application down with it, which is the worse of the two. `critical`, because + * every other level is gated behind `debug: true`, and a redaction hook that is + * quietly inert is exactly what an operator has to hear about without opting in. */ function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend'], logger?: Logger): BeforeSpanSendFn[] { if (!beforeSpanSend) { @@ -79,7 +81,7 @@ function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend'], l const supplied = [beforeSpanSend].flat() const hooks = supplied.filter((hook): hook is BeforeSpanSendFn => typeof hook === 'function') if (hooks.length !== supplied.length) { - logger?.warn( + logger?.critical( `beforeSpanSend: ignoring ${supplied.length - hooks.length} of ${supplied.length} entries that are not functions. ` + 'Spans export without them, so whatever they were redacting is not redacted.' ) From c3c20a56f4dcbe421228645f20deaa1c85fedb65 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 11:50:38 -0400 Subject: [PATCH 07/22] test(traces): cover the hook shape that actually drops the propagation fields The existing tests spread the record, which carries traceFlags through whether or not the rebuild reads it from the span, so both passed against the bug. Name the public fields instead, the shape the hook-visible type invites. Typing the rebuild so every field must be named turns a future field on either half of SpanRecord into a compile error there. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ERsAtgPJqKF46yur8bm6K --- packages/core/src/traces/index.spec.ts | 31 ++++++++++++++++++++++++++ packages/core/src/traces/index.ts | 10 ++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index c1e190e8fc..4a911717d1 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -743,6 +743,37 @@ describe('PostHogTraces', () => { expect(sentSpans(instance)[0].flags).toBe(0x300) }) + it('keeps them when the hook builds its record from the fields it can see', async () => { + // Spreading the record carries the propagation fields through even though + // no public type declares them; naming the public fields is what drops + // them, and is what the hook-visible type invites a caller to write. + const instance = createMockInstance() + const traces = createTraces( + { + beforeSpanSend: [ + (span: SpanRecord) => + ({ + traceId: span.traceId, + spanId: span.spanId, + parentSpanId: span.parentSpanId, + name: span.name, + kind: span.kind, + status: span.status, + attributes: span.attributes, + events: span.events, + startTime: span.startTime, + endTime: span.endTime, + }) as SpanRecord, + ], + }, + instance + ) + traces.startSpan('child', { parent: `00-${TRACE_ID}-${REMOTE_SPAN_ID}-00` }).end() + await traces.flush() + + expect(sentSpans(instance)[0].flags).toBe(0x300) + }) + it('keeps them when the rebuilding hook also freezes what it returns', async () => { // Restoring these onto the returned record would throw here, and a throwing // hook drops the span. diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 1fc23e6ebf..acbe14954d 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -65,6 +65,13 @@ function looksLikeSpan(value: unknown): boolean { } } +/** + * The rebuilt record with every field named, optional ones included. A field + * added to either half of `SpanRecord` is a compile error at the rebuild until + * it says whether a hook may set that field or the span keeps its own value. + */ +type RebuiltSpanRecord = { [K in keyof Required]: SpanRecord[K] } + interface SpanIdentity { traceId: string spanId: string @@ -521,7 +528,7 @@ export class PostHogTraces { // return value may be frozen, where every write here would throw, or a // class instance whose fields are prototype getters a spread would miss. // Naming them also bounds what can reach the wire. - current = { + const rebuilt: RebuiltSpanRecord = { // All four from the snapshot, never from the hook's return value. A hook // that forges an id has it ignored, which is the documented behaviour, // and one that also freezes what it returns keeps its span: writing the @@ -548,6 +555,7 @@ export class PostHogTraces { droppedAttributesCount: originalDropped.attributes, droppedEventsCount: originalDropped.events, } + current = rebuilt // A value missing either collection is not a span record — an `async` // hook returns a Promise, truthy and `undefined` for every field. Filling // the gaps in would export a nameless span carrying no person or session. From d046d2f99ee7618f686092e5a4ed0d995a24dc74 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 11:52:52 -0400 Subject: [PATCH 08/22] fix(traces): keep earliest-set attributes and bound span names applySpanLimits walked Object.keys, which hoists integer-like keys ahead of insertion order, so a beforeSpanSend hook adding `attributes['0']` evicted an attribute the caller had set before it ran. The spec makes the cap earliest-set-wins, so the walk now follows the span's own pre-hook key order. Span names and event names were the only caller-controlled free text left unbounded: a name built from a URL shipped whole while the status message beside it obeyed maxAttributeValueLength, and one large enough 413s the batch. Also: a discriminating test for the maxEventsPerSpan resolver, which a mutation survived; TSDoc on the hook-visible SpanRecord members; and the changeset now says how large the exception reserve is instead of calling it small. --- .changeset/node-span-limits.md | 2 +- packages/core/src/traces/index.spec.ts | 26 +++++++++++++++++++ packages/core/src/traces/index.ts | 12 ++++++--- packages/core/src/traces/sanitize.ts | 7 +++-- packages/core/src/traces/span.spec.ts | 17 +++++++++++- packages/core/src/traces/span.ts | 25 +++++++++++++++--- packages/core/src/traces/types.ts | 4 --- .../src/__tests__/traces-defaults.spec.ts | 9 +++++++ packages/node/src/__tests__/traces.spec.ts | 3 +-- packages/types/src/traces.ts | 14 +++++++--- 10 files changed, 97 insertions(+), 22 deletions(-) diff --git a/.changeset/node-span-limits.md b/.changeset/node-span-limits.md index 9d5fe27d45..ed04ded43f 100644 --- a/.changeset/node-span-limits.md +++ b/.changeset/node-span-limits.md @@ -4,4 +4,4 @@ '@posthog/types': minor --- -Cap spans at 128 user attributes, 128 events, and 8192 characters per string attribute value, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. The character bound applies to every string a value contains, including the ones nested inside arrays and objects, and to status messages and resource attributes as well. Once a span has spent its event cap, a small reserve stays available to `exception` events, so a span that fills its events and then throws still carries the exception. +Cap spans at 128 user attributes, 128 events, and 8192 characters per string attribute value, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. The character bound applies to every string a value contains, including the ones nested inside arrays and objects, and to span names, event names, status messages and resource attributes as well. Once a span has spent its event cap, up to four more `exception` events are still accepted, so a span that fills its events and then throws still carries the exception. diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index c1e190e8fc..532f05051d 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -757,6 +757,32 @@ describe('PostHogTraces', () => { expect(sentSpans(instance).map((s) => s.flags)).toEqual([0x300]) }) + it('keeps the earliest-set attributes when the hook adds an integer-like key', async () => { + // Object.keys hoists integer-like keys whatever the write order, so a key + // the hook added last outranked one the caller set before it ran. + const instance = createMockInstance() + const traces = createTraces( + { + maxAttributesPerSpan: 3, + beforeSpanSend: [ + (span: SpanRecord) => { + span.attributes['0'] = 'added-last' + return span + }, + ], + }, + instance + ) + const span = traces.startSpan('ordered') + span.setAttribute('alpha', 1) + span.setAttribute('beta', 2) + span.setAttribute('gamma', 3) + span.end() + await traces.flush() + + expect(sentSpans(instance)[0].attributes.map((a) => a.key)).toEqual(['alpha', 'beta', 'gamma']) + }) + it('ignores a forged identity from a frozen hook rather than dropping the span', async () => { // Writing the id back onto a frozen return throws, and a throwing hook // drops the span, so forging plus freezing used to lose every span. diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 1fc23e6ebf..3170e595a9 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -198,7 +198,7 @@ export class PostHogTraces { traceState: parent?.traceState, traceFlags: parent?.traceFlags, parentIsRemote: parent?.isRemote, - name: sanitizeName(name, 'Span name', this._logger), + name: sanitizeName(name, 'Span name', this._config.maxAttributeValueLength, this._logger), kind: options?.kind ?? 'internal', // Auto-context first so user-supplied attributes win on collision. attributes: assignUserAttributes({ ...autoAttributes }, options?.attributes), @@ -498,6 +498,9 @@ export class PostHogTraces { // Read here rather than restored onto the hook's return value: writing them // back would throw on a frozen record, and neither is on the record a hook // is handed, so a rebuilding hook always arrives without them. + // The order the span itself wrote them in, so the caps below can keep the + // earliest-set entries even when a hook adds an integer-like key. + const keysBeforeHook = Object.keys(record.attributes) const originalPropagation = { traceFlags: record.traceFlags, parentIsRemote: record.parentIsRemote, @@ -559,7 +562,7 @@ export class PostHogTraces { // Re-applied to whatever the hook returned: one undecodable timestamp 400s // the whole request, taking unrelated spans with it. - current.name = sanitizeName(current.name, 'Span name', this._logger) + current.name = sanitizeName(current.name, 'Span name', this._config.maxAttributeValueLength, this._logger) // A status the hook rewrote never went through `setStatus`. An unknown code // encodes as an empty status object, which loses an error the span really had. if (current.status && current.status.code !== 'ok' && current.status.code !== 'error') { @@ -576,7 +579,7 @@ export class PostHogTraces { try { sanitizedEvents.push({ ...event, - name: sanitizeName(event.name, 'Span event name', this._logger), + name: sanitizeName(event.name, 'Span event name', this._config.maxAttributeValueLength, this._logger), timestamp: resolveSuppliedTime(event.timestamp, current.startTime, 'event timestamp', this._logger), }) } catch { @@ -591,7 +594,8 @@ export class PostHogTraces { autoKeys, this._config.maxAttributesPerSpan, this._config.maxEventsPerSpan, - this._config.maxAttributeValueLength + this._config.maxAttributeValueLength, + keysBeforeHook ) return current } catch (error) { diff --git a/packages/core/src/traces/sanitize.ts b/packages/core/src/traces/sanitize.ts index 47257deba7..05241c848d 100644 --- a/packages/core/src/traces/sanitize.ts +++ b/packages/core/src/traces/sanitize.ts @@ -24,9 +24,12 @@ const DEEP_BACKDATE_WARNING_MS = 24 * 60 * 60 * 1000 * replaced rather than dropped, so a mis-instrumented call site loses its name, * not its span. `label` names what is being sanitized in the warning. */ -export function sanitizeName(name: unknown, label: string, logger?: Logger): string { +export function sanitizeName(name: unknown, label: string, maxLength: number, logger?: Logger): string { if (typeof name === 'string' && name.trim()) { - return name + // Bounded like a status message and an attribute value: a name built from a + // URL or a payload is caller-controlled too, and one large enough takes the + // span past the ingestion body limit, which drops it whole. + return name.length > maxLength ? name.slice(0, maxLength) : name } logger?.debug(`${label} must be a non-empty string; using "${FALLBACK_SPAN_NAME}"`) return FALLBACK_SPAN_NAME diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 2e95ea337e..3c4f991a2a 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -334,12 +334,14 @@ describe('PostHogSpan', () => { }) describe('maxAttributeValueLength', () => { - it('truncates a long string attribute', () => { + it('truncates a long string attribute without counting it as dropped', () => { const span = createSpan({ maxAttributeValueLength: 10 }) span.setAttribute('payload', 'x'.repeat(5000)) span.end() expect(ended[0].attributes.payload).toBe('xxxxxxxxxx') + // The count is for whole entries; a trimmed value is still exported. + expect(ended[0].droppedAttributesCount).toBeUndefined() }) it('truncates the strings inside an array attribute, and leaves other types alone', () => { @@ -534,6 +536,19 @@ describe('PostHogSpan', () => { expect(Object.getOwnPropertyDescriptor(payload, '__proto__')?.value).toEqual({ body: 'abcd' }) }) + it('bounds a span name and an event name, like a status message', () => { + // A name built from a URL is caller-controlled, and one large enough takes + // the span past the ingestion body limit. + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.updateName('abcdefgh') + span.addEvent('abcdefgh') + span.end() + + expect(ended[0].name).toBe('abcd') + expect(ended[0].events[0].name).toBe('abcd') + }) + it('bounds a status message', () => { const span = createSpan({ maxAttributeValueLength: 4 }) diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 29a8bfd279..1d94e4f5b1 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -195,7 +195,7 @@ export class PostHogSpan implements Span { if (this._mutable('addEvent')) { // Sanitised before the bucket check, so the name deciding the bucket is the // one that ends up on the record. - const eventName = sanitizeName(name, 'Span event name', this._logger) + const eventName = sanitizeName(name, 'Span event name', this._maxAttributeValueLength, this._logger) if (!this._claimEventSlot(eventName)) { this._droppedEvents++ return this @@ -250,7 +250,7 @@ export class PostHogSpan implements Span { updateName(name: string): this { if (this._mutable('updateName')) { - this._name = sanitizeName(name, 'Span name', this._logger) + this._name = sanitizeName(name, 'Span name', this._maxAttributeValueLength, this._logger) } return this } @@ -353,19 +353,36 @@ export function nonNegativeCount(value: unknown): number { * Earliest-set entries win, matching the span-side rule; SDK-attached keys are * exempt. Counts add to whatever the span already dropped. */ +/** + * The record's keys with the ones the span itself set first, in that order. + * + * `Object.keys` hoists integer-like keys to the front whatever the write order, + * so a hook adding `attributes['0']` would otherwise outrank an attribute the + * caller set before the hook ran — and the cap is documented as earliest-set-wins. + */ +function orderedKeys(attributes: SpanAttributes, keysBeforeHook: readonly string[]): string[] { + if (!keysBeforeHook.length) { + return Object.keys(attributes) + } + const beforeHook = keysBeforeHook.filter((key) => key in attributes) + const seen = new Set(beforeHook) + return [...beforeHook, ...Object.keys(attributes).filter((key) => !seen.has(key))] +} + export function applySpanLimits( record: SpanRecord, autoKeys: ReadonlySet, maxAttributes: number, maxEvents: number, - maxAttributeValueLength: number + maxAttributeValueLength: number, + keysBeforeHook: readonly string[] = [] ): void { let kept = 0 let droppedAttributes = 0 // Built fresh rather than edited in place: a hook is free to return a record // whose attributes it froze, and a `delete` on one throws. const attributes: SpanAttributes = {} - for (const key of Object.keys(record.attributes)) { + for (const key of orderedKeys(record.attributes, keysBeforeHook)) { const value = record.attributes[key] // Matches `_writeAttribute`: the encoder drops these, so a hook that blanks a // value rather than deleting the key must not evict a real attribute. diff --git a/packages/core/src/traces/types.ts b/packages/core/src/traces/types.ts index 9a8192afa3..7360c70a26 100644 --- a/packages/core/src/traces/types.ts +++ b/packages/core/src/traces/types.ts @@ -65,10 +65,6 @@ export interface SpanEventRecord { attributes?: SpanAttributes } -/** - * A completed span in plain, pre-encoding form: strings for kind and status, a - * plain attribute map, ms-epoch timestamps. - */ /** * A finished span as the SDK carries it, which is the hook-visible record plus * the fields no hook may rewrite. Declaring only the additions keeps the shared diff --git a/packages/node/src/__tests__/traces-defaults.spec.ts b/packages/node/src/__tests__/traces-defaults.spec.ts index 7f38deb076..7a712be76a 100644 --- a/packages/node/src/__tests__/traces-defaults.spec.ts +++ b/packages/node/src/__tests__/traces-defaults.spec.ts @@ -17,6 +17,15 @@ describe('resolveTracesConfig', () => { expect(resolved.maxAttributeValueLength).toBe(8192) }) + it('honours explicit per-span caps', () => { + // Without this the resolver can ignore maxEventsPerSpan entirely and every + // other test still passes, because they all assert the default. + expect(resolveTracesConfig({ maxAttributesPerSpan: 5, maxEventsPerSpan: 7 })).toMatchObject({ + maxAttributesPerSpan: 5, + maxEventsPerSpan: 7, + }) + }) + it('applies the documented defaults', () => { expect(resolveTracesConfig(undefined)).toMatchObject({ flushIntervalMs: 5000, diff --git a/packages/node/src/__tests__/traces.spec.ts b/packages/node/src/__tests__/traces.spec.ts index cbb9719fac..964bc075ab 100644 --- a/packages/node/src/__tests__/traces.spec.ts +++ b/packages/node/src/__tests__/traces.spec.ts @@ -341,11 +341,10 @@ describe('PostHog traces', () => { }) }) - it('bounds a stack by maxAttributeValueLength, and beforeSpanSend can scrub it', async () => { + it('beforeSpanSend can scrub a stacktrace', async () => { const scrubbed = createClient({ traces: { serviceName: 'checkout-api', - maxAttributeValueLength: 64, beforeSpanSend: (span: any) => { for (const event of span.events) { if (event.attributes?.['exception.stacktrace']) { diff --git a/packages/types/src/traces.ts b/packages/types/src/traces.ts index 4a3e571cd7..4f53471d58 100644 --- a/packages/types/src/traces.ts +++ b/packages/types/src/traces.ts @@ -183,17 +183,23 @@ export interface Span { * @experimental Subject to change in a minor release. */ export interface SpanRecord { - /** Assignment is ignored with a debug warning: rewriting ids orphans children that already shipped. */ + /** + * Assignment to any of the three identity fields is ignored with a debug + * warning: rewriting ids orphans children that already shipped. + */ readonly traceId: string readonly spanId: string + /** Absent on a root span. */ readonly parentSpanId?: string name: string kind: SpanKind status?: { code: SpanStatusCode; message?: string } + /** Editable in place; this is where to redact. */ attributes: SpanAttributes - events: { name: string; timestamp: number; attributes?: SpanAttributes }[] + events: { name: string; /** Millisecond epoch. */ timestamp: number; attributes?: SpanAttributes }[] /** Millisecond epoch. */ startTime: number + /** Millisecond epoch. */ endTime: number } @@ -311,8 +317,8 @@ export interface TracesConfig { * and later ones are dropped, with the number dropped reported on the * exported span. * - * Once the cap is spent, a small reserve is still available to `exception` - * events, so a span that fills its events and then throws still carries the + * Once the cap is spent, up to four more `exception` events are still + * accepted, so a span that fills its events and then throws still carries the * exception rather than only an `error` status. Below the cap an exception * is an ordinary event and spends an ordinary slot. * From e32c6e921be39982df2ea2168606d3dafc819231 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 12:14:23 -0400 Subject: [PATCH 09/22] fix(traces): charge every value against the attribute traversal budget Leaves skipped the node charge, so only containers spent the budget. A value whose siblings share a subtree is re-walked once per path that reaches it, and without a leaf charge that costs budget * items string copies where the encoder stops at budget: a 38 MB shared graph took 245 ms inside setAttribute and left 422 MB on the span record until flush. Now 4 ms and 19 MB. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ERsAtgPJqKF46yur8bm6K --- packages/core/src/traces/span.spec.ts | 36 +++++++++++++++++++++++++-- packages/core/src/traces/span.ts | 22 ++++++++-------- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 3c4f991a2a..617a883000 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -1,9 +1,9 @@ -import { NOOP_SPAN, PostHogSpan, describeError } from './span' +import { NOOP_SPAN, PostHogSpan, describeError, truncateAttributeValue } from './span' import type { SpanInit } from './span' import type { SpanRecord } from './types' import type { Logger } from '../types' import { createMockLogger } from '@/testing' -import { MAX_JSON_SAFE_VALUE_ITEMS } from '../utils/json-utils' +import { MAX_JSON_SAFE_VALUE_ITEMS, MAX_JSON_SAFE_VALUE_NODES } from '../utils/json-utils' const TRACE_ID = '4bf92f3577b34da6a3ce929d0e0e4736' const SPAN_ID = '00f067aa0ba902b7' @@ -370,6 +370,38 @@ describe('PostHogSpan', () => { expect(ended[0].attributes.rows).toEqual([{ body: 'abcd' }, ['abcd']]) }) + it('bounds the work a shared subtree costs, not just a cyclic one', () => { + // Siblings pointing at one object are re-walked once per path, so a DAG + // ten levels wide reaches the same leaves ten million times. Only the node + // budget stops it, and a leaf that skips the charge does not spend it. + const leaf: string[] = [] + for (let i = 0; i < 1000; i++) { + leaf.push('x'.repeat(2000)) + } + const mid = Array.from({ length: 1000 }, () => leaf) + const shared = Array.from({ length: 10 }, () => mid) + + const bounded = truncateAttributeValue(shared, 8) + + // Counting what the walk shortened, not what the result can reach: once + // the budget is gone the original is handed back by reference, so the + // untouched tail of the graph is still reachable through it. + const cap = MAX_JSON_SAFE_VALUE_NODES * 2 + let shortened = 0 + const stack: unknown[] = [bounded] + while (stack.length && shortened <= cap) { + const value = stack.pop() + if (typeof value === 'string') { + if (value.length === 8) { + shortened++ + } + } else if (Array.isArray(value) && value !== leaf && value !== mid) { + stack.push(...value) + } + } + expect(shortened).toBeLessThanOrEqual(cap) + }) + it('terminates on a self-referencing value', () => { const cyclic: any = { body: 'abcdefgh' } cyclic.self = cyclic diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 1d94e4f5b1..d918e2a93b 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -566,7 +566,7 @@ export function truncateAttributeValue(value: SpanAttributeValue, maxLength: num /** * Walks under the same depth cap, node budget and ancestor set as - * `encodeAnyValue`. Depth alone does not bound this: a value whose children + * `encodeAnyValue`, charging a node per value so the two cost the same. Depth alone does not bound this: a value whose children * point back at their siblings costs `fanout ** depth` visits, which is minutes * of synchronous work inside the caller's own `setAttribute` call. * @@ -579,14 +579,16 @@ function truncateValue( state: TruncateState, depth: number ): SpanAttributeValue { - // A string is a leaf and costs no traversal, so it is bounded before the - // budget is consulted. Charging it would let one large collection spend the - // budget and leave every string after it on the wire at full length. - if (typeof value === 'string') { - return truncateString(value, maxLength) - } if (value === null || typeof value !== 'object') { - return value + // Charged like every other value, as the encoder charges it. A shared + // subtree is re-walked once per path that reaches it, so leaving the leaves + // free lets one value cost `budget * items` string copies where the encoder + // would have stopped at `budget`. + if (state.remainingNodes <= 0) { + return value + } + state.remainingNodes-- + return typeof value === 'string' ? truncateString(value, maxLength) : value } if (state.ancestors.has(value)) { // The marker the encoder would produce, not the value itself. Handing the @@ -596,8 +598,8 @@ function truncateValue( return CIRCULAR_VALUE } // Unlike a cycle, these two are bounded by the encoder as well: it stops at - // the same depth and charges every value, strings included, so its budget is - // spent no later than this one's. + // the same depth and charges every value the same way, so its budget is spent + // no later than this one's and it marks whatever this walk left unbounded. if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH) { return value } From dff40e9a24a2c6255b8641e4ee3a6c76b73d4bd6 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 12:26:24 -0400 Subject: [PATCH 10/22] fix(traces): stop a hook-deleted prototype key from coming back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit orderedKeys tested `key in attributes`, which walks the prototype chain, so an attribute named `constructor` or `toString` survived the hook deleting it and read back as the inherited member. At the cap those ghosts took the slots: a hook returning a scrubbed bag exported ["toString","valueOf"] and dropped the one attribute it meant to keep. Three more from the same review: - The new name bound cut the SDK's own `exception` event name below a bound of 9, so the reserve stopped recognising it and dropped the event it exists to keep. `eventNameBound` floors it, at both sanitize call sites. - `_startFlush` installed its in-flight slot only after `_flushInner` had run its synchronous prefix, which this PR lengthened by bounding the resource attributes. A getter there that ends a span re-entered with no pass recorded and re-sent the head batch — 3070 times in a probe, not once. The pass now starts a microtask later, after the slot is installed. - A `false` entry from `[featureEnabled && scrub]` no longer reports an inert redaction hook, and the maxAttributeValueLength doc lists what it now bounds. --- packages/core/src/traces/index.spec.ts | 72 +++++++++++++++++++ packages/core/src/traces/index.ts | 31 +++++--- packages/core/src/traces/span.spec.ts | 26 +++++-- packages/core/src/traces/span.ts | 37 ++++++++-- .../src/__tests__/traces-defaults.spec.ts | 11 +++ packages/node/src/traces-defaults.ts | 7 +- packages/types/src/traces.ts | 4 +- 7 files changed, 164 insertions(+), 24 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 4ade6120b5..d45b0bcc02 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -610,6 +610,34 @@ describe('PostHogTraces', () => { }) }) + describe('flush reentrancy', () => { + it('does not re-send the head batch when a span ends during the flush prefix', async () => { + // `_flushInner` runs synchronously as far as its first await, and it reads + // the resource attributes in that window. A getter there that ends a span + // used to re-enter the flush with no pass yet recorded, and the same head + // batch went out again on every pass — thousands of times, unbounded. + const resourceAttributes: Record = {} + Object.defineProperty(resourceAttributes, 'tenant', { + enumerable: true, + // Reads `traces` only when a flush runs, which is after it is assigned. + get: () => { + traces.startSpan('late').end() + return 'acme' + }, + }) + const instance = createMockInstance() + const traces = createTraces({ maxExportBatchSize: 2, resourceAttributes }, instance) + + traces.startSpan('a').end() + traces.startSpan('b').end() + await traces.flush() + await traces.flush() + + expect(sentPayloads(instance)).toHaveLength(2) + expect(sentSpans(instance).map((s) => s.name)).toEqual(['a', 'b', 'late']) + }) + }) + describe('beforeSpanSend', () => { const endOneSpan = (beforeSpanSend: any): PostHogTraces => { const traces = createTraces({ beforeSpanSend: [beforeSpanSend].flat() }) @@ -788,6 +816,50 @@ describe('PostHogTraces', () => { expect(sentSpans(instance).map((s) => s.flags)).toEqual([0x300]) }) + it('does not resurrect a prototype-named attribute the hook removed', async () => { + // `key in attributes` walks the prototype chain, so a deleted `constructor` + // read back as the inherited function and shipped as [Function]. + const instance = createMockInstance() + const traces = createTraces( + { + beforeSpanSend: [ + (span: SpanRecord) => { + delete (span.attributes as Record).constructor + return span + }, + ], + }, + instance + ) + const span = traces.startSpan('ghost') + span.setAttribute('constructor', 'user-value') + span.setAttribute('safe', 'ok') + span.end() + await traces.flush() + + expect(sentSpans(instance)[0].attributes.map((a) => a.key)).toEqual(['safe']) + }) + + it('does not let prototype-named ghosts evict what the hook kept', async () => { + // Worse than resurrection: at the cap the ghosts won the slots and the + // attribute the hook deliberately kept was the one dropped. + const instance = createMockInstance() + const traces = createTraces( + { + maxAttributesPerSpan: 2, + beforeSpanSend: [(span: SpanRecord) => ({ ...span, attributes: { onlyThis: 'yes' } }) as SpanRecord], + }, + instance + ) + const span = traces.startSpan('ghosts') + span.setAttribute('toString', 1) + span.setAttribute('valueOf', 2) + span.end() + await traces.flush() + + expect(sentSpans(instance)[0].attributes.map((a) => a.key)).toEqual(['onlyThis']) + }) + it('keeps the earliest-set attributes when the hook adds an integer-like key', async () => { // Object.keys hoists integer-like keys whatever the write order, so a key // the hook added last outranked one the caller set before it ran. diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 1217dac1d9..04726a6459 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -14,6 +14,7 @@ import { PostHogSpan, applySpanLimits, describeError, + eventNameBound, inertSpan, monotonicNow, truncateAttributes, @@ -297,14 +298,21 @@ export class PostHogTraces { private _startFlush(): Promise { this._clearFlushTimer() - const promise = this._flushInner().finally(() => { - // Only clear the slot this call installed: a `reset()` mid-flight may - // already have installed a newer one. - if (this._flushPromise === promise) { - this._flushPromise = null - } - this._armFlushTimerIfQueued() - }) + // Deferred by a microtask so the slot below is installed before the pass + // reads anything: `_flushInner` runs synchronously as far as its first + // await, and a resource-attribute getter or `toJSON` that ends a span in + // that window would otherwise re-enter here, find no pass in flight, and + // send the same head batch again — without bound. + const promise = Promise.resolve() + .then(() => this._flushInner()) + .finally(() => { + // Only clear the slot this call installed: a `reset()` mid-flight may + // already have installed a newer one. + if (this._flushPromise === promise) { + this._flushPromise = null + } + this._armFlushTimerIfQueued() + }) this._flushPromise = promise return promise } @@ -587,7 +595,12 @@ export class PostHogTraces { try { sanitizedEvents.push({ ...event, - name: sanitizeName(event.name, 'Span event name', this._config.maxAttributeValueLength, this._logger), + name: sanitizeName( + event.name, + 'Span event name', + eventNameBound(this._config.maxAttributeValueLength), + this._logger + ), timestamp: resolveSuppliedTime(event.timestamp, current.startTime, 'event timestamp', this._logger), }) } catch { diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 3c4f991a2a..3a4526047a 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -536,17 +536,31 @@ describe('PostHogSpan', () => { expect(Object.getOwnPropertyDescriptor(payload, '__proto__')?.value).toEqual({ body: 'abcd' }) }) + it('never cuts the reserved exception event name', () => { + // The reserve compares against the whole constant, so a bound short enough + // to cut it silently dropped the exception event it exists to keep. + const span = createSpan({ maxAttributeValueLength: 8, maxEvents: 2 }) + + span.addEvent('a') + span.addEvent('b') + span.recordException(new Error('boom')) + span.end() + + expect(ended[0].events.map((e) => e.name)).toEqual(['a', 'b', 'exception']) + }) + it('bounds a span name and an event name, like a status message', () => { // A name built from a URL is caller-controlled, and one large enough takes - // the span past the ingestion body limit. - const span = createSpan({ maxAttributeValueLength: 4 }) + // the span past the ingestion body limit. The bound is 12 rather than 4 + // because an event name is floored at the reserved `exception`. + const span = createSpan({ maxAttributeValueLength: 12 }) - span.updateName('abcdefgh') - span.addEvent('abcdefgh') + span.updateName('abcdefghijklmnop') + span.addEvent('abcdefghijklmnop') span.end() - expect(ended[0].name).toBe('abcd') - expect(ended[0].events[0].name).toBe('abcd') + expect(ended[0].name).toBe('abcdefghijkl') + expect(ended[0].events[0].name).toBe('abcdefghijkl') }) it('bounds a status message', () => { diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 1d94e4f5b1..1a0467327a 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -195,7 +195,12 @@ export class PostHogSpan implements Span { if (this._mutable('addEvent')) { // Sanitised before the bucket check, so the name deciding the bucket is the // one that ends up on the record. - const eventName = sanitizeName(name, 'Span event name', this._maxAttributeValueLength, this._logger) + const eventName = sanitizeName( + name, + 'Span event name', + eventNameBound(this._maxAttributeValueLength), + this._logger + ) if (!this._claimEventSlot(eventName)) { this._droppedEvents++ return this @@ -311,6 +316,17 @@ export class PostHogSpan implements Span { const EXCEPTION_EVENT_NAME = 'exception' +/** + * The value bound as it applies to an event name, floored at the reserved name. + * + * `isExceptionEvent` compares against the whole constant, so a bound short + * enough to cut it would silently disable the exception reserve and drop the + * event the reserve exists to keep. + */ +export function eventNameBound(maxAttributeValueLength: number): number { + return Math.max(maxAttributeValueLength, EXCEPTION_EVENT_NAME.length) +} + /** * How many `exception` events may sit past the event cap. * @@ -364,7 +380,11 @@ function orderedKeys(attributes: SpanAttributes, keysBeforeHook: readonly string if (!keysBeforeHook.length) { return Object.keys(attributes) } - const beforeHook = keysBeforeHook.filter((key) => key in attributes) + // hasOwnProperty, not `in`: `in` walks the prototype chain, so a key the caller + // set that collides with Object.prototype survives the hook deleting it and + // reads back as the inherited member — the same trap the attribute store's own + // Object.keys comment describes. + const beforeHook = keysBeforeHook.filter((key) => Object.prototype.hasOwnProperty.call(attributes, key)) const seen = new Set(beforeHook) return [...beforeHook, ...Object.keys(attributes).filter((key) => !seen.has(key))] } @@ -536,7 +556,14 @@ export function inertSpan(options?: { parent?: unknown; tracestate?: unknown }): return new PassThroughSpan(traceparent, sanitizeTracestate(options?.tracestate)) } -/** Mirrors `encodeAnyValue`'s traversal budget so the two walks cost the same. */ +/** + * The same depth, node and item caps `encodeAnyValue` uses, but spent per + * attribute rather than per bag: the encoder allocates one budget for a whole + * attribute map, this walk allocates one per value. That makes the encoder's + * budget the stricter of the two — whatever this walk hands back unbounded, the + * encoder has already stopped short of — at the cost of a wide span paying for + * a walk whose results the encoder then discards. + */ interface TruncateState { /** Containers on the current path, so a back-reference stops the walk. */ ancestors: WeakSet @@ -596,8 +623,8 @@ function truncateValue( return CIRCULAR_VALUE } // Unlike a cycle, these two are bounded by the encoder as well: it stops at - // the same depth and charges every value, strings included, so its budget is - // spent no later than this one's. + // the same depth, charges every value rather than only containers, and spends + // one budget across the whole bag, so it runs out no later than this walk. if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH) { return value } diff --git a/packages/node/src/__tests__/traces-defaults.spec.ts b/packages/node/src/__tests__/traces-defaults.spec.ts index 7a712be76a..c22e1aa211 100644 --- a/packages/node/src/__tests__/traces-defaults.spec.ts +++ b/packages/node/src/__tests__/traces-defaults.spec.ts @@ -186,6 +186,17 @@ describe('resourceAttributes guarding', () => { expect(logger.critical).toHaveBeenCalledWith(expect.stringContaining('ignoring 1 of 2 entries')) }) + it('stays quiet for a conditionally disabled hook', () => { + // `[featureEnabled && scrub]` is ordinary JS; nothing was configured to + // redact, so claiming redaction is broken would be false alarm. + const logger = createMockLogger() + + const resolved = resolveTracesConfig({ beforeSpanSend: [false, null, undefined] as never }, undefined, logger) + + expect(resolved.beforeSpanSend).toEqual([]) + expect(logger.critical).not.toHaveBeenCalled() + }) + it('stays quiet when every hook is callable', () => { const logger = createMockLogger() diff --git a/packages/node/src/traces-defaults.ts b/packages/node/src/traces-defaults.ts index 8362c3bf28..06292e84ed 100644 --- a/packages/node/src/traces-defaults.ts +++ b/packages/node/src/traces-defaults.ts @@ -1,4 +1,4 @@ -import { assignUserAttributes } from '@posthog/core' +import { assignUserAttributes, isNullish } from '@posthog/core' import type { BeforeSpanSendFn, Logger, ResolvedTracesConfig, TracesConfig } from '@posthog/core' // OpenTelemetry's BatchSpanProcessor defaults, which sit comfortably under the @@ -78,7 +78,10 @@ function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend'], l if (!beforeSpanSend) { return [] } - const supplied = [beforeSpanSend].flat() + // `[featureEnabled && scrub]` is ordinary JS, and a caller who wrote it did not + // configure a hook at all — only a value that was meant to be one is worth + // shouting about. + const supplied = [beforeSpanSend].flat().filter((hook) => (hook as unknown) !== false && !isNullish(hook)) const hooks = supplied.filter((hook): hook is BeforeSpanSendFn => typeof hook === 'function') if (hooks.length !== supplied.length) { logger?.critical( diff --git a/packages/types/src/traces.ts b/packages/types/src/traces.ts index 4f53471d58..4bdd8c1b3a 100644 --- a/packages/types/src/traces.ts +++ b/packages/types/src/traces.ts @@ -330,8 +330,8 @@ export interface TracesConfig { * Maximum length of a string attribute value. Longer values are truncated, * and the bound reaches every string the value contains, including the ones * nested inside arrays and objects. It applies to span attributes, event - * attributes, status messages and resource attributes alike — including - * `exception.stacktrace`. + * attributes, span names, event names, status messages and resource + * attributes alike — including `exception.stacktrace`. * * The bound is what keeps one large value from making a span too large for * the ingestion endpoint, which drops an oversized span whole. From deacd9ca5d79941ee3f2cefb7f4ab6b4bdf99195 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 12:53:20 -0400 Subject: [PATCH 11/22] fix(traces): stop nullish leaves spending the truncation budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The encoder drops a nullish value without charging its budget, so charging one here made this walk the stricter of the two and broke the invariant the rest of the walk relies on: a value with 10,000 null leaves ahead of a large string exhausted this budget while the encoder still had room, and the string shipped whole — 2 MB under a bound of 8, with no backstop on either side. Nullish leaves are free again; the shared-subtree cost that charging fixed stays fixed. Also from the same review: - `_startFlush` samples the generation before the microtask, so a `reset()` in that window marks the pending pass stale rather than letting it drain the post-reset queue alongside the pass `reset()` started. - `orderedKeys` uses `propertyIsEnumerable`, the predicate the encoder itself uses, so a key a hook hid by making it non-enumerable cannot take a cap slot. - The `beforeSpanSend` filter reports only on a value meant to be a hook, so `[items.length && scrub]` and `[name && scrub]` are quiet too. --- packages/core/src/traces/index.ts | 6 ++++- packages/core/src/traces/span.spec.ts | 17 ++++++++++++++ packages/core/src/traces/span.ts | 33 ++++++++++++++++----------- packages/node/src/traces-defaults.ts | 4 ++-- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 04726a6459..6336d37522 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -303,8 +303,12 @@ export class PostHogTraces { // await, and a resource-attribute getter or `toJSON` that ends a span in // that window would otherwise re-enter here, find no pass in flight, and // send the same head batch again — without bound. + // Sampled before the microtask, not inside `_flushInner`: a `reset()` landing + // in the window would otherwise be invisible to this pass, which would then + // drain the post-reset queue alongside the pass `reset()` started. + const startedAtGeneration = this._generation const promise = Promise.resolve() - .then(() => this._flushInner()) + .then(() => (startedAtGeneration === this._generation ? this._flushInner() : 0)) .finally(() => { // Only clear the slot this call installed: a `reset()` mid-flight may // already have installed a newer one. diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 52d12f4250..3f2fe0dbb2 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -543,6 +543,23 @@ describe('PostHogSpan', () => { expect(ended[0].attributes.doc).toBe('ab') }) + it('bounds a string that follows a large collection of nulls', () => { + // The encoder drops a nullish value without spending its budget, so a walk + // that charges for one runs out first and leaves the string after it + // unbounded on both sides — a 2 MB value under a bound of 8. + const span = createSpan({ maxAttributeValueLength: 8 }) + + span.setAttribute('payload', { + rows: Array.from({ length: 400 }, () => + Object.fromEntries(Array.from({ length: 50 }, (_unused, index) => [`c${index}`, null])) + ), + html: 'X'.repeat(50000), + }) + span.end() + + expect((ended[0].attributes.payload as any).html).toHaveLength(8) + }) + it('bounds a string that follows a large collection', () => { // The traversal budget is spent on containers, not leaves: a big array // used to exhaust it and leave every later string at full length. diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index af923ae33f..0c12153ed0 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -380,11 +380,11 @@ function orderedKeys(attributes: SpanAttributes, keysBeforeHook: readonly string if (!keysBeforeHook.length) { return Object.keys(attributes) } - // hasOwnProperty, not `in`: `in` walks the prototype chain, so a key the caller - // set that collides with Object.prototype survives the hook deleting it and - // reads back as the inherited member — the same trap the attribute store's own - // Object.keys comment describes. - const beforeHook = keysBeforeHook.filter((key) => Object.prototype.hasOwnProperty.call(attributes, key)) + // The encoder's own predicate: `in` would walk the prototype chain, so a key + // the caller set that collides with Object.prototype survives the hook deleting + // it and reads back as the inherited member, and `hasOwnProperty` would keep a + // key the hook hid by making it non-enumerable, which the encoder never emits. + const beforeHook = keysBeforeHook.filter((key) => Object.prototype.propertyIsEnumerable.call(attributes, key)) const seen = new Set(beforeHook) return [...beforeHook, ...Object.keys(attributes).filter((key) => !seen.has(key))] } @@ -607,10 +607,17 @@ function truncateValue( depth: number ): SpanAttributeValue { if (value === null || typeof value !== 'object') { - // Charged like every other value, as the encoder charges it. A shared - // subtree is re-walked once per path that reaches it, so leaving the leaves - // free lets one value cost `budget * items` string copies where the encoder - // would have stopped at `budget`. + // A nullish leaf is free here because the encoder drops one without spending + // its budget. Charging it would make this walk the stricter of the two, and + // then a value whose leaves are mostly nulls exhausts this budget while the + // encoder still has room — leaving a large string unbounded on both sides. + if (isNullish(value)) { + return value + } + // Everything else is charged as the encoder charges it. A shared subtree is + // re-walked once per path that reaches it, so leaving the leaves free lets + // one value cost `budget * items` string copies where the encoder would have + // stopped at `budget`. if (state.remainingNodes <= 0) { return value } @@ -624,10 +631,10 @@ function truncateValue( // its strings at full length. return CIRCULAR_VALUE } - // Unlike a cycle, these two are bounded by the encoder as well: it stops at - // the same depth, charges values the same way, and spends one budget across the - // whole bag rather than one per attribute, so it runs out no later than this - // walk and marks whatever this one left unbounded. + // Unlike a cycle, these two are bounded by the encoder as well: it stops at the + // same depth, charges the same values — nullish leaves free on both sides — and + // spends one budget across the whole bag rather than one per attribute, so it + // runs out no later than this walk and marks whatever this one left unbounded. if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH) { return value } diff --git a/packages/node/src/traces-defaults.ts b/packages/node/src/traces-defaults.ts index 06292e84ed..c207bb85ad 100644 --- a/packages/node/src/traces-defaults.ts +++ b/packages/node/src/traces-defaults.ts @@ -1,4 +1,4 @@ -import { assignUserAttributes, isNullish } from '@posthog/core' +import { assignUserAttributes } from '@posthog/core' import type { BeforeSpanSendFn, Logger, ResolvedTracesConfig, TracesConfig } from '@posthog/core' // OpenTelemetry's BatchSpanProcessor defaults, which sit comfortably under the @@ -81,7 +81,7 @@ function resolveBeforeSpanSend(beforeSpanSend: TracesConfig['beforeSpanSend'], l // `[featureEnabled && scrub]` is ordinary JS, and a caller who wrote it did not // configure a hook at all — only a value that was meant to be one is worth // shouting about. - const supplied = [beforeSpanSend].flat().filter((hook) => (hook as unknown) !== false && !isNullish(hook)) + const supplied = [beforeSpanSend].flat().filter((hook) => Boolean(hook)) const hooks = supplied.filter((hook): hook is BeforeSpanSendFn => typeof hook === 'function') if (hooks.length !== supplied.length) { logger?.critical( From 41179c64620ede59a97238e0bb1df321dceac15e Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 12:57:03 -0400 Subject: [PATCH 12/22] fix(traces): wait for the span export before a flush settles `_flushEventsAndSpans` combined the two flushes with `Promise.all`, which rejects the moment the event flush does. A serverless host treats the returned promise as the end of the invocation, so an event endpoint failing mid-request let the platform freeze the handler with the span POST still open. `allSettled` waits for both and still surfaces the events rejection to the caller. `reset()` also discarded whatever was queued without a word, while the only line the operator had seen was the export failure promising a retry on a flush that will never come. It now names the count at `critical`, the one level posthog-node does not gate behind `debug`. --- packages/core/src/traces/index.spec.ts | 26 ++++++++++++++ packages/core/src/traces/index.ts | 10 ++++++ .../src/__tests__/waituntil-flush.spec.ts | 36 +++++++++++++++++++ packages/node/src/client.ts | 11 +++++- 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index d45b0bcc02..874d8ccd6e 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -610,6 +610,32 @@ describe('PostHogTraces', () => { }) }) + describe('reset', () => { + it('says so when it discards queued spans', async () => { + // Terminal loss: there is no next flush to retry on, and the export + // failure the caller already saw promises one. + const instance = createMockInstance({ + _sendTracesBatch: vi.fn(() => Promise.resolve({ kind: 'retry-later' as const, error: new Error('down') })), + }) + const traces = createTraces({}, instance) + traces.startSpan('a').end() + traces.startSpan('b').end() + await traces.flush() + + traces.reset() + + expect(logger.critical).toHaveBeenCalledWith(expect.stringContaining('Discarding 2 span(s)')) + }) + + it('stays quiet when nothing was queued', () => { + const traces = createTraces() + + traces.reset() + + expect(logger.critical).not.toHaveBeenCalled() + }) + }) + describe('flush reentrancy', () => { it('does not re-send the head batch when a span ends during the flush prefix', async () => { // `_flushInner` runs synchronously as far as its first await, and it reads diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 6336d37522..4ce66257d6 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -324,6 +324,16 @@ export class PostHogTraces { /** Clears the queue and timer. Used on shutdown and between tests. */ reset(): void { this._clearFlushTimer() + if (this._queue.length) { + // Critical, and said here rather than counted: this is the last chance to + // say anything about these spans, the drop warning is gated behind `debug` + // on some hosts, and the only other line the caller sees is the export + // failure promising a retry on a flush that will never come. + this._logger.critical( + `Discarding ${this._queue.length} span(s) that were still queued when tracing was shut down. ` + + 'Raise the shutdown timeout or flush earlier if they matter.' + ) + } this._queue = [] this._liveSpans.clear() this._flushPromise = null diff --git a/packages/node/src/__tests__/waituntil-flush.spec.ts b/packages/node/src/__tests__/waituntil-flush.spec.ts index e16c36cb8c..6cb4855988 100644 --- a/packages/node/src/__tests__/waituntil-flush.spec.ts +++ b/packages/node/src/__tests__/waituntil-flush.spec.ts @@ -12,6 +12,42 @@ function getFlushedBatches(): any[][] { .map((c) => JSON.parse((c[1] as any).body).batch) } +describe('flush combines events and spans', () => { + it('waits for the span export even when the event flush rejects', async () => { + // `Promise.all` settled on the event rejection, so a serverless host could + // freeze the invocation with the span request still open. + vi.useRealTimers() + const order: string[] = [] + mockedFetch.mockImplementation(async (url: any) => { + if (String(url).includes('/i/v1/traces')) { + order.push('traces-start') + await new Promise((resolve) => setTimeout(resolve, 30)) + order.push('traces-done') + return { status: 200, text: () => Promise.resolve('{}'), json: () => Promise.resolve({}) } as any + } + throw new Error('events endpoint down') + }) + const posthog = new PostHog('key', { + host: 'http://example.com', + flushAt: 1, + fetchRetryCount: 0, + traces: { serviceName: 'svc' }, + disableCompression: true, + }) + posthog.capture({ distinctId: 'u', event: 'e' }) + posthog.startSpan('s').end() + + await expect(posthog.flush()).rejects.toThrow() + order.push('flush-returned') + + // Exact, not an index comparison: before the fix `traces-done` is absent + // when flush returns, and `indexOf` gives -1, which passes any `lessThan`. + expect(order).toEqual(['traces-start', 'traces-done', 'flush-returned']) + await posthog.shutdown() + vi.useFakeTimers() + }) +}) + describe('waitUntil debounced flush', () => { vi.useFakeTimers() diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 5dd090024e..119bc70a1d 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -1,6 +1,7 @@ import { version } from './version' import { + allSettled, FeatureFlagValue, getEventUuid, isBlockedUA, @@ -298,7 +299,15 @@ export abstract class PostHogBackendClient extends PostHogCoreStateless implemen if (!this._traces) { return events } - return Promise.all([events, this._traces.flush().catch(() => {})]).then(() => undefined) + // Settled, not `all`: `all` rejects the moment the event flush does, and a + // serverless host that treats this promise as the end of the invocation can + // freeze it with the span request still open. The events rejection is still + // the one the caller sees. + return allSettled([events, this._traces.flush().catch(() => {})]).then(([eventsResult]) => { + if (eventsResult.status === 'rejected') { + throw eventsResult.reason + } + }) } override async flush(): Promise { From 1b11488459afe61abcd7e84e34d00987fffdec0d Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 14:24:50 -0400 Subject: [PATCH 13/22] docs(traces): tighten the truncation comments and the traces changesets Budget parity was explained in four places; keep it in the function doc. Rewrite the three changesets as one-line, outcome-first entries. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ERsAtgPJqKF46yur8bm6K --- .changeset/node-before-span-send.md | 2 +- .changeset/node-exception-stacktrace.md | 2 +- .changeset/node-span-limits.md | 2 +- packages/core/src/traces/index.spec.ts | 5 ++--- packages/core/src/traces/span.spec.ts | 10 ++++------ packages/core/src/traces/span.ts | 26 +++++++++---------------- 6 files changed, 18 insertions(+), 29 deletions(-) diff --git a/.changeset/node-before-span-send.md b/.changeset/node-before-span-send.md index 44a95b6da2..8d2afd5521 100644 --- a/.changeset/node-before-span-send.md +++ b/.changeset/node-before-span-send.md @@ -4,4 +4,4 @@ '@posthog/types': minor --- -Add a `traces.beforeSpanSend` hook to scrub attributes on a finished span, or return `null` to drop it. +Add a `traces.beforeSpanSend` hook to edit a finished span before it is queued, or drop it by returning `null` — a hook that throws, or returns anything that is not a span record, also drops the span. diff --git a/.changeset/node-exception-stacktrace.md b/.changeset/node-exception-stacktrace.md index efe7d6d27f..e4cfa02136 100644 --- a/.changeset/node-exception-stacktrace.md +++ b/.changeset/node-exception-stacktrace.md @@ -4,4 +4,4 @@ '@posthog/types': minor --- -Record `exception.stacktrace` on spans: `recordException` and a throwing `withSpan` callback now attach the stack alongside `exception.type` and `exception.message`. Stacks are attached by default and carry your server's file paths; remove the attribute in `traces.beforeSpanSend` if you'd rather they didn't leave the process. The value is bounded by `traces.maxAttributeValueLength` like any other attribute. +Attach `exception.stacktrace` to the exception events recorded by `recordException` and by a throwing `withSpan` callback — remove it in `traces.beforeSpanSend` to keep your server's file paths out of PostHog. diff --git a/.changeset/node-span-limits.md b/.changeset/node-span-limits.md index ed04ded43f..88ab8432b7 100644 --- a/.changeset/node-span-limits.md +++ b/.changeset/node-span-limits.md @@ -4,4 +4,4 @@ '@posthog/types': minor --- -Cap spans at 128 user attributes, 128 events, and 8192 characters per string attribute value, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. The character bound applies to every string a value contains, including the ones nested inside arrays and objects, and to span names, event names, status messages and resource attributes as well. Once a span has spent its event cap, up to four more `exception` events are still accepted, so a span that fills its events and then throws still carries the exception. +Cap spans at 128 user attributes, 128 events (plus a small reserve for exception events), and 8192 characters per string, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 874d8ccd6e..80e1764533 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -798,9 +798,8 @@ describe('PostHogTraces', () => { }) it('keeps them when the hook builds its record from the fields it can see', async () => { - // Spreading the record carries the propagation fields through even though - // no public type declares them; naming the public fields is what drops - // them, and is what the hook-visible type invites a caller to write. + // Spreading carries the propagation fields through even though no public + // type declares them; naming the public fields is what drops them. const instance = createMockInstance() const traces = createTraces( { diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index 3f2fe0dbb2..dc4feef046 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -371,9 +371,8 @@ describe('PostHogSpan', () => { }) it('bounds the work a shared subtree costs, not just a cyclic one', () => { - // Siblings pointing at one object are re-walked once per path, so a DAG - // ten levels wide reaches the same leaves ten million times. Only the node - // budget stops it, and a leaf that skips the charge does not spend it. + // Siblings pointing at one object are re-walked once per path, so this + // reaches the same leaves ten million times. Only the node budget stops it. const leaf: string[] = [] for (let i = 0; i < 1000; i++) { leaf.push('x'.repeat(2000)) @@ -383,9 +382,8 @@ describe('PostHogSpan', () => { const bounded = truncateAttributeValue(shared, 8) - // Counting what the walk shortened, not what the result can reach: once - // the budget is gone the original is handed back by reference, so the - // untouched tail of the graph is still reachable through it. + // Counting what the walk shortened, not what the result can reach: past + // the budget the original is handed back by reference. const cap = MAX_JSON_SAFE_VALUE_NODES * 2 let shortened = 0 const stack: unknown[] = [bounded] diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 0c12153ed0..e76b7942a7 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -593,12 +593,13 @@ export function truncateAttributeValue(value: SpanAttributeValue, maxLength: num /** * Walks under the same depth cap, node budget and ancestor set as - * `encodeAnyValue`, charging a node per value so the two cost the same. Depth alone does not bound this: a value whose children - * point back at their siblings costs `fanout ** depth` visits, which is minutes - * of synchronous work inside the caller's own `setAttribute` call. + * `encodeAnyValue`, charging the same values. The encoder spends one budget + * across the whole attribute bag where this spends one per value, so it runs out + * no later and marks whatever this walk returned whole. * - * A value the walk cannot bound is returned as it came. The encoder walks it - * again under its own budget and marks whatever it finds there. + * Depth alone does not bound this: a value whose children point back at their + * siblings costs `fanout ** depth` visits, which is minutes of synchronous work + * inside the caller's own `setAttribute` call. */ function truncateValue( value: SpanAttributeValue, @@ -607,17 +608,12 @@ function truncateValue( depth: number ): SpanAttributeValue { if (value === null || typeof value !== 'object') { - // A nullish leaf is free here because the encoder drops one without spending - // its budget. Charging it would make this walk the stricter of the two, and - // then a value whose leaves are mostly nulls exhausts this budget while the - // encoder still has room — leaving a large string unbounded on both sides. + // Free, as it is in the encoder, which drops a nullish leaf without charging. if (isNullish(value)) { return value } - // Everything else is charged as the encoder charges it. A shared subtree is - // re-walked once per path that reaches it, so leaving the leaves free lets - // one value cost `budget * items` string copies where the encoder would have - // stopped at `budget`. + // A shared subtree is re-walked once per path reaching it, so a leaf that + // skips the charge lets one value cost `budget * items` string copies. if (state.remainingNodes <= 0) { return value } @@ -631,10 +627,6 @@ function truncateValue( // its strings at full length. return CIRCULAR_VALUE } - // Unlike a cycle, these two are bounded by the encoder as well: it stops at the - // same depth, charges the same values — nullish leaves free on both sides — and - // spends one budget across the whole bag rather than one per attribute, so it - // runs out no later than this walk and marks whatever this one left unbounded. if (state.remainingNodes <= 0 || depth >= MAX_JSON_SAFE_VALUE_DEPTH) { return value } From 8997a166b9de13683efd953d1f1aa64423731dbf Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Thu, 3 Sep 2026 14:55:21 -0400 Subject: [PATCH 14/22] fix(traces): recheck consent between span batches A drain sends one batch per loop iteration, so the user could opt out while a batch was in flight and the batches behind it would still export. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01L8eFZB35NExUZyq5hGgh43 --- packages/core/src/traces/index.spec.ts | 48 ++++++++++++++++++++++++++ packages/core/src/traces/index.ts | 35 ++++++++++++++----- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 80e1764533..47d6fb3b15 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -1938,6 +1938,54 @@ describe('PostHogTraces', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('2 span(s)')) expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('the user has opted out')) }) + + it('stops draining a backlog when optOut() lands while a batch is in flight', async () => { + const instance = createMockInstance() + instance._sendTracesBatch = vi.fn(() => + Promise.resolve({ kind: 'retry-later' as const, error: new Error('down') }) + ) + const traces = createTraces({ maxExportBatchSize: 2 }, instance) + context = { distinctId: 'alice', sessionId: 'session-1' } + for (let i = 0; i < 6; i++) { + traces.startSpan(`span-${i}`).end() + await flushMicrotasks() + } + expect(instance._sendTracesBatch).toHaveBeenCalledTimes(1) + + instance._sendTracesBatch.mockImplementation(() => + Promise.resolve().then(() => { + instance.optedOut = true + return { kind: 'ok' as const } + }) + ) + await traces.flush() + + expect(instance._sendTracesBatch).toHaveBeenCalledTimes(2) + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('4 span(s)')) + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('the user has opted out')) + }) + + it('stops draining a backlog when the client is disabled while a batch is in flight', async () => { + const instance = createMockInstance() + instance._sendTracesBatch = vi.fn(() => + Promise.resolve({ kind: 'retry-later' as const, error: new Error('down') }) + ) + const traces = createTraces({ maxExportBatchSize: 2 }, instance) + for (let i = 0; i < 6; i++) { + traces.startSpan(`span-${i}`).end() + await flushMicrotasks() + } + + instance._sendTracesBatch.mockImplementation(() => + Promise.resolve().then(() => { + instance.isDisabled = true + return { kind: 'ok' as const } + }) + ) + await traces.flush() + + expect(instance._sendTracesBatch).toHaveBeenCalledTimes(2) + }) }) describe('drop accounting', () => { diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 4ce66257d6..4c71ec27bc 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -712,21 +712,31 @@ export class PostHogTraces { return encoded } + /** + * Discards the queue when consent has been withdrawn, returning how many spans + * it dropped. Spans carry `posthogDistinctId` and `sessionId`, so anything still + * queued when the user opts out must not be exported. + */ + private _discardQueueIfConsentWithdrawn(): number { + if (!this._instance.isDisabled && !this._instance.optedOut) { + return 0 + } + const discarded = this._queue.length + this._queue = [] + this._recordDrop(discarded, 'the user has opted out') + this._warnAboutDrops() + return discarded + } + /** Returns how many spans it removed from the queue, sent or dropped. */ private async _flushInner(): Promise { if (!this._queue.length) { return 0 } - // Consent can flip between a span being queued and this pass running. Spans - // carry `posthogDistinctId` and `sessionId`, so anything still queued when - // the user opts out must be discarded rather than exported. - if (this._instance.isDisabled || this._instance.optedOut) { - const discarded = this._queue.length - this._queue = [] - this._recordDrop(discarded, 'the user has opted out') - this._warnAboutDrops() - return discarded + const discardedBeforeDrain = this._discardQueueIfConsentWithdrawn() + if (discardedBeforeDrain) { + return discardedBeforeDrain } // Bounded like span attributes: resource attributes are caller-supplied too, @@ -745,6 +755,13 @@ export class PostHogTraces { try { while (remaining > 0 && this._queue.length > 0) { + // Re-checked per batch: a send suspends, so the user can opt out while one + // batch is in flight and the batches behind it would still export. + const discardedMidDrain = this._discardQueueIfConsentWithdrawn() + if (discardedMidDrain) { + return removed + discardedMidDrain + } + // Floor at one, or a non-positive batch size loops forever on an empty batch. const cap = this._headBatchFailures > 0 From 39fb8610941cb04fec624057d2be71f84c0fe453 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 08:05:45 -0400 Subject: [PATCH 15/22] fix(traces): materialize a toJSON that resolves to nothing Keeping the object left the encoder to probe toJSON a second time, so a serializer that answered null under the bound could answer with a megabyte over it. Stores the string the encoder builds from the same result instead, which leaves the wire unchanged and gives it nothing left to re-probe. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/span.spec.ts | 36 +++++++++++++++++++++------ packages/core/src/traces/span.ts | 10 +++++++- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index dc4feef046..d10fd83add 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -447,19 +447,41 @@ describe('PostHogSpan', () => { span.setAttribute('payload', { inner: new Redacted('S'.repeat(50)) } as any) span.end() - expect((ended[0].attributes.payload as any).inner).toBeInstanceOf(Redacted) + // The string the encoder builds from the same `null`, so the wire is + // unchanged, and the secret is nowhere in what the span kept. + expect((ended[0].attributes.payload as any).inner).toBe('null') + expect(JSON.stringify(ended[0].attributes)).not.toContain('S') }) - it('keeps a toJSON that resolves to nothing as the object that defines it', () => { - // Replacing it would spend a cap slot on an attribute encoding to nothing, - // and would drop an invalid Date the encoder still describes. - const ghost = { toJSON: () => undefined } + it('materializes a toJSON that resolves to nothing, so a second call cannot answer differently', () => { + // Keeping the object itself left the encoder to probe toJSON again. A + // serializer that answered `null` here could answer with a megabyte + // there, past the bound entirely. + let calls = 0 + const stateful = { + toJSON: () => { + calls++ + return calls === 1 ? null : 'x'.repeat(100) + }, + } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('doc', stateful as any) + span.end() + + expect(calls).toBe(1) + expect(ended[0].attributes.doc).toBe('null') + }) + + it('describes a toJSON resolving to undefined the way the encoder would', () => { const span = createSpan({ maxAttributeValueLength: 4 }) - span.setAttribute('ghost', ghost as any) + span.setAttribute('ghost', { toJSON: () => undefined } as any) span.end() - expect(ended[0].attributes.ghost).toBe(ghost) + // Not trimmed to the bound: this is the SDK's own marker, like + // `[Circular]`, and `unde` reads as nothing at all. + expect(ended[0].attributes.ghost).toBe('undefined') }) it('bounds event attributes too', () => { diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 9726f1a629..3cc6f98b94 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -648,7 +648,15 @@ function truncateValue( // Resolving to nothing is the value's answer. Walking its keys anyway // would build a plain object the encoder no longer treats as // self-describing, putting the internals of a redacted value on the wire. - return isNullish(resolved.value) ? value : truncateValue(resolved.value, maxLength, state, depth + 1) + // Stored as the string the encoder builds from that same nullish result + // rather than as the value itself: the encoder probes `toJSON` a second + // time, so one that answers `null` here is free to answer with a megabyte + // there, past the bound this walk exists to apply. Left unbounded like the + // other markers — nine characters at most, and trimming it to `unde` would + // only make it unreadable. + return isNullish(resolved.value) + ? String(resolved.value) + : truncateValue(resolved.value, maxLength, state, depth + 1) } if (isArray(value)) { // Only the items the encoder will emit are walked; it stops at the same From b4f08b739d7cdbb1864894e1af74d57b223370b6 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 08:05:50 -0400 Subject: [PATCH 16/22] fix(traces): take the default for a fractional numeric option Flooring made maxAttributesPerSpan: 1.5 resolve to 1, capping a span an order of magnitude below what the caller wrote and saying nothing. Every numeric traces option now falls back the way the spec describes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/config.spec.ts | 20 +++++++++++++++++--- packages/core/src/traces/config.ts | 6 +++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/core/src/traces/config.spec.ts b/packages/core/src/traces/config.spec.ts index e589f2192b..5237a0d52d 100644 --- a/packages/core/src/traces/config.spec.ts +++ b/packages/core/src/traces/config.spec.ts @@ -6,6 +6,11 @@ describe('resolveTracesConfig', () => { ['zero', 0], ['negative', -1], ['not a number', NaN], + // Floored, these read as 1, 2 and 3 — caps an order of magnitude below what + // the caller wrote, applied silently. + ['a fraction', 1.5], + ['a large fraction', 200.5], + ['infinity', Infinity], ])('falls back to the default per-span caps when given %s', (_label, value) => { const resolved = resolveTracesConfig({ maxAttributesPerSpan: value, @@ -97,14 +102,23 @@ describe('resolveTracesConfig', () => { } ) - it('floors a fractional batch size to an integer', () => { - expect(resolveTracesConfig({ maxExportBatchSize: 10.9 }).maxExportBatchSize).toBe(10) + it('takes the default for a fractional batch size rather than flooring it', () => { + // Every numeric knob resolves the same way, so a fraction is a value the + // caller did not mean rather than one to round down behind their back. + expect(resolveTracesConfig({ maxExportBatchSize: 10.9 }).maxExportBatchSize).toBe(512) }) - it.each([0, -1, Number.NaN])('falls back to the default for an unusable flushIntervalMs (%p)', (value) => { + it.each([0, -1, 1.5, Number.NaN])('falls back to the default for an unusable flushIntervalMs (%p)', (value) => { expect(resolveTracesConfig({ flushIntervalMs: value }).flushIntervalMs).toBe(5000) }) + it.each([0.5, 10_000.5])('falls back to the defaults for fractional live-span bounds (%p)', (value) => { + expect(resolveTracesConfig({ maxLiveSpans: value, maxSpanAgeMs: value })).toMatchObject({ + maxLiveSpans: 10_000, + maxSpanAgeMs: 3_600_000, + }) + }) + it('keeps the queue at least as large as the export batch', () => { // A queue smaller than the flush trigger would stop the depth-based flush // from ever firing. diff --git a/packages/core/src/traces/config.ts b/packages/core/src/traces/config.ts index a7b453c4e5..dc60973a43 100644 --- a/packages/core/src/traces/config.ts +++ b/packages/core/src/traces/config.ts @@ -30,9 +30,13 @@ const DEFAULT_MAX_SPAN_AGE_MS = 3_600_000 /** * Coerces a caller-supplied positive-integer option. `0`, a negative, or `NaN` * reaching the export loop would stall it. + * + * A fraction takes the default rather than being floored: these are documented + * as positive integers, and silently reading `maxAttributesPerSpan: 1.5` as `1` + * caps a span an order of magnitude below what the caller wrote. */ function positiveInteger(value: number | undefined, fallback: number): number { - return typeof value === 'number' && Number.isFinite(value) && value >= 1 ? Math.floor(value) : fallback + return typeof value === 'number' && Number.isInteger(value) && value >= 1 ? value : fallback } const IDENTITY_KEYS = ['service.name', 'service.version', 'deployment.environment'] as const From 28883b011b951e6c5ce3e192ec4570850042c948 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 08:06:19 -0400 Subject: [PATCH 17/22] fix(traces): drop a beforeSpanSend result missing a required field Carrying attributes and events was the whole shape check, so a hook returning only those two exported a span named unknown at a fallback time with no join keys, silently. A record missing any field the public SpanRecord requires is now a counted drop, as the rest of the hook contract already is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/index.spec.ts | 34 ++++++++++++++++++++++++++ packages/core/src/traces/index.ts | 24 ++++++++++++------ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 47d6fb3b15..08f1e372b6 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -1230,6 +1230,13 @@ describe('PostHogTraces', () => { ['attributes replaced with an array', (span: SpanRecord) => ({ ...span, attributes: ['a'] as never })], ['events replaced with null', (span: SpanRecord) => ({ ...span, events: null as never })], ['an async hook returning a promise', (span: SpanRecord) => Promise.resolve(span) as never], + // Carrying both collections was the whole shape check, so these reached + // the wire as a span named `unknown` at a fallback time with no join keys. + ['only the two collections', () => ({ attributes: {}, events: [] }) as never], + ['no name', (span: SpanRecord) => ({ ...span, name: undefined as never })], + ['no kind', (span: SpanRecord) => ({ ...span, kind: undefined as never })], + ['no start time', (span: SpanRecord) => ({ ...span, startTime: undefined as never })], + ['no end time', (span: SpanRecord) => ({ ...span, endTime: undefined as never })], ])('drops the span when the hook returns %s', async (_label, beforeSpanSend) => { // Repairing these would export a nameless span carrying no join keys. const traces = createTraces({ beforeSpanSend: [beforeSpanSend] }) @@ -1239,6 +1246,33 @@ describe('PostHogTraces', () => { expect(sentSpans()).toHaveLength(0) }) + it('counts an incomplete hook result as a drop rather than losing it silently', async () => { + const traces = createTraces({ beforeSpanSend: [() => ({ attributes: {}, events: [] }) as never] }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('beforeSpanSend returned an unusable record')) + }) + + it('still exports a span whose required fields the hook left in place', async () => { + // The shape check reads presence, so an ordinary scrub is untouched by it. + const traces = createTraces({ + beforeSpanSend: [ + (span) => { + delete span.attributes.secret + return span + }, + ], + }) + traces.startSpan('checkout', { attributes: { secret: 'shh', keep: 1 } }).end() + await traces.flush() + + expect(sentSpans()).toHaveLength(1) + expect(sentSpans()[0].name).toBe('checkout') + expect(sentSpans()[0].attributes!.map((a) => a.key)).toContain('keep') + expect(sentSpans()[0].attributes!.map((a) => a.key)).not.toContain('secret') + }) + it('exports the span when the hook status message refuses to stringify', async () => { // The encoder downstream only marks the field, so coercing here must not // be the thing that costs the span. diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index 0cdf54bda8..bd0c58ef38 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -76,16 +76,25 @@ interface SpanIdentity { } /** - * Whether a `beforeSpanSend` return value still carries the two collections the - * rest of the pipeline reads. An array is rejected for `attributes`: it would - * encode as `{ "0": ... }` rather than fail. + * Whether a `beforeSpanSend` return value still carries every field the public + * `SpanRecord` declares as required. An array is rejected for `attributes`: it + * would encode as `{ "0": ... }` rather than fail. + * + * Presence, not usability: a field that is there but holds the wrong type is a + * hook editing a real record badly, and the sanitising below is what answers + * that. A field that is absent means the hook returned something that was never + * a span record, and the fallbacks would dress it up as one. */ function isSpanRecordShape(record: SpanRecord): boolean { return ( !!record.attributes && typeof record.attributes === 'object' && !Array.isArray(record.attributes) && - Array.isArray(record.events) + Array.isArray(record.events) && + record.name !== undefined && + record.kind !== undefined && + record.startTime !== undefined && + record.endTime !== undefined ) } @@ -572,9 +581,10 @@ export class PostHogTraces { droppedEventsCount: originalDropped.events, } current = rebuilt - // A value missing either collection is not a span record — an `async` - // hook returns a Promise, truthy and `undefined` for every field. Filling - // the gaps in would export a nameless span carrying no person or session. + // A value missing a required field is not a span record — an `async` hook + // returns a Promise, truthy and `undefined` for every field. Filling the + // gaps in would export a span named `unknown` at a fallback time carrying + // no person or session, joinable to nothing and silent about it. if (!isSpanRecordShape(current)) { this._logger.debug('beforeSpanSend did not return a span record; dropping the span') this._recordDrop(1, 'beforeSpanSend returned an unusable record') From da5384f4b263ad87398be2af5f3a2083ca724d42 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 08:06:30 -0400 Subject: [PATCH 18/22] fix(traces): give a later beforeSpanSend hook the real span identity An earlier hook that forged an id and froze what it returned refused the restoring writes, so the next hook in the chain sampled on the forged id. Hands it a corrected view built from the record's own descriptors, which keeps the prototype and the keys the hook returned. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/index.spec.ts | 44 ++++++++++++++++++++++++++ packages/core/src/traces/index.ts | 35 +++++++++++++++++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 08f1e372b6..81e044ef7e 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -1273,6 +1273,50 @@ describe('PostHogTraces', () => { expect(sentSpans()[0].attributes!.map((a) => a.key)).not.toContain('secret') }) + it('gives a later hook the real identity after an earlier one froze a forged record', async () => { + // The export reads the snapshot either way, but a hook that samples or + // routes on an id must not see one an earlier hook invented. + const seen: { traceId: string; spanId: string }[] = [] + const traces = createTraces({ + beforeSpanSend: [ + (span) => Object.freeze({ ...span, traceId: '0'.repeat(32), spanId: 'f'.repeat(16) }), + (span) => { + seen.push({ traceId: span.traceId, spanId: span.spanId }) + return span + }, + ], + }) + traces.startSpan('checkout').end() + await traces.flush() + + expect(sentSpans()).toHaveLength(1) + expect(seen[0].traceId).toBe(sentSpans()[0].traceId) + expect(seen[0].spanId).toBe(sentSpans()[0].spanId) + expect(seen[0].traceId).not.toBe('0'.repeat(32)) + }) + + it('leaves the rest of a frozen forged record readable to the next hook', async () => { + // The corrected view is built from the record's own descriptors, so a hook + // reading anything but identity sees exactly what the previous one returned. + let seen: SpanRecord | undefined + const traces = createTraces({ + beforeSpanSend: [ + (span) => Object.freeze({ ...span, name: 'renamed', traceId: '0'.repeat(32) }), + (span) => { + seen = span + return span + }, + ], + }) + traces.startSpan('checkout', { attributes: { keep: 1 } }).end() + await traces.flush() + + expect(seen!.name).toBe('renamed') + expect(seen!.attributes.keep).toBe(1) + expect(Object.keys(seen!)).toContain('name') + expect(sentSpans()[0].name).toBe('renamed') + }) + it('exports the span when the hook status message refuses to stringify', async () => { // The encoder downstream only marks the field, so coercing here must not // be the thing that costs the span. diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index bd0c58ef38..e9de1e6343 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -105,6 +105,35 @@ function restoreField(record: SpanIdentity, field: } } +/** + * A stand-in for a record whose identity could not be written back, carrying the + * original ids and everything else the hook returned. + * + * Built from the descriptors rather than spread so a class instance keeps its + * prototype — `instanceof` and a field exposed as a prototype getter both still + * answer — and so `Object.keys` reads what it read before. Only the four + * identity descriptors are replaced, which is what makes the copy writable where + * the original was frozen. + */ +function withRestoredIdentity(hooked: HookSpanRecord, original: SpanIdentity): HookSpanRecord { + try { + const descriptors = Object.getOwnPropertyDescriptors(hooked) as Record + for (const field of ['traceId', 'spanId', 'parentSpanId', 'traceState'] as const) { + descriptors[field] = { + value: original[field], + enumerable: true, + writable: true, + configurable: true, + } + } + return Object.create(Object.getPrototypeOf(hooked) as object | null, descriptors) as HookSpanRecord + } catch { + // A hostile descriptor read. The export still uses the snapshot, so this + // costs the next hook a correct id rather than the span. + return hooked + } +} + interface ParentContext { traceId: string parentSpanId?: string @@ -669,7 +698,11 @@ export class PostHogTraces { // drop tracestate, which is not part of the record the hook is handed. restoreField(hooked, 'traceState', original.traceState) } catch { - // Frozen. The rebuild reads the identity from the snapshot regardless. + // Frozen, so the writes above were refused and this record still carries + // whatever identity the hook forged. The export reads the snapshot either + // way, but the next hook in the chain reads this — and would sample or + // route on a forged id, which identity immutability exists to prevent. + return withRestoredIdentity(hooked, original) } return hooked } From 3f95aeb3cf8337a62942897b15fb3f9c808eefa9 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 08:07:40 -0400 Subject: [PATCH 19/22] test(traces): pin the toJSON bound through buildOtlpSpan The second call is the encoder's, so the guarantee is worth asserting past the record. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/span.spec.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index d10fd83add..f0670b9a00 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -1,4 +1,5 @@ import { NOOP_SPAN, PostHogSpan, describeError, truncateAttributeValue } from './span' +import { buildOtlpSpan } from './otlp' import type { SpanInit } from './span' import type { SpanRecord } from './types' import type { Logger } from '../types' @@ -473,6 +474,29 @@ describe('PostHogSpan', () => { expect(ended[0].attributes.doc).toBe('null') }) + it('keeps a stateful toJSON bounded through to the encoded span', () => { + // End to end, because the second call is the encoder's: what the span + // stored has to leave it nothing to call. + let calls = 0 + const stateful = { + toJSON: () => { + calls++ + return calls === 1 ? null : 'x'.repeat(100) + }, + } + const span = createSpan({ maxAttributeValueLength: 4 }) + + span.setAttribute('doc', stateful as any) + span.end() + const encoded = buildOtlpSpan(ended[0]) + + expect(calls).toBe(1) + expect(encoded.attributes?.find((attribute) => attribute.key === 'doc')).toEqual({ + key: 'doc', + value: { stringValue: 'null' }, + }) + }) + it('describes a toJSON resolving to undefined the way the encoder would', () => { const span = createSpan({ maxAttributeValueLength: 4 }) From b8425c80848288a72c95cd2127cd64aedd6cb844 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 10:54:26 -0400 Subject: [PATCH 20/22] fix(traces): reserve exception slots by provenance, not event name The reserve is for what the SDK records on your behalf, so an event named `exception` by the caller was claiming it too. Marks SDK-recorded events with an internal symbol the hook cannot see and the wire cannot carry, and keys both enforcement points on that. Removes eventNameBound with it: the name no longer decides anything, so it no longer needs a floor. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/index.spec.ts | 88 ++++++++++++++++++++++++++ packages/core/src/traces/index.ts | 8 +-- packages/core/src/traces/span.spec.ts | 41 ++++++++++-- packages/core/src/traces/span.ts | 75 +++++++++++++--------- packages/types/src/traces.ts | 10 +-- 5 files changed, 175 insertions(+), 47 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 81e044ef7e..5aa6833094 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -1317,6 +1317,94 @@ describe('PostHogTraces', () => { expect(sentSpans()[0].name).toBe('renamed') }) + it('keeps the exception reserve through a hook that edits events in place', async () => { + // The re-apply after the hook has to reach the same verdict as the span + // writer, and it reads a mark the hook never sees. + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + for (const event of span.events) { + event.attributes = { ...event.attributes, scrubbed: true } + } + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step-0') + span.recordException(new Error('boom')) + span.end() + await traces.flush() + + expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0', 'exception']) + }) + + it('does not give the reserve to an exception-named event a hook appended', async () => { + // The hook writes plain records carrying no mark, so an event it invents + // cannot claim slots the SDK reserved for what it recorded itself. + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + span.events.push({ name: 'exception', timestamp: Date.now() }) + span.events.push({ name: 'exception', timestamp: Date.now() }) + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step-0') + span.end() + await traces.flush() + + expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0']) + }) + + it('drops a recorded exception when a hook rebuilds its events from scratch', async () => { + // The documented residual of keying on a mark rather than the name: a hook + // that constructs fresh event objects field by field drops it, and the + // event falls back to an ordinary slot. Fail-safe — the reserve is never + // handed to an event the SDK did not record — but worth pinning so the + // behaviour is deliberate rather than discovered. + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + span.events = span.events.map((event) => ({ name: event.name, timestamp: event.timestamp })) + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step-0') + span.recordException(new Error('boom')) + span.end() + await traces.flush() + + expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0']) + }) + + it('keeps the reserve through a hook that spreads its events', async () => { + // A spread carries the mark, so the common "copy and edit" shape is safe. + const traces = createTraces({ + maxEventsPerSpan: 1, + beforeSpanSend: [ + (span) => { + span.events = span.events.map((event) => ({ ...event })) + return span + }, + ], + }) + const span = traces.startSpan('checkout') + span.addEvent('step-0') + span.recordException(new Error('boom')) + span.end() + await traces.flush() + + expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0', 'exception']) + }) + it('exports the span when the hook status message refuses to stringify', async () => { // The encoder downstream only marks the field, so coercing here must not // be the thing that costs the span. diff --git a/packages/core/src/traces/index.ts b/packages/core/src/traces/index.ts index e9de1e6343..a5d385e58c 100644 --- a/packages/core/src/traces/index.ts +++ b/packages/core/src/traces/index.ts @@ -14,7 +14,6 @@ import { PostHogSpan, applySpanLimits, describeError, - eventNameBound, inertSpan, monotonicNow, runWithActiveSpan, @@ -639,12 +638,7 @@ export class PostHogTraces { try { sanitizedEvents.push({ ...event, - name: sanitizeName( - event.name, - 'Span event name', - eventNameBound(this._config.maxAttributeValueLength), - this._logger - ), + name: sanitizeName(event.name, 'Span event name', this._config.maxAttributeValueLength, this._logger), timestamp: resolveSuppliedTime(event.timestamp, current.startTime, 'event timestamp', this._logger), }) } catch { diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index f0670b9a00..d9dfac14ec 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -226,6 +226,33 @@ describe('PostHogSpan', () => { expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1']) expect(ended[0].droppedEventsCount).toBe(3) }) + + it('does not give the reserve to a caller who names their own event exception', () => { + // The reserve is for what the SDK records on your behalf. Keying it on the + // name handed it to anyone who happened to use that name. + const span = createSpan({ maxEvents: 2 }) + for (let i = 0; i < 7; i++) { + span.addEvent('exception', { mine: i }) + } + span.end() + + expect(ended[0].events).toHaveLength(2) + expect(ended[0].droppedEventsCount).toBe(5) + }) + + it('gives the reserve to a recorded exception alongside a caller using the same name', () => { + // Both land on a full span; only the one the SDK recorded draws on it. + const span = createSpan({ maxEvents: 1 }) + span.addEvent('step-0') + span.addEvent('exception', { mine: true }) + span.recordException(new Error('boom')) + span.end() + + expect(ended[0].events).toHaveLength(2) + expect(ended[0].events[0].name).toBe('step-0') + expect(ended[0].events[1].attributes?.['exception.type']).toBe('Error') + expect(ended[0].droppedEventsCount).toBe(1) + }) }) describe('attribute store hygiene', () => { @@ -629,9 +656,10 @@ describe('PostHogSpan', () => { expect(Object.getOwnPropertyDescriptor(payload, '__proto__')?.value).toEqual({ body: 'abcd' }) }) - it('never cuts the reserved exception event name', () => { - // The reserve compares against the whole constant, so a bound short enough - // to cut it silently dropped the exception event it exists to keep. + it('keeps the reserve when the value bound cuts the event name', () => { + // The reserve keys on what the SDK recorded, not on the name, so a bound + // short enough to trim `exception` no longer decides whether the event + // survives. The name is bounded like any other, and the event is kept. const span = createSpan({ maxAttributeValueLength: 8, maxEvents: 2 }) span.addEvent('a') @@ -639,13 +667,14 @@ describe('PostHogSpan', () => { span.recordException(new Error('boom')) span.end() - expect(ended[0].events.map((e) => e.name)).toEqual(['a', 'b', 'exception']) + expect(ended[0].events).toHaveLength(3) + expect(ended[0].events[2].name).toBe('exceptio') + expect(ended[0].events[2].attributes?.['exception.type']).toBe('Error') }) it('bounds a span name and an event name, like a status message', () => { // A name built from a URL is caller-controlled, and one large enough takes - // the span past the ingestion body limit. The bound is 12 rather than 4 - // because an event name is floored at the reserved `exception`. + // the span past the ingestion body limit. const span = createSpan({ maxAttributeValueLength: 12 }) span.updateName('abcdefghijklmnop') diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 3cc6f98b94..0bf60fd406 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -180,12 +180,12 @@ export class PostHogSpan implements Span { * included — the reserve is what an exception falls back on once the cap is * spent, not a smaller budget it is confined to. */ - private _claimEventSlot(name: string): boolean { + private _claimEventSlot(sdkException: boolean): boolean { if (this._userEventCount < this._maxEvents) { this._userEventCount++ return true } - if (isExceptionEvent(name) && this._exceptionEventCount < MAX_EXCEPTION_EVENTS_PER_SPAN) { + if (sdkException && this._exceptionEventCount < MAX_EXCEPTION_EVENTS_PER_SPAN) { this._exceptionEventCount++ return true } @@ -193,26 +193,33 @@ export class PostHogSpan implements Span { } addEvent(name: string, attributes?: SpanAttributes, timestamp?: SpanTimeInput): this { + return this._pushEvent(name, attributes, timestamp, false) + } + + /** + * The shared body of `addEvent` and `recordException`. `sdkException` is what + * the reserve keys on, so it is set only where the SDK records an exception + * itself and can never be reached through the public surface. + */ + private _pushEvent( + name: string, + attributes: SpanAttributes | undefined, + timestamp: SpanTimeInput | undefined, + sdkException: boolean + ): this { if (this._mutable('addEvent')) { - // Sanitised before the bucket check, so the name deciding the bucket is the - // one that ends up on the record. - const eventName = sanitizeName( - name, - 'Span event name', - eventNameBound(this._maxAttributeValueLength), - this._logger - ) - if (!this._claimEventSlot(eventName)) { + if (!this._claimEventSlot(sdkException)) { this._droppedEvents++ return this } this._events.push({ - name: eventName, + name: sanitizeName(name, 'Span event name', this._maxAttributeValueLength, this._logger), timestamp: resolveSuppliedTime(timestamp, this._now(), 'event timestamp', this._logger), // Copied so a caller reusing one object across events can't mutate a recorded one. ...(attributes && { attributes: truncateAttributes(assignUserAttributes({}, attributes), this._maxAttributeValueLength), }), + ...(sdkException && { [SDK_EXCEPTION_EVENT]: true }), }) } return this @@ -244,11 +251,16 @@ export class PostHogSpan implements Span { return this } const { type, message, stack } = describeError(error) - this.addEvent(EXCEPTION_EVENT_NAME, { - 'exception.type': type, - 'exception.message': message, - ...(stack && { 'exception.stacktrace': stack }), - }) + this._pushEvent( + EXCEPTION_EVENT_NAME, + { + 'exception.type': type, + 'exception.message': message, + ...(stack && { 'exception.stacktrace': stack }), + }, + undefined, + true + ) // recordException is itself an explicit call, so it follows last-write-wins // rather than deferring to an earlier `ok`. return this.setStatus('error', message) @@ -318,18 +330,19 @@ export class PostHogSpan implements Span { const EXCEPTION_EVENT_NAME = 'exception' /** - * The value bound as it applies to an event name, floored at the reserved name. + * Marks an event the SDK recorded itself, which is what the reserve below is + * for. Symbol-keyed rather than a field: `Object.keys`, `for...in` and + * `JSON.stringify` all skip it, so it is invisible to `beforeSpanSend` and + * cannot reach the wire, while an object spread still carries it — which is how + * it survives the re-sanitising pass that runs after the hook. * - * `isExceptionEvent` compares against the whole constant, so a bound short - * enough to cut it would silently disable the exception reserve and drop the - * event the reserve exists to keep. + * The alternative, matching on the event name, hands the reserve to a caller who + * names their own event `exception` too. */ -export function eventNameBound(maxAttributeValueLength: number): number { - return Math.max(maxAttributeValueLength, EXCEPTION_EVENT_NAME.length) -} +const SDK_EXCEPTION_EVENT = Symbol('posthog.sdkExceptionEvent') /** - * How many `exception` events may sit past the event cap. + * How many SDK-recorded exception events may sit past the event cap. * * A span that fills its events and then throws would otherwise lose the only * record of why it failed — the span you most want to read. Fixed and small @@ -340,11 +353,13 @@ export function eventNameBound(maxAttributeValueLength: number): number { const MAX_EXCEPTION_EVENTS_PER_SPAN = 4 /** - * Identified by name, the same trade-off the attribute exemption makes: a - * caller who names their own event `exception` gets the exemption too. + * Whether the SDK recorded this event, and so whether it may draw on the + * reserve. A hook that rebuilds its events into fresh objects drops the mark, + * and those events fall back to ordinary slots — the safe direction, since the + * reserve is never handed to an event the SDK did not record. */ -function isExceptionEvent(name: string): boolean { - return name === EXCEPTION_EVENT_NAME +function isSdkExceptionEvent(event: SpanEventRecord): boolean { + return (event as { [SDK_EXCEPTION_EVENT]?: boolean })[SDK_EXCEPTION_EVENT] === true } /** A value as its string form, or the encoder's marker when it refuses to produce one. */ @@ -442,7 +457,7 @@ export function applySpanLimits( for (const event of record.events) { if (keptEvents < maxEvents) { keptEvents++ - } else if (isExceptionEvent(event.name) && keptExceptions < MAX_EXCEPTION_EVENTS_PER_SPAN) { + } else if (isSdkExceptionEvent(event) && keptExceptions < MAX_EXCEPTION_EVENTS_PER_SPAN) { keptExceptions++ } else { droppedEvents++ diff --git a/packages/types/src/traces.ts b/packages/types/src/traces.ts index 4bdd8c1b3a..13a1d3d41a 100644 --- a/packages/types/src/traces.ts +++ b/packages/types/src/traces.ts @@ -317,10 +317,12 @@ export interface TracesConfig { * and later ones are dropped, with the number dropped reported on the * exported span. * - * Once the cap is spent, up to four more `exception` events are still - * accepted, so a span that fills its events and then throws still carries the - * exception rather than only an `error` status. Below the cap an exception - * is an ordinary event and spends an ordinary slot. + * Once the cap is spent, up to four more exception events recorded by the + * SDK — through `recordException` or a scoped helper whose callback threw — + * are still accepted, so a span that fills its events and then throws still + * carries the exception rather than only an `error` status. Below the cap + * such an event is ordinary and spends an ordinary slot, and an event you + * add yourself never draws on the reserve whatever you name it. * * @default 128 */ From 0bb43aa5589c44c1a23b5b157e53d28d8c854d22 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 11:18:27 -0400 Subject: [PATCH 21/22] fix(traces): make maxEventsPerSpan an absolute cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops the four reserved slots for exception events. The reserve bought a case we cannot show occurs — a span holding 128 events that then throws — at the cost of the most intricate code in this change, which had already carried one bug. The cap now matches the spec's number exactly. A span that fills its events and then throws keeps its error status and reports the loss through droppedEventsCount, so the case is measurable once traces ships and the reserve can be added back additively if it turns out to matter. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc --- packages/core/src/traces/index.spec.ts | 86 +++----------------- packages/core/src/traces/span.spec.ts | 74 +++++------------ packages/core/src/traces/span.ts | 105 ++++--------------------- packages/types/src/traces.ts | 10 +-- 4 files changed, 50 insertions(+), 225 deletions(-) diff --git a/packages/core/src/traces/index.spec.ts b/packages/core/src/traces/index.spec.ts index 5aa6833094..6153550c93 100644 --- a/packages/core/src/traces/index.spec.ts +++ b/packages/core/src/traces/index.spec.ts @@ -1087,9 +1087,10 @@ describe('PostHogTraces', () => { expect(sentSpans()[0].attributes).toBeUndefined() }) - it('keeps the exception event when the hook pushes past the event cap', async () => { - // The re-apply used to slice to the first `maxEvents`, and an exception - // event is the last thing on a span that threw — exactly what a slice cuts. + it('keeps the error status when the event cap costs the exception event', async () => { + // The status is set independently of the event, so a span whose exception + // event did not fit still exports as failed and still counts the loss. + // That pair is what makes the case findable once traces is live. const traces = createTraces({ maxEventsPerSpan: 1, beforeSpanSend: [ @@ -1106,8 +1107,8 @@ describe('PostHogTraces', () => { await traces.flush() const [sent] = sentSpans() - expect(sent.events!.map((event) => event.name)).toEqual(['step', 'exception']) - expect(sent.droppedEventsCount).toBe(1) + expect(sent.events!.map((event) => event.name)).toEqual(['step']) + expect(sent.droppedEventsCount).toBe(2) expect(sent.status).toEqual({ code: 2, message: 'boom' }) }) @@ -1317,38 +1318,15 @@ describe('PostHogTraces', () => { expect(sentSpans()[0].name).toBe('renamed') }) - it('keeps the exception reserve through a hook that edits events in place', async () => { - // The re-apply after the hook has to reach the same verdict as the span - // writer, and it reads a mark the hook never sees. + it('applies the event cap to what a hook leaves behind', async () => { + // A hook can append events or rewrite them, neither of which goes through + // `addEvent`, so the cap has to be re-applied to whatever it returns. const traces = createTraces({ maxEventsPerSpan: 1, beforeSpanSend: [ (span) => { - for (const event of span.events) { - event.attributes = { ...event.attributes, scrubbed: true } - } - return span - }, - ], - }) - const span = traces.startSpan('checkout') - span.addEvent('step-0') - span.recordException(new Error('boom')) - span.end() - await traces.flush() - - expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0', 'exception']) - }) - - it('does not give the reserve to an exception-named event a hook appended', async () => { - // The hook writes plain records carrying no mark, so an event it invents - // cannot claim slots the SDK reserved for what it recorded itself. - const traces = createTraces({ - maxEventsPerSpan: 1, - beforeSpanSend: [ - (span) => { - span.events.push({ name: 'exception', timestamp: Date.now() }) span.events.push({ name: 'exception', timestamp: Date.now() }) + span.events.push({ name: 'appended', timestamp: Date.now() }) return span }, ], @@ -1361,50 +1339,6 @@ describe('PostHogTraces', () => { expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0']) }) - it('drops a recorded exception when a hook rebuilds its events from scratch', async () => { - // The documented residual of keying on a mark rather than the name: a hook - // that constructs fresh event objects field by field drops it, and the - // event falls back to an ordinary slot. Fail-safe — the reserve is never - // handed to an event the SDK did not record — but worth pinning so the - // behaviour is deliberate rather than discovered. - const traces = createTraces({ - maxEventsPerSpan: 1, - beforeSpanSend: [ - (span) => { - span.events = span.events.map((event) => ({ name: event.name, timestamp: event.timestamp })) - return span - }, - ], - }) - const span = traces.startSpan('checkout') - span.addEvent('step-0') - span.recordException(new Error('boom')) - span.end() - await traces.flush() - - expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0']) - }) - - it('keeps the reserve through a hook that spreads its events', async () => { - // A spread carries the mark, so the common "copy and edit" shape is safe. - const traces = createTraces({ - maxEventsPerSpan: 1, - beforeSpanSend: [ - (span) => { - span.events = span.events.map((event) => ({ ...event })) - return span - }, - ], - }) - const span = traces.startSpan('checkout') - span.addEvent('step-0') - span.recordException(new Error('boom')) - span.end() - await traces.flush() - - expect(sentSpans()[0].events!.map((event) => event.name)).toEqual(['step-0', 'exception']) - }) - it('exports the span when the hook status message refuses to stringify', async () => { // The encoder downstream only marks the field, so coercing here must not // be the thing that costs the span. diff --git a/packages/core/src/traces/span.spec.ts b/packages/core/src/traces/span.spec.ts index d9dfac14ec..2039adc43a 100644 --- a/packages/core/src/traces/span.spec.ts +++ b/packages/core/src/traces/span.spec.ts @@ -172,30 +172,28 @@ describe('PostHogSpan', () => { expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('OK')) }) - describe('exception events past the event cap', () => { + describe('event cap', () => { const fillEvents = (span: PostHogSpan, count: number): void => { for (let i = 0; i < count; i++) { span.addEvent(`step-${i}`) } } - it('records an exception on a span that has filled its events', () => { - // Without its own budget the exception event arrives last, hits the cap and - // is dropped, leaving a span marked `error` with no record of why. + it('drops an exception event on a span that has filled its events', () => { + // The cap is absolute, so an exception arriving last is dropped like any + // other event. The span keeps its `error` status and `droppedEventsCount` + // reports the loss, which is what makes the case findable in production. const span = createSpan({ maxEvents: 2 }) - fillEvents(span, 3) + fillEvents(span, 2) span.recordException(new Error('boom')) span.end() - expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1', 'exception']) - expect(ended[0].events[2].attributes?.['exception.message']).toBe('boom') + expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1']) expect(ended[0].status).toEqual({ code: 'error', message: 'boom' }) expect(ended[0].droppedEventsCount).toBe(1) }) - it('spends an ordinary slot on an exception while the cap has room', () => { - // The reserve is a fallback past the cap, not a smaller budget exceptions - // are confined to: with room to spare, exceptions are ordinary events. + it('records an exception like any other event while the cap has room', () => { const span = createSpan({ maxEvents: 128 }) for (let i = 0; i < 20; i++) { span.recordException(new Error(`boom-${i}`)) @@ -206,30 +204,9 @@ describe('PostHogSpan', () => { expect(ended[0].droppedEventsCount).toBeUndefined() }) - it('bounds the reserve so recordException cannot grow a span without limit', () => { - const span = createSpan({ maxEvents: 1 }) - for (let i = 0; i < 7; i++) { - span.recordException(new Error(`boom-${i}`)) - } - span.end() - - // One ordinary slot, then the reserve of four. - expect(ended[0].events).toHaveLength(5) - expect(ended[0].droppedEventsCount).toBe(2) - }) - - it('does not let the reserve rescue ordinary events', () => { - const span = createSpan({ maxEvents: 2 }) - fillEvents(span, 5) - span.end() - - expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1']) - expect(ended[0].droppedEventsCount).toBe(3) - }) - - it('does not give the reserve to a caller who names their own event exception', () => { - // The reserve is for what the SDK records on your behalf. Keying it on the - // name handed it to anyone who happened to use that name. + it('does not let an exception-named event bypass the cap', () => { + // The cap counts events, not names: nothing about the name `exception` + // buys a slot, whoever wrote it. const span = createSpan({ maxEvents: 2 }) for (let i = 0; i < 7; i++) { span.addEvent('exception', { mine: i }) @@ -240,18 +217,13 @@ describe('PostHogSpan', () => { expect(ended[0].droppedEventsCount).toBe(5) }) - it('gives the reserve to a recorded exception alongside a caller using the same name', () => { - // Both land on a full span; only the one the SDK recorded draws on it. - const span = createSpan({ maxEvents: 1 }) - span.addEvent('step-0') - span.addEvent('exception', { mine: true }) - span.recordException(new Error('boom')) + it('drops ordinary events past the cap', () => { + const span = createSpan({ maxEvents: 2 }) + fillEvents(span, 5) span.end() - expect(ended[0].events).toHaveLength(2) - expect(ended[0].events[0].name).toBe('step-0') - expect(ended[0].events[1].attributes?.['exception.type']).toBe('Error') - expect(ended[0].droppedEventsCount).toBe(1) + expect(ended[0].events.map((event) => event.name)).toEqual(['step-0', 'step-1']) + expect(ended[0].droppedEventsCount).toBe(3) }) }) @@ -656,20 +628,14 @@ describe('PostHogSpan', () => { expect(Object.getOwnPropertyDescriptor(payload, '__proto__')?.value).toEqual({ body: 'abcd' }) }) - it('keeps the reserve when the value bound cuts the event name', () => { - // The reserve keys on what the SDK recorded, not on the name, so a bound - // short enough to trim `exception` no longer decides whether the event - // survives. The name is bounded like any other, and the event is kept. - const span = createSpan({ maxAttributeValueLength: 8, maxEvents: 2 }) + it('bounds an event name the SDK records like any other', () => { + const span = createSpan({ maxAttributeValueLength: 8, maxEvents: 4 }) - span.addEvent('a') - span.addEvent('b') span.recordException(new Error('boom')) span.end() - expect(ended[0].events).toHaveLength(3) - expect(ended[0].events[2].name).toBe('exceptio') - expect(ended[0].events[2].attributes?.['exception.type']).toBe('Error') + expect(ended[0].events[0].name).toBe('exceptio') + expect(ended[0].events[0].attributes?.['exception.type']).toBe('Error') }) it('bounds a span name and an event name, like a status message', () => { diff --git a/packages/core/src/traces/span.ts b/packages/core/src/traces/span.ts index 0bf60fd406..d423894fc2 100644 --- a/packages/core/src/traces/span.ts +++ b/packages/core/src/traces/span.ts @@ -68,7 +68,6 @@ export class PostHogSpan implements Span { private readonly _maxAttributeValueLength: number private _userAttributeCount = 0 private _userEventCount = 0 - private _exceptionEventCount = 0 private _droppedAttributes = 0 private _droppedEvents = 0 @@ -173,45 +172,17 @@ export class PostHogSpan implements Span { return this } - /** - * Reserves a slot for an event, or refuses when the span is full. - * - * Every event takes an ordinary slot while the cap has room, exception events - * included — the reserve is what an exception falls back on once the cap is - * spent, not a smaller budget it is confined to. - */ - private _claimEventSlot(sdkException: boolean): boolean { - if (this._userEventCount < this._maxEvents) { - this._userEventCount++ - return true - } - if (sdkException && this._exceptionEventCount < MAX_EXCEPTION_EVENTS_PER_SPAN) { - this._exceptionEventCount++ - return true - } - return false - } - addEvent(name: string, attributes?: SpanAttributes, timestamp?: SpanTimeInput): this { - return this._pushEvent(name, attributes, timestamp, false) - } - - /** - * The shared body of `addEvent` and `recordException`. `sdkException` is what - * the reserve keys on, so it is set only where the SDK records an exception - * itself and can never be reached through the public surface. - */ - private _pushEvent( - name: string, - attributes: SpanAttributes | undefined, - timestamp: SpanTimeInput | undefined, - sdkException: boolean - ): this { if (this._mutable('addEvent')) { - if (!this._claimEventSlot(sdkException)) { + // An exception the SDK records spends an ordinary slot like any other + // event. A span that fills its events and then throws therefore keeps its + // `error` status but loses the exception detail, which `droppedEventsCount` + // reports — enough to find the case in production if it turns out to occur. + if (this._userEventCount >= this._maxEvents) { this._droppedEvents++ return this } + this._userEventCount++ this._events.push({ name: sanitizeName(name, 'Span event name', this._maxAttributeValueLength, this._logger), timestamp: resolveSuppliedTime(timestamp, this._now(), 'event timestamp', this._logger), @@ -219,7 +190,6 @@ export class PostHogSpan implements Span { ...(attributes && { attributes: truncateAttributes(assignUserAttributes({}, attributes), this._maxAttributeValueLength), }), - ...(sdkException && { [SDK_EXCEPTION_EVENT]: true }), }) } return this @@ -251,16 +221,11 @@ export class PostHogSpan implements Span { return this } const { type, message, stack } = describeError(error) - this._pushEvent( - EXCEPTION_EVENT_NAME, - { - 'exception.type': type, - 'exception.message': message, - ...(stack && { 'exception.stacktrace': stack }), - }, - undefined, - true - ) + this.addEvent(EXCEPTION_EVENT_NAME, { + 'exception.type': type, + 'exception.message': message, + ...(stack && { 'exception.stacktrace': stack }), + }) // recordException is itself an explicit call, so it follows last-write-wins // rather than deferring to an earlier `ok`. return this.setStatus('error', message) @@ -329,39 +294,6 @@ export class PostHogSpan implements Span { const EXCEPTION_EVENT_NAME = 'exception' -/** - * Marks an event the SDK recorded itself, which is what the reserve below is - * for. Symbol-keyed rather than a field: `Object.keys`, `for...in` and - * `JSON.stringify` all skip it, so it is invisible to `beforeSpanSend` and - * cannot reach the wire, while an object spread still carries it — which is how - * it survives the re-sanitising pass that runs after the hook. - * - * The alternative, matching on the event name, hands the reserve to a caller who - * names their own event `exception` too. - */ -const SDK_EXCEPTION_EVENT = Symbol('posthog.sdkExceptionEvent') - -/** - * How many SDK-recorded exception events may sit past the event cap. - * - * A span that fills its events and then throws would otherwise lose the only - * record of why it failed — the span you most want to read. Fixed and small - * rather than configurable: this is a safety margin, not a tuning knob, and - * four covers a catch-retry-fail loop without letting `recordException` grow a - * span without limit. - */ -const MAX_EXCEPTION_EVENTS_PER_SPAN = 4 - -/** - * Whether the SDK recorded this event, and so whether it may draw on the - * reserve. A hook that rebuilds its events into fresh objects drops the mark, - * and those events fall back to ordinary slots — the safe direction, since the - * reserve is never handed to an event the SDK did not record. - */ -function isSdkExceptionEvent(event: SpanEventRecord): boolean { - return (event as { [SDK_EXCEPTION_EVENT]?: boolean })[SDK_EXCEPTION_EVENT] === true -} - /** A value as its string form, or the encoder's marker when it refuses to produce one. */ function safeString(value: unknown): string { try { @@ -446,23 +378,18 @@ export function applySpanLimits( record.droppedAttributesCount = nonNegativeCount(record.droppedAttributesCount) + droppedAttributes } - // Walked in order rather than sliced: an exception event is the last thing on - // a span that threw, so a plain slice would cut off the exemption the writer - // just granted. A hook can also append events or rewrite their attributes, - // neither of which goes through `addEvent`. + // Walked rather than sliced: a hook can append events or rewrite their + // attributes, neither of which goes through `addEvent`, so each one still + // needs its attributes bounded on the way past. let keptEvents = 0 - let keptExceptions = 0 let droppedEvents = 0 const events: SpanEventRecord[] = [] for (const event of record.events) { - if (keptEvents < maxEvents) { - keptEvents++ - } else if (isSdkExceptionEvent(event) && keptExceptions < MAX_EXCEPTION_EVENTS_PER_SPAN) { - keptExceptions++ - } else { + if (keptEvents >= maxEvents) { droppedEvents++ continue } + keptEvents++ if (event.attributes) { event.attributes = truncateAttributes({ ...event.attributes }, maxAttributeValueLength) } diff --git a/packages/types/src/traces.ts b/packages/types/src/traces.ts index 13a1d3d41a..eaf84e2309 100644 --- a/packages/types/src/traces.ts +++ b/packages/types/src/traces.ts @@ -317,12 +317,10 @@ export interface TracesConfig { * and later ones are dropped, with the number dropped reported on the * exported span. * - * Once the cap is spent, up to four more exception events recorded by the - * SDK — through `recordException` or a scoped helper whose callback threw — - * are still accepted, so a span that fills its events and then throws still - * carries the exception rather than only an `error` status. Below the cap - * such an event is ordinary and spends an ordinary slot, and an event you - * add yourself never draws on the reserve whatever you name it. + * The cap is absolute: an `exception` event the SDK records on your behalf + * spends an ordinary slot like any other, so a span that fills its events + * and then throws keeps its `error` status but not the exception detail. + * Raise the cap on spans that record many events and can also fail. * * @default 128 */ From 164434626d4f4f592f65759f48b646564831811f Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Fri, 4 Sep 2026 14:20:03 -0400 Subject: [PATCH 22/22] docs(traces): drop the exception reserve from the span-limits changeset The reserve was removed and the event cap is now absolute, but the changeset still promised it. Also names the dropped counters, which are how a caller sees that a span was truncated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J1XjgPzJ4zbHBcYmEDymA2 --- .changeset/node-span-limits.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/node-span-limits.md b/.changeset/node-span-limits.md index 88ab8432b7..fbe06127fc 100644 --- a/.changeset/node-span-limits.md +++ b/.changeset/node-span-limits.md @@ -4,4 +4,4 @@ '@posthog/types': minor --- -Cap spans at 128 user attributes, 128 events (plus a small reserve for exception events), and 8192 characters per string, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. +Cap spans at 128 user attributes, 128 events and 8192 characters per string, configurable with `traces.maxAttributesPerSpan`, `traces.maxEventsPerSpan` and `traces.maxAttributeValueLength`. The earliest entries are kept, and a span that lost any reports how many as `droppedAttributesCount` and `droppedEventsCount`. The event cap is absolute, so an `exception` event the SDK records for you spends an ordinary slot.