feat: add telemetry abstraction#252
Conversation
Introduces src/lib/telemetry.ts as a unified interface for analytics and error reporting. Backed by PostHog on both client and server with a no-op fallback when PUBLIC_POSTHOG_PROJECT_TOKEN is unset. - track(event, props?), pageview(route), captureError(err, ctx?) - identify(userId, traits?), reset(), shutdown() - hooks.server.ts: pageview tracking + error capture via abstraction - hooks.client.ts: error capture via abstraction - Graceful shutdown on SIGTERM
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR implements a centralized telemetry abstraction layer in ChangesTelemetry Abstraction and Integration
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant ClientHook as Client Hook
participant Telemetry as Telemetry Layer
participant PostHogBrowser as PostHog Browser
participant Server as SvelteKit Server
participant ServerHook as Server Hook
participant PostHogNode as PostHog Node
Client->>ClientHook: error thrown
ClientHook->>Telemetry: captureError(error, {source: 'client'})
Telemetry->>PostHogBrowser: capture exception
Server->>ServerHook: HTTP request
ServerHook->>Telemetry: pageview(route)
Telemetry->>PostHogNode: capture $pageview
ServerHook-->>Server: resolved response
Note over Server: Process termination signal
Server->>Telemetry: shutdown()
Telemetry->>PostHogNode: close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes PostHog telemetry into a new src/lib/telemetry.ts module, updating client and server hooks to use unified tracking and error reporting. The review identifies several issues: potential double-counting of pageviews, a regression in user identification for server events, and performance concerns due to immediate event flushing. Suggestions were also provided to include error stack traces, handle SIGINT for graceful shutdowns, and optimize the shutdown logic to prevent unnecessary client re-initialization.
| if (!pathname.startsWith('/api') && !pathname.startsWith('/_')) { | ||
| pageview(pathname).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Capturing a pageview on the server here will likely result in double-counting, as the PostHog client-side SDK also captures pageviews by default on the initial load. In a SvelteKit application, it is generally preferred to rely on client-side tracking for richer telemetry (e.g., browser info, screen resolution). If server-side tracking is intentionally desired, you should disable client-side auto-capture by setting capture_pageview: false in the PostHog initialization config.
There was a problem hiding this comment.
Fixed. Added capture_pageview: false to client PostHog init config to prevent double-counting. Server-side pageview tracking is intentionally kept for route-level analytics that client-side auto-capture misses (SSR navigation, bot traffic). hooks.server.ts:51-53.
| process.on('SIGTERM', () => { | ||
| shutdown().catch(() => {}); | ||
| }); |
There was a problem hiding this comment.
It is recommended to handle SIGINT (e.g., when pressing Ctrl+C in a terminal) in addition to SIGTERM to ensure graceful shutdown and flushing of telemetry events during local development or manual restarts.
| process.on('SIGTERM', () => { | |
| shutdown().catch(() => {}); | |
| }); | |
| ['SIGTERM', 'SIGINT'].forEach((signal) => { | |
| process.on(signal, () => { | |
| shutdown().catch(() => {}); | |
| }); | |
| }); |
There was a problem hiding this comment.
Fixed. Added SIGINT alongside SIGTERM: ['SIGTERM', 'SIGINT'].forEach(signal => process.on(signal, () => shutdown().catch(() => {}))). hooks.server.ts:63-67.
| ph?.capture(event, props); | ||
| } else { | ||
| const ph = await getServerPh(); | ||
| ph?.capture({ distinctId: 'server', event, properties: props }); |
There was a problem hiding this comment.
The new implementation hardcodes 'server' as the distinctId. The previous logic in hooks.server.ts attempted to use the user ID from the session (event.locals.session.user.id). This change is a regression in data quality, as it prevents attributing server-side events to specific users. Consider allowing an optional userId or distinctId to be passed to the telemetry functions.
There was a problem hiding this comment.
Fixed. track() now accepts an optional userId in props and passes it as distinctId on the server path: const distinctId = props?.userId ? String(props.userId) : 'server'. telemetry.ts:8-14. Full identify() is also available for explicit user attribution.
| distinctId: 'server', | ||
| event: 'server_error', | ||
| properties: { | ||
| error: err instanceof Error ? err.message : String(err), |
There was a problem hiding this comment.
Including the error stack trace provides essential context for debugging server-side issues in PostHog. Currently, only the error message is captured.
| error: err instanceof Error ? err.message : String(err), | |
| error: err instanceof Error ? err.message : String(err), | |
| stack: err instanceof Error ? err.stack : undefined, |
There was a problem hiding this comment.
Fixed. Added stack: err instanceof Error ? err.stack : undefined to server_error properties. telemetry.ts:38-39.
| const ph = await getServerPh(); | ||
| await ph?.shutdown(); | ||
| serverPhInstance = null; |
There was a problem hiding this comment.
The shutdown function should check for the existence of serverPhInstance before calling getServerPh(). Otherwise, it will unnecessarily initialize a new PostHog instance just to immediately shut it down if one wasn't already active.
| const ph = await getServerPh(); | |
| await ph?.shutdown(); | |
| serverPhInstance = null; | |
| if (!serverPhInstance) return; | |
| await serverPhInstance.shutdown(); | |
| serverPhInstance = null; |
There was a problem hiding this comment.
Fixed. shutdown() now checks if (!serverPhInstance) return; before calling shutdown, avoiding unnecessary PostHog initialization. telemetry.ts:63-67.
| flushAt: 1, | ||
| flushInterval: 0, |
There was a problem hiding this comment.
Setting flushAt: 1 and flushInterval: 0 on the server-side client causes an HTTP request to be sent for every single event. This can lead to significant performance overhead under load. It is recommended to use the default batching behavior for better efficiency in a production environment.
There was a problem hiding this comment.
Fixed. Removed flushAt: 1 and flushInterval: 0 from server PostHog config. Now uses default batching behavior for production efficiency. telemetry.ts:97-99.
- Replace 'any' types with proper typed imports (clientPhInstance, serverPhInstance) - Fix import ordering in hooks.client.ts and hooks.server.ts (biome organizeImports) - Add capture_pageview: false to client PostHog config to prevent double-counting - Handle SIGINT alongside SIGTERM for graceful shutdown - Add error stack trace to server_error events - Guard shutdown() against initializing PostHog just to shut it down - Remove flushAt:1/flushInterval:0 to use default batching - Allow optional userId passthrough in track()
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks.client.ts (1)
1-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDirect posthog-js initialization in hooks.client.ts violates the single-wrapper pattern.
This file still initializes PostHog directly, which bypasses
$lib/telemetryand causes config drift (the wrapper setscapture_pageview: false, but this direct init does not). Additionally, the wrapper's__tilt_initflag prevents re-initialization, masking the architectural violation.💡 Suggested direction
Remove the direct initialization and delegate to the telemetry wrapper:
-import posthog from 'posthog-js'; -import { PUBLIC_POSTHOG_HOST, PUBLIC_POSTHOG_PROJECT_TOKEN } from '$env/static/public'; -import { captureError } from '$lib/telemetry'; +import { captureError, initTelemetry } from '$lib/telemetry'; export async function init() { - posthog.init(PUBLIC_POSTHOG_PROJECT_TOKEN, { - api_host: '/ingest', - ui_host: PUBLIC_POSTHOG_HOST, - defaults: '2026-01-30', - capture_exceptions: true, - person_profiles: 'identified_only' - }); + await initTelemetry(); }Then add
initTelemetry()tosrc/lib/telemetry.tsto expose SDK initialization in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks.client.ts` around lines 1 - 13, Remove the direct posthog-js initialization in the exported init function (the posthog.init call and its PUBLIC_* env usage) and instead call a single initialization entrypoint exported from the telemetry wrapper (e.g. export/initTelemetry in src/lib/telemetry.ts). Add an initTelemetry() function in telemetry.ts that performs the SDK init with the wrapper’s canonical options (including capture_pageview: false, capture_exceptions, person_profiles, api_host/ui_host) and respects the wrapper’s __tilt_init reinitialization guard; then update the hooks.client.ts init function to import and call initTelemetry() (and remove direct posthog import and env var usage) so all PostHog configuration goes through $lib/telemetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks.server.ts`:
- Around line 63-67: The module-level process signal handlers using
['SIGTERM','SIGINT'] and process.on(...) around shutdown() are incompatible with
adapter-static and should be removed or gated; locate the signal registration
block that calls process.on(signal, () => { shutdown().catch(() => {}); }) and
either delete that block entirely or wrap it behind an environment/runtime check
(e.g., only register when using a Node runtime) so shutdown() is only wired when
running with a server adapter (adapter-node) or a proper server process.
In `@src/lib/telemetry.ts`:
- Around line 13-14: The current distinctId assignment can turn object userIds
into "[object Object]"; change the derivation so it only accepts primitive
userId values (string or number) and falls back to 'server' for anything else:
inspect props?.userId with typeof checks (e.g., === 'string' or === 'number')
before calling String(...) and use that safe distinctId when calling ph?.capture
(the variable currently declared as distinctId and the capture invocation).
- Around line 72-90: Replace the loose any typing for clientPhInstance with a
concrete PostHog type and a small local extension for the injected __tilt_init
flag: import the PostHog type from 'posthog-js', declare an interface (e.g.
TiltPostHog extends PostHog { __tilt_init?: boolean }), change
clientPhInstance's type to TiltPostHog | null and update getClientPh's return
type accordingly, and remove or adjust any (clientPhInstance as Record<string,
unknown>) casts to use the new TiltPostHog type so the client is strictly typed
like the server-side posthog-node usage.
---
Outside diff comments:
In `@src/hooks.client.ts`:
- Around line 1-13: Remove the direct posthog-js initialization in the exported
init function (the posthog.init call and its PUBLIC_* env usage) and instead
call a single initialization entrypoint exported from the telemetry wrapper
(e.g. export/initTelemetry in src/lib/telemetry.ts). Add an initTelemetry()
function in telemetry.ts that performs the SDK init with the wrapper’s canonical
options (including capture_pageview: false, capture_exceptions, person_profiles,
api_host/ui_host) and respects the wrapper’s __tilt_init reinitialization guard;
then update the hooks.client.ts init function to import and call initTelemetry()
(and remove direct posthog import and env var usage) so all PostHog
configuration goes through $lib/telemetry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 688b05c8-54cc-433e-b11d-d595d0a7b899
⛔ Files ignored due to path filters (4)
.svelte-kit/ambient.d.tsis excluded by!**/.svelte-kit/**.svelte-kit/generated/client/app.jsis excluded by!**/.svelte-kit/**,!**/generated/**.svelte-kit/generated/client/nodes/1.jsis excluded by!**/.svelte-kit/**,!**/generated/**.svelte-kit/generated/server/internal.jsis excluded by!**/.svelte-kit/**,!**/generated/**
📒 Files selected for processing (3)
src/hooks.client.tssrc/hooks.server.tssrc/lib/telemetry.ts
| ['SIGTERM', 'SIGINT'].forEach((signal) => { | ||
| process.on(signal, () => { | ||
| shutdown().catch(() => {}); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this app targets Node-only runtime or an edge adapter
fd -i "svelte.config.*" --exec rg -n "adapter|kit"
rg -nP "@sveltejs/adapter|adapter-" package.jsonRepository: Jonathangadeaharder/Tilt
Length of output: 244
Remove or refactor signal handlers for static adapter compatibility.
This codebase uses @sveltejs/adapter-static, which generates static sites. Module-level signal handlers like process.on(signal) are executed during the build phase, but they won't function in static deployments since there is no running server to catch signals. Either remove these handlers if unnecessary for static builds, or migrate to a Node.js adapter (e.g., @sveltejs/adapter-node) if server-side lifecycle management is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks.server.ts` around lines 63 - 67, The module-level process signal
handlers using ['SIGTERM','SIGINT'] and process.on(...) around shutdown() are
incompatible with adapter-static and should be removed or gated; locate the
signal registration block that calls process.on(signal, () => {
shutdown().catch(() => {}); }) and either delete that block entirely or wrap it
behind an environment/runtime check (e.g., only register when using a Node
runtime) so shutdown() is only wired when running with a server adapter
(adapter-node) or a proper server process.
| const distinctId = props?.userId ? String(props.userId) : 'server'; | ||
| ph?.capture({ distinctId, event, properties: props }); |
There was a problem hiding this comment.
Harden distinctId derivation to avoid [object Object] identities.
Line 13 currently stringifies any props.userId, so object values collapse to [object Object] and can merge unrelated users in telemetry.
💡 Suggested fix
-export async function track(event: string, props?: Record<string, unknown>) {
+type TrackProps = Record<string, unknown> & { userId?: string | number };
+
+export async function track(event: string, props?: TrackProps) {
@@
- const distinctId = props?.userId ? String(props.userId) : 'server';
+ const distinctId =
+ typeof props?.userId === 'string' || typeof props?.userId === 'number'
+ ? String(props.userId)
+ : 'server';🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 13-13: 'props.userId' will use Object's default stringification format ('[object Object]') when stringified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/telemetry.ts` around lines 13 - 14, The current distinctId assignment
can turn object userIds into "[object Object]"; change the derivation so it only
accepts primitive userId values (string or number) and falls back to 'server'
for anything else: inspect props?.userId with typeof checks (e.g., === 'string'
or === 'number') before calling String(...) and use that safe distinctId when
calling ph?.capture (the variable currently declared as distinctId and the
capture invocation).
| let clientPhInstance: Record<string, any> | null = null; | ||
|
|
||
| async function getClientPh() { | ||
| if (!browser) return null; | ||
| if (clientPhInstance) return clientPhInstance; | ||
| const mod = await import('posthog-js'); | ||
| clientPhInstance = mod.default; | ||
| if (!(clientPhInstance as Record<string, unknown>).__tilt_init) { | ||
| clientPhInstance.init(PUBLIC_POSTHOG_PROJECT_TOKEN, { | ||
| api_host: '/ingest', | ||
| ui_host: PUBLIC_POSTHOG_HOST, | ||
| defaults: '2026-01-30', | ||
| capture_pageview: false, | ||
| capture_exceptions: true, | ||
| person_profiles: 'identified_only' | ||
| }); | ||
| (clientPhInstance as Record<string, unknown>).__tilt_init = true; | ||
| } | ||
| return clientPhInstance; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit any usage and current client instance typing in telemetry wrapper
rg -nP --type=ts '\bany\b' src/lib/telemetry.ts
rg -nP --type=ts 'clientPhInstance|getClientPh\s*\(' src/lib/telemetry.tsRepository: Jonathangadeaharder/Tilt
Length of output: 721
🏁 Script executed:
cat -n src/lib/telemetry.ts | head -100Repository: Jonathangadeaharder/Tilt
Length of output: 3639
🏁 Script executed:
# Check if PostHog types are available or need to be inferred
rg -nP --type=ts 'import.*posthog|from.*posthog' src/lib/telemetry.tsRepository: Jonathangadeaharder/Tilt
Length of output: 237
🏁 Script executed:
# Check how the return value of getClientPh() is used in the codebase
rg -B2 -A3 'await getClientPh\(\)' src/lib/telemetry.tsRepository: Jonathangadeaharder/Tilt
Length of output: 813
Replace any-based client instance typing with concrete PostHog types.
Record<string, any> on line 72 violates strict typing requirements and leaks any through getClientPh(). This is inconsistent with the server-side client at line 93, which properly uses import('posthog-node').PostHog.
Suggested fix
-let clientPhInstance: Record<string, any> | null = null;
+type PostHogClient = typeof import('posthog-js').default;
+type TiltPostHogClient = PostHogClient & { __tilt_init?: boolean };
+let clientPhInstance: TiltPostHogClient | null = null;
-async function getClientPh() {
+async function getClientPh(): Promise<TiltPostHogClient | null> {
@@
- if (!(clientPhInstance as Record<string, unknown>).__tilt_init) {
+ if (!clientPhInstance.__tilt_init) {
@@
- (clientPhInstance as Record<string, unknown>).__tilt_init = true;
+ clientPhInstance.__tilt_init = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let clientPhInstance: Record<string, any> | null = null; | |
| async function getClientPh() { | |
| if (!browser) return null; | |
| if (clientPhInstance) return clientPhInstance; | |
| const mod = await import('posthog-js'); | |
| clientPhInstance = mod.default; | |
| if (!(clientPhInstance as Record<string, unknown>).__tilt_init) { | |
| clientPhInstance.init(PUBLIC_POSTHOG_PROJECT_TOKEN, { | |
| api_host: '/ingest', | |
| ui_host: PUBLIC_POSTHOG_HOST, | |
| defaults: '2026-01-30', | |
| capture_pageview: false, | |
| capture_exceptions: true, | |
| person_profiles: 'identified_only' | |
| }); | |
| (clientPhInstance as Record<string, unknown>).__tilt_init = true; | |
| } | |
| return clientPhInstance; | |
| type PostHogClient = typeof import('posthog-js').default; | |
| type TiltPostHogClient = PostHogClient & { __tilt_init?: boolean }; | |
| let clientPhInstance: TiltPostHogClient | null = null; | |
| async function getClientPh(): Promise<TiltPostHogClient | null> { | |
| if (!browser) return null; | |
| if (clientPhInstance) return clientPhInstance; | |
| const mod = await import('posthog-js'); | |
| clientPhInstance = mod.default; | |
| if (!clientPhInstance.__tilt_init) { | |
| clientPhInstance.init(PUBLIC_POSTHOG_PROJECT_TOKEN, { | |
| api_host: '/ingest', | |
| ui_host: PUBLIC_POSTHOG_HOST, | |
| defaults: '2026-01-30', | |
| capture_pageview: false, | |
| capture_exceptions: true, | |
| person_profiles: 'identified_only' | |
| }); | |
| clientPhInstance.__tilt_init = true; | |
| } | |
| return clientPhInstance; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/telemetry.ts` around lines 72 - 90, Replace the loose any typing for
clientPhInstance with a concrete PostHog type and a small local extension for
the injected __tilt_init flag: import the PostHog type from 'posthog-js',
declare an interface (e.g. TiltPostHog extends PostHog { __tilt_init?: boolean
}), change clientPhInstance's type to TiltPostHog | null and update
getClientPh's return type accordingly, and remove or adjust any
(clientPhInstance as Record<string, unknown>) casts to use the new TiltPostHog
type so the client is strictly typed like the server-side posthog-node usage.
|
Closes #232. Single telemetry abstraction at src/lib/telemetry.ts — PostHog-backed with no-op fallback. Hooks into SvelteKit handle + handleError. Lazy-loads client/server SDKs.
Summary by CodeRabbit
Release Notes
New Features