-
Notifications
You must be signed in to change notification settings - Fork 337
feat(aiobs): dedicated AI endpoint routing #3773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@posthog/core': patch | ||
| 'posthog-node': patch | ||
| --- | ||
|
|
||
| Add an internal event-channel mechanism so `$ai_*` events can be routed to a dedicated capture endpoint in their own batch, independent of analytics events. Gated behind the unstable, internal-only `_internal_dedicatedAiEndpoint` option on `posthog-node` — not ready for general use. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { PostHog, PostHogOptions } from '@/entrypoints/index.node' | ||
|
|
||
| const mockedFetch = jest.spyOn(globalThis, 'fetch').mockImplementation() | ||
|
|
||
| const HOST = 'http://example.com' | ||
| const ANALYTICS_URL = `${HOST}/batch/` | ||
| const AI_URL = `${HOST}/i/v0/ai/batch/` | ||
|
|
||
| const okResponse = { | ||
| status: 200, | ||
| text: () => Promise.resolve('ok'), | ||
| json: () => Promise.resolve({ status: 'ok' }), | ||
| } as any | ||
|
|
||
| // Exact-match — AI_URL contains "/batch/" as a substring, so substring matching would conflate them. | ||
| const callsTo = (url: string): any[] => mockedFetch.mock.calls.filter((c) => c[0] === url) | ||
|
|
||
| const batchSentTo = (url: string): any[] | undefined => { | ||
| const call = callsTo(url).at(-1) | ||
| return call ? JSON.parse((call[1] as any).body).batch : undefined | ||
| } | ||
|
|
||
| const newClient = (options: Partial<PostHogOptions> = {}): PostHog => | ||
| new PostHog('TEST_API_KEY', { host: HOST, fetchRetryCount: 0, disableCompression: true, ...options }) | ||
|
|
||
| describe('PostHog Node.js — dedicated AI endpoint (_internal_dedicatedAiEndpoint)', () => { | ||
| let errorSpy: jest.SpyInstance | ||
|
|
||
| beforeEach(() => { | ||
| errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) | ||
| mockedFetch.mockResolvedValue(okResponse) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| mockedFetch.mockReset() | ||
| errorSpy.mockRestore() | ||
| }) | ||
|
|
||
| describe('when enabled', () => { | ||
| it('routes batched $ai_* events to the dedicated AI path', async () => { | ||
| const ph = newClient({ _internal_dedicatedAiEndpoint: true }) | ||
| ph.capture({ distinctId: 'u1', event: '$ai_generation', properties: { $ai_model: 'gpt-4' } }) | ||
| await ph.shutdown() | ||
|
|
||
| expect(batchSentTo(AI_URL)?.map((e: any) => e.event)).toEqual(['$ai_generation']) | ||
| expect(callsTo(ANALYTICS_URL)).toHaveLength(0) | ||
| }) | ||
|
|
||
| it('keeps non-AI events on the normal path, in a separate batch from AI events', async () => { | ||
| const ph = newClient({ _internal_dedicatedAiEndpoint: true }) | ||
| ph.capture({ distinctId: 'u1', event: '$ai_generation', properties: {} }) | ||
| ph.capture({ distinctId: 'u1', event: 'button_clicked', properties: {} }) | ||
| await ph.shutdown() | ||
|
|
||
| expect(batchSentTo(AI_URL)?.map((e: any) => e.event)).toEqual(['$ai_generation']) | ||
| expect(batchSentTo(ANALYTICS_URL)?.map((e: any) => e.event)).toEqual(['button_clicked']) | ||
| }) | ||
|
|
||
| it('routes immediate $ai_* captures to the dedicated AI path', async () => { | ||
| const ph = newClient({ _internal_dedicatedAiEndpoint: true }) | ||
| await ph.captureImmediate({ distinctId: 'u1', event: '$ai_embedding', properties: {} }) | ||
|
|
||
| expect(batchSentTo(AI_URL)?.[0].event).toBe('$ai_embedding') | ||
| expect(callsTo(ANALYTICS_URL)).toHaveLength(0) | ||
| await ph.shutdown() | ||
| }) | ||
| }) | ||
|
|
||
| describe('when disabled (default)', () => { | ||
| it('routes $ai_* events to the normal batch path', async () => { | ||
| const ph = newClient() | ||
| ph.capture({ distinctId: 'u1', event: '$ai_generation', properties: {} }) | ||
| await ph.shutdown() | ||
|
|
||
| expect(batchSentTo(ANALYTICS_URL)?.map((e: any) => e.event)).toEqual(['$ai_generation']) | ||
| expect(callsTo(AI_URL)).toHaveLength(0) | ||
| }) | ||
|
Comment on lines
+40
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The first test ("routes batched Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/node/src/__tests__/dedicated-ai-endpoint.spec.ts
Line: 40-77
Comment:
**Prefer parameterised tests for the routing cases**
The first test ("routes batched `$ai_*` events to the dedicated AI path") and the last test ("routes `$ai_*` events to the normal batch path when disabled") cover the exact same scenario — one `$ai_generation` event captured via `capture()` / `shutdown()` — differing only in the `_internal_dedicatedAiEndpoint` flag and the expected URL. Per the team's style, these should be consolidated into a single `it.each` table. Similarly, a disabled-path counterpart for the `captureImmediate` case is absent; adding it to an `it.each` would close that coverage gap at the same time.
How can I resolve this? If you propose a fix, please make it concise.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! |
||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,6 +135,13 @@ export type PostHogOptions = Omit<PostHogCoreOptions, 'before_send'> & { | |
| persistence?: 'memory' | ||
| personalApiKey?: string | ||
| privacyMode?: boolean | ||
| /** | ||
| * Routes `$ai_*` events to a dedicated capture-ai endpoint in their own batch. | ||
| * | ||
| * @internal Not ready for use — the backend endpoint and ingress routing are still being | ||
| * rolled out. Do not enable; behaviour and naming may change or be removed without notice. | ||
| */ | ||
| _internal_dedicatedAiEndpoint?: boolean | ||
|
Comment on lines
+138
to
+144
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will we roll this out? asking users to enable
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea was to test this with test projects or team 2 in prod, and once ready just remove the guard and ship a new minor.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the goal to test the SDK or the backend? |
||
| enableExceptionAutocapture?: boolean | ||
| // The interval in milliseconds between polls for refreshing feature flag definitions. Defaults to 30 seconds. | ||
| featureFlagsPollingInterval?: number | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
firstError ??= errcaptures only the first error encountered. IfDEFAULT_EVENT_CHANNELflushes successfully butAI_EVENT_CHANNELthrows, the error is rethrown correctly. However, if both channels fail (the analytics flush throws first), the AI channel's error is silently discarded. Callers see the analytics error and have no indication that AI events were also undelivered. UsingAggregateError(or a simple array-join) would preserve both failure reasons without changing the throw semantics.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good point with
AggregateErrorUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AggregateErrorwas my first intuition too but there are many things that depend on the error being a specific PostHog error class downstream, such as:flushguarantees it throws@throws PostHogFetchHttpError / PostHogFetchNetworkError, so it'd need a major bump for this changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points — the
AggregateErrorsuggestion would break the documentedthrowscontract and the downstream class-identity checks. The currentfirstError ??= errbehavior is a reasonable tradeoff, especially behind an@internalflag.One lightweight option that preserves the existing contract while keeping the second error visible: log the dropped error rather than silently discarding it:
This way callers still get the original PostHog error class from the first failure, but the second failure isn't completely invisible — it surfaces through the existing
errorevent that the SDK already uses for non-fatal issues. No contract change, no class-identity breakage.That said, if you're comfortable accepting the silent drop given the
@internalgating, the current code is also fine as-is.Tip: You can customize Greptile's behavior for this repo with
.greptile/rules.mdand.greptile/config.json.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, lets add a TODO about this behavior then at least, so we can consider a refactor in the next major