-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add telemetry sampling for non-Pro users #2642
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 |
|---|---|---|
|
|
@@ -4,7 +4,11 @@ import { router } from "./router"; | |
| import { RouterProvider } from "@tanstack/react-router"; | ||
| import { PostHogProvider } from "posthog-js/react"; | ||
| import posthog from "posthog-js"; | ||
| import { getTelemetryUserId, isTelemetryOptedIn } from "./hooks/useSettings"; | ||
| import { | ||
| getTelemetryUserId, | ||
| isTelemetryOptedIn, | ||
| isDyadProUser, | ||
| } from "./hooks/useSettings"; | ||
|
|
||
| // Initialize i18next before any rendering | ||
| import "./i18n"; | ||
|
|
@@ -87,6 +91,20 @@ const posthogClient = posthog.init( | |
| event.properties["$ip"] = null; | ||
| } | ||
|
|
||
| // For non-Pro users, only send 10% of events (but always send errors) | ||
| if (!isDyadProUser()) { | ||
| const isErrorEvent = | ||
| event?.event === "$exception" || | ||
| event?.event?.toLowerCase().includes("error") || | ||
| event?.properties?.$exception_type || | ||
| event?.properties?.error; | ||
|
|
||
|
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. 🟡 MEDIUM | complexity | Found by: Code Health, Endorsed by: Correctness, UX Magic number for sampling rate The sampling rate 💡 Suggestion: Extract to a named constant, e.g. |
||
| if (!isErrorEvent && Math.random() > 0.1) { | ||
| console.debug("Non-Pro user: sampling out event", event?.event); | ||
| return null; | ||
|
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. 🟡 MEDIUM | race-condition | Found by: Correctness, Endorsed by: Code Health, UX Pro status in localStorage may be stale at PostHog init time
💡 Suggestion: Add a comment documenting this known timing gap, e.g. |
||
| } | ||
|
Comment on lines
+96
to
+105
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 current sampling logic for non-Pro users will also sample crucial PostHog events like
These special events should always be sent, just like error events, to ensure the collected data is accurate and useful. The suggested change updates the logic to prevent these events from being sampled. |
||
| } | ||
|
|
||
| console.debug( | ||
| "Telemetry opted in - UUID:", | ||
| telemetryUserId, | ||
|
|
||
Uh 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.
P2: The optional chaining stops before
.includes, soevent?.event?.toLowerCase()can returnundefinedand then.includes()throws. Guard the string check (or optional-chain the result) before calling.includes()to avoid runtime errors in telemetry sampling.Prompt for AI agents