Repository navigation
feat(ai): add per-request context to span processor - #5299
Conversation
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| export type PostHogContextSource = Pick<PostHog, 'getContext'> | ||
| export type CapturedContext = NonNullable<ReturnType<PostHogContextSource['getContext']>> |
There was a problem hiding this comment.
P2 · Keep context source types compatible with supported Node SDK versions
With a supported older posthog-node version such as 5.0.0 and skipLibCheck: false, importing the OTel entrypoint fails type checking because PostHog lacks getContext, so this Pick violates its key constraint. The runtime guard cannot protect published declarations, including consumers without client; define a compatible structural context-source type.
| const context = client.getContext() | ||
| return context && hasValues(context) ? context : undefined |
There was a problem hiding this comment.
P2 · Snapshot context values instead of retaining mutable references
When application code mutates client.getContext() or a context object supplied with fresh: true before a span ends, captureContext retains that same object, so export uses the updated identity and properties rather than their span-start values. Primitive arrays also remain mutable after enqueueing; capture independent attribute values in onStart.
There was a problem hiding this comment.
intentional. we keep the context reference at span start and convert only for ai spans at end, so non-ai spans stay cheap. mutating a context object (or a property inside it) while a span is open isn't a pattern we want to pay a per-span copy for. documented on the client option that properties are read at span end
Coherence4 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures 44 components, 7 bullets (5 invariants, 2 requirements), 4 problems Spec problems (spec --check)
Chokepoints: none declared. Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline
Close one with |
|
[Medium impact] Adds per-request context capture to the span processor. Fix older-client type compatibility, freeze context values at span start, and satisfy the repository guidance before merging. Reviews (1) · Last reviewed commit: "feat: span processor request context" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export type PostHogContextSource = Pick<PostHog, 'getContext'> | ||
| export type CapturedContext = NonNullable<ReturnType<PostHogContextSource['getContext']>> |
There was a problem hiding this comment.
PostHogContextSource requires PostHog.getContext, but the supported posthog-node ^5.0.0 range includes releases before context support arrived in 5.15.0. TypeScript consumers on those releases with library checking enabled get a declaration error when importing the OTel entry point, even without passing client. The runtime guard cannot prevent this. Define a standalone structural context interface instead of deriving it from PostHog.
Knowledge Base Used: AI observability integrations
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/src/otel/requestContext.ts
Line: 7-8
Comment:
**Older clients break type checks**
`PostHogContextSource` requires `PostHog.getContext`, but the supported `posthog-node ^5.0.0` range includes releases before context support arrived in 5.15.0. TypeScript consumers on those releases with library checking enabled get a declaration error when importing the OTel entry point, even without passing `client`. The runtime guard cannot prevent this. Define a standalone structural context interface instead of deriving it from `PostHog`.
**Knowledge Base Used:** [AI observability integrations](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-js/-/docs/ai-observability.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const context = client.getContext() | ||
| return context && hasValues(context) ? context : undefined |
There was a problem hiding this comment.
captureContext keeps the live context object rather than a snapshot. With withContext(..., { fresh: true }), the Node client stores the caller's object directly. If the caller changes its IDs or properties while an AI span is open, onEnd exports the later values instead of those present at onStart. Nested property values remain shared even without fresh. Convert and copy context values at span start, including arrays, and add a test that changes the source before ending the span.
Knowledge Base Used: Node server SDK
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/src/otel/requestContext.ts
Line: 60-61
Comment:
**Context changes after span start**
`captureContext` keeps the live context object rather than a snapshot. With `withContext(..., { fresh: true })`, the Node client stores the caller's object directly. If the caller changes its IDs or properties while an AI span is open, `onEnd` exports the later values instead of those present at `onStart`. Nested property values remain shared even without `fresh`. Convert and copy context values at span start, including arrays, and add a test that changes the source before ending the span.
**Knowledge Base Used:** [Node server SDK](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-js/-/docs/node-server-sdk.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
intentional. we keep the context reference at span start and convert only for ai spans at end, so non-ai spans stay cheap. mutating a context object (or a property inside it) while a span is open isn't a pattern we want to pay a per-span copy for. documented on the client option that properties are read at span end
There was a problem hiding this comment.
Got it — that trade-off is intentional: the span keeps the context reference, and values are read only when an AI span ends so non-AI spans avoid a per-span copy. Since the client option documents that properties are read at span end, this is not a bug; I’m withdrawing the finding.
| function toAttributeValue(value: unknown): AttributeValue | undefined { | ||
| if (value === undefined || value === null || value === '') { | ||
| return undefined |
There was a problem hiding this comment.
toAttributeValue deliberately returns undefined for skipped values. The repository instruction requires null for an intended lack of result. Return null and update the return type and caller check before merging. The same pattern appears in captureContext.
Rule Used: Use 'null' instead of 'undefined' to indicate an intended lack of result in functions. (source)
Learned From
PostHog/posthog#32556
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/src/otel/requestContext.ts
Line: 32-34
Comment:
**Skipped values use wrong sentinel**
`toAttributeValue` deliberately returns `undefined` for skipped values. The repository instruction requires `null` for an intended lack of result. Return `null` and update the return type and caller check before merging. The same pattern appears in `captureContext`.
**Rule Used:** Use 'null' instead of 'undefined' to indicate an intended lack of result in functions. ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=0631667e-2260-42be-a589-0d8da9ef007d))
**Learned From**
[PostHog/posthog#32556](https://github.com/PostHog/posthog/pull/32556)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
that rule is from the main posthog repo, posthog-js uses undefined for "no value" throughout (including the rest of @posthog/ai), so keeping it consistent with the package
There was a problem hiding this comment.
You’re right — that null rule is for the main posthog repository and doesn’t apply here. undefined is the established absence sentinel throughout posthog-js and @posthog/ai, so the current implementation is consistent with the package. I’ll withdraw this comment.
| '@posthog/ai': minor | ||
| --- | ||
|
|
||
| `PostHogSpanProcessor` accepts a `client` option. When set, each AI span it exports gets the distinct ID, session ID and properties from the context entered with `client.withContext()`, so those spans reach PostHog under the right person on every request. Attributes already on a span are kept. |
There was a problem hiding this comment.
The changeset has three sentences, including precedence details. RELEASING.md requires one short, title-like line that states the feature and leaves details to documentation. Shorten this description before merging to satisfy that requirement.
| `PostHogSpanProcessor` accepts a `client` option. When set, each AI span it exports gets the distinct ID, session ID and properties from the context entered with `client.withContext()`, so those spans reach PostHog under the right person on every request. Attributes already on a span are kept. | |
| Add per-request PostHog context to AI spans through the `PostHogSpanProcessor` client option. |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: .changeset/span-processor-request-context.md
Line: 5
Comment:
**Release entry is too long**
The changeset has three sentences, including precedence details. `RELEASING.md` requires one short, title-like line that states the feature and leaves details to documentation. Shorten this description before merging to satisfy that requirement.
```suggestion
Add per-request PostHog context to AI spans through the `PostHogSpanProcessor` client option.
```
**Context Used:** AGENTS.md ([source](https://github.com/posthog/posthog-js/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| export type CapturedContext = NonNullable<ReturnType<PostHogContextSource['getContext']>> | ||
|
|
||
| const DISTINCT_ID_ATTRIBUTE = 'posthog.distinct_id' | ||
| const SESSION_ID_ATTRIBUTE = '$ai_session_id' |
There was a problem hiding this comment.
Should this be $session_id? posthog-node treats the context sessionId as the web/replay session. capture writes it as $session_id (packages/node/src/client.ts:3274), and the Express middleware fills it from the posthog-js X-POSTHOG-SESSION-ID header. Stamping it as $ai_session_id has three effects:
- AI events get no
$session_id, so the trace view's replay link never shows (getSessionIDreads only$session_id). - Ingestion fills
$ai_session_idfrom the instrumentation's own session ID (e.g.eve.session.id) only while it's unset, so this replaces the real conversation grouping with the browser session. - The wrapper integrations in this package send
$session_idunder the same context, so OTel and wrapper events disagree.
Would mapping it to $session_id, and leaving $ai_session_id to span attributes or an explicit context property, work? posthog-python#1060 and posthog-go#371 use the same mapping, so all three would change together.
| expect(ended.map((span) => span.attributes)).toEqual([{}, {}]) | ||
| }) | ||
|
|
||
| it('stamps each concurrent request with its own context', async () => { |
There was a problem hiding this comment.
Each span here starts and ends in the same tick, so the two spans never overlap. A single shared "last captured" slot would still pass, and so would moving captureContext from onStart to onEnd. Could this start both spans inside their contexts, await, and then end them outside? That covers concurrent spans and the streaming case the onStart snapshot exists for.
| if (hasIdentity && key === DISTINCT_ID_ATTRIBUTE) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
P2 · Preserve span identity across all context identity aliases
When a span has user.id: 'bob' and context properties contain ai.telemetry.metadata.posthog_distinct_id: 'alice', toAttributes filters only posthog.distinct_id, so ingestion selects Alice and attributes Bob's call to another person. This violates span-identity precedence; skip all context identity aliases when the span already identifies a user.
| } | ||
|
|
||
| function toAttributes(context: CapturedContext, spanAttributes: Attributes): Attributes { | ||
| const hasIdentity = IDENTITY_ATTRIBUTES.some((key) => spanAttributes[key] !== undefined) |
There was a problem hiding this comment.
P2 · Validate span identity before suppressing the context user
When instrumentation sets user.id to '' or a non-string value, hasIdentity treats it as an existing user and suppresses a valid context distinctId. extract_distinct_id_for_span ignores that unusable value, so the exported AI event receives a fallback identity instead of the request's user; check for a nonempty string before suppressing context identity.
Problem
PostHogSpanProcessorsends every AI event under one person, or none.Changes
PostHogSpanProcessortakes a new optionalclientoption, which is your posthog-node instance.clientset, the AI spans it exports carry the context fromclient.withContext()orclient.enterContext():distinctId→posthog.distinct_idsessionId→$session_idHow it works
onStartstores the activeContextDatafor the span in aWeakMap.onEnddeletes it and, for AI spans only, exports a copy with the context attributes merged in.posthog.distinct_id,user.idor theai.telemetry/ai.settingsdistinct ID keys), noposthog.distinct_idis added from the context or from a property.$session_id, the same ascapture, so AI events link to replays.$ai_session_idstays free for the instrumentation's own conversation ID, or an explicit$ai_session_idproperty.distinctIdandsessionId.Dateand othertoJSONvalues become plain stringsnull, empty strings, BigInt and circular values are skippedtypeof client.getContextis checked, because the peer rangeposthog-node ^5.0.0includes versions released beforegetContextexisted.client, nothing changes.redactSpanand the context copy now share onewithAttributeshelper inspans.ts.PostHogContextSourceis exported, so users can type the option.PostHogTraceExportercan't do this, because an exporter only sees finished spans. Its JSDoc now says so.Same change in posthog-python and posthog-go (
otel).Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
claude-opus-5-5) and subagents, directed by @carlos-marchal-ph.packages/ai/tests/otel_request_context.test.tsruns a realBasicTracerProviderand a real posthog-node client. It covers:withContextcallsenterContext$ai_generationevent with the right distinct ID,$session_idand property.clientoption was chosen over reading a global, because each posthog-node instance has its own AsyncLocalStorage.🤖 Generated with Claude Code