-
Notifications
You must be signed in to change notification settings - Fork 500
fix(cli): stop telemetry hangs and errors on blocked networks #5880
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
Open
pamelachia
wants to merge
9
commits into
develop
Choose a base branch
from
pamela/cli-telemetry-network-resilience
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+188
−18
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
baa3c5a
fix(cli): bound posthog close, quiet sdk logs
pamelachia a96c6f5
fix(cli): fire-and-forget posthog fetch in ts cli
pamelachia e38cb95
test(cli): cover posthog logger routing and scoped client
pamelachia c367a3e
fix(cli): cap go telemetry close at 2s
pamelachia 77ce839
fix(cli): 2s telemetry request timeout, flatten factory
pamelachia 4246000
fix(cli): catch posthog shutdown rejection
pamelachia 6133c2d
chore(cli): trim telemetry client comments
pamelachia 8bec921
Merge branch 'develop' into pamela/cli-telemetry-network-resilience
pamelachia 878576b
fix(cli): bound posthog shutdown with a single deadline
pamelachia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { Effect } from "effect"; | ||
| import { PostHog, type PostHogOptions } from "posthog-node"; | ||
|
|
||
| const EXIT_DELAY_CAP_MS = 2_000; | ||
|
|
||
| const delivered = { | ||
| status: 200, | ||
| text: () => Promise.resolve(""), | ||
| json: () => Promise.resolve({}), | ||
| }; | ||
|
|
||
| // posthog-node has no logger hook: delivery failures hit hardcoded | ||
| // console.error calls and multi-second retries, so report them as delivered. | ||
| export const fireAndForgetFetch: NonNullable<PostHogOptions["fetch"]> = async (url, options) => { | ||
| try { | ||
| const response = await globalThis.fetch(url, options); | ||
| return response.status >= 400 ? delivered : response; | ||
| } catch { | ||
| return delivered; | ||
| } | ||
| }; | ||
|
|
||
| export const scopedPosthogClient = (apiKey: string, host: string) => | ||
| Effect.acquireRelease( | ||
| Effect.sync( | ||
| () => | ||
| new PostHog(apiKey, { | ||
| host, | ||
| flushAt: 1, | ||
| flushInterval: 0, | ||
| requestTimeout: EXIT_DELAY_CAP_MS, | ||
| fetch: fireAndForgetFetch, | ||
| }), | ||
| ), | ||
| // Catch on the promise: Effect.promise turns rejections into defects, | ||
| // which escape Effect.ignore and fail the command. | ||
| (client) => Effect.promise(() => client._shutdown(EXIT_DELAY_CAP_MS).catch(() => undefined)), | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import { describe, expect, it } from "@effect/vitest"; | ||
| import { afterEach, vi } from "vitest"; | ||
| import { Effect } from "effect"; | ||
| import { PostHog } from "posthog-node"; | ||
| import { fireAndForgetFetch, scopedPosthogClient } from "./posthog-client.ts"; | ||
|
|
||
| const BATCH_URL = "https://eu.i.posthog.com/batch/"; | ||
| const BATCH_OPTIONS = { method: "POST" as const, headers: {}, body: "{}" }; | ||
|
|
||
| describe("fireAndForgetFetch", () => { | ||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| it("passes successful responses through untouched", async () => { | ||
| vi.stubGlobal("fetch", async () => new Response(`{"status":1}`, { status: 200 })); | ||
|
|
||
| const response = await fireAndForgetFetch(BATCH_URL, BATCH_OPTIONS); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(await response.text()).toBe(`{"status":1}`); | ||
| }); | ||
|
|
||
| it("reports success when the network is unreachable", async () => { | ||
| vi.stubGlobal("fetch", async () => { | ||
| throw new Error("connect ECONNREFUSED"); | ||
| }); | ||
|
|
||
| const response = await fireAndForgetFetch(BATCH_URL, BATCH_OPTIONS); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(await response.text()).toBe(""); | ||
| expect(await response.json()).toEqual({}); | ||
| }); | ||
|
|
||
| it("reports success on error responses so the SDK never retries or logs", async () => { | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| async () => new Response("Proxy Authentication Required", { status: 407 }), | ||
| ); | ||
|
|
||
| const response = await fireAndForgetFetch(BATCH_URL, BATCH_OPTIONS); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(await response.text()).toBe(""); | ||
| }); | ||
| }); | ||
|
|
||
| describe("scopedPosthogClient", () => { | ||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| it.live("captures and shuts down cleanly against an unreachable host", () => | ||
| Effect.gen(function* () { | ||
| const client = yield* scopedPosthogClient("phc_test", "http://127.0.0.1:9"); | ||
| expect(client).toBeInstanceOf(PostHog); | ||
| client.capture({ event: "verify_event", distinctId: "device-1" }); | ||
| }).pipe(Effect.scoped), | ||
| ); | ||
|
|
||
| it.live( | ||
| "bounds the whole shutdown when a request is in flight and another event is queued", | ||
| () => | ||
| Effect.gen(function* () { | ||
| let requestStarted = () => {}; | ||
| const firstRequestInFlight = new Promise<void>((resolve) => { | ||
| requestStarted = resolve; | ||
| }); | ||
| vi.stubGlobal( | ||
| "fetch", | ||
| (_url: string, options: { signal?: AbortSignal }) => | ||
| new Promise<Response>((_resolve, reject) => { | ||
| requestStarted(); | ||
| options.signal?.addEventListener("abort", () => | ||
| reject(new DOMException("The operation was aborted.", "AbortError")), | ||
| ); | ||
| }), | ||
| ); | ||
|
|
||
| const startedAt = performance.now(); | ||
| yield* Effect.gen(function* () { | ||
| const client = yield* scopedPosthogClient("phc_test", "https://blackhole.invalid"); | ||
| client.capture({ event: "first_event", distinctId: "device-1" }); | ||
| yield* Effect.promise(() => firstRequestInFlight); | ||
| client.capture({ event: "second_event", distinctId: "device-1" }); | ||
| }).pipe(Effect.scoped); | ||
|
|
||
| expect(performance.now() - startedAt).toBeLessThan(3_000); | ||
| }), | ||
| 10_000, | ||
| ); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.