-
Notifications
You must be signed in to change notification settings - Fork 370
chore(nextjs, shared) adjust telemetry sampling for keyless #6488
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
89a05f3
ce31470
335c889
66ac9bc
5f2d474
1a19920
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 @@ | ||
--- | ||
'@clerk/nextjs': patch | ||
'@clerk/shared': patch | ||
--- | ||
|
||
adjust telemetry for keyless |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/** | ||
* @vitest-environment jsdom | ||
*/ | ||
import { describe, expect, test, vi } from 'vitest'; | ||
|
||
import { NextJSTelemetryCollector } from '../utils/telemetry-collector'; | ||
|
||
describe('Keyless Telemetry Integration', () => { | ||
test('NextJSTelemetryCollector can be instantiated and has the required interface', () => { | ||
const collector = new NextJSTelemetryCollector({ | ||
publishableKey: 'pk_test_123', | ||
}); | ||
|
||
// Verify it implements the telemetry collector interface | ||
expect(collector).toBeDefined(); | ||
expect(typeof collector.record).toBe('function'); | ||
expect(typeof collector.isEnabled).toBe('boolean'); | ||
expect(typeof collector.isDebug).toBe('boolean'); | ||
}); | ||
|
||
test('collector handles events without throwing errors', () => { | ||
// Set up keyless environment | ||
(window as any).__clerk_keyless = true; | ||
|
||
const collector = new NextJSTelemetryCollector({ | ||
publishableKey: 'pk_test_123', | ||
disabled: true, // Disable to avoid actual network calls | ||
}); | ||
|
||
// These should not throw | ||
expect(() => { | ||
collector.record({ | ||
event: 'COMPONENT_MOUNTED', | ||
payload: { component: 'SignIn' }, | ||
}); | ||
}).not.toThrow(); | ||
|
||
expect(() => { | ||
collector.record({ | ||
event: 'OTHER_EVENT', | ||
payload: {}, | ||
}); | ||
}).not.toThrow(); | ||
|
||
// Clean up | ||
delete (window as any).__clerk_keyless; | ||
}); | ||
|
||
test('boosts sampling for component-mounted events in keyless mode', () => { | ||
(window as any).__clerk_keyless = true; | ||
|
||
// Spy on underlying collector | ||
const records: any[] = []; | ||
vi.doMock('@clerk/shared/telemetry', async () => { | ||
const actual = await vi.importActual<any>('@clerk/shared/telemetry'); | ||
return { | ||
...actual, | ||
TelemetryCollector: class { | ||
isEnabled = true; | ||
isDebug = false; | ||
record(ev: any) { | ||
records.push(ev); | ||
} | ||
}, | ||
}; | ||
}); | ||
|
||
// Re-require after mock | ||
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
const { NextJSTelemetryCollector: Collector } = require('../utils/telemetry-collector'); | ||
const collector = new Collector({ publishableKey: 'pk_test_123' }); | ||
|
||
collector.record({ event: 'COMPONENT_MOUNTED', payload: { component: 'SignIn' } }); | ||
collector.record({ event: 'COMPONENT_MOUNTED', payload: { component: 'SignUp' }, eventSamplingRate: 0.1 }); | ||
collector.record({ event: 'OTHER_EVENT', payload: {} }); | ||
|
||
expect(records[0].eventSamplingRate).toBe(1); | ||
expect(records[1].eventSamplingRate).toBe(1); | ||
expect(records[2].eventSamplingRate).toBeUndefined(); | ||
|
||
delete (window as any).__clerk_keyless; | ||
vi.resetModules(); | ||
}); | ||
}); | ||
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,6 +10,9 @@ export const KeylessCreatorOrReader = (props: NextClerkProviderProps) => { | |||||||||||||
const isNotFoundRoute = segments[0]?.startsWith('/_not-found') || false; | ||||||||||||||
const [state, fetchKeys] = React.useActionState(createOrReadKeylessAction, null); | ||||||||||||||
useEffect(() => { | ||||||||||||||
if (typeof window !== 'undefined') { | ||||||||||||||
(window as any).__clerk_keyless = true; | ||||||||||||||
} | ||||||||||||||
Comment on lines
+13
to
+15
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. 🛠️ Refactor suggestion Improve type safety for global keyless flag. Instead of using - if (typeof window !== 'undefined') {
- (window as any).__clerk_keyless = true;
- }
+ if (typeof window !== 'undefined') {
+ window.__clerk_keyless = true;
+ } This assumes the property is properly declared in the global.d.ts file as mentioned in the AI summary. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||
if (isNotFoundRoute) { | ||||||||||||||
return; | ||||||||||||||
} | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
'use client'; | ||
|
||
import { useEffect } from 'react'; | ||
|
||
import { useClerk } from '../client-boundary/hooks'; | ||
import { canUseKeyless } from './feature-flags'; | ||
import { NextJSTelemetryCollector } from './telemetry-collector'; | ||
|
||
/** | ||
* Component that enhances the Clerk instance's telemetry collector with keyless boosting | ||
* when running in Next.js keyless mode. | ||
*/ | ||
export const KeylessTelemetryEnhancer = () => { | ||
const clerk = useClerk(); | ||
|
||
useEffect(() => { | ||
// Only enhance telemetry if we're actually in keyless runtime and telemetry exists | ||
const isKeylessRuntime = typeof window !== 'undefined' && (window as any).__clerk_keyless === true; | ||
if (!canUseKeyless || !isKeylessRuntime || !clerk.telemetry) { | ||
return; | ||
} | ||
|
||
// Save original collector and avoid double swapping | ||
const originalCollector = clerk.telemetry; | ||
|
||
// If already wrapped by our NextJSTelemetryCollector, do nothing | ||
if (originalCollector instanceof NextJSTelemetryCollector) { | ||
return; | ||
} | ||
|
||
// Create a new collector that wraps the original with keyless boosting | ||
const enhancedCollector = new NextJSTelemetryCollector({ | ||
disabled: !originalCollector.isEnabled, | ||
debug: originalCollector.isDebug, | ||
publishableKey: clerk.publishableKey, | ||
samplingRate: 1, | ||
sdk: PACKAGE_NAME, | ||
sdkVersion: PACKAGE_VERSION, | ||
}); | ||
|
||
// Replace the telemetry collector | ||
clerk.telemetry = enhancedCollector; | ||
|
||
// Restore the original collector on cleanup (mainly for HMR in dev) | ||
return () => { | ||
if (clerk.telemetry === enhancedCollector) { | ||
clerk.telemetry = originalCollector; | ||
} | ||
}; | ||
}, [clerk]); | ||
|
||
return null; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import type { TelemetryCollectorOptions } from '@clerk/shared/telemetry'; | ||
import { TelemetryCollector } from '@clerk/shared/telemetry'; | ||
import type { TelemetryCollector as TelemetryCollectorInterface, TelemetryEventRaw } from '@clerk/types'; | ||
|
||
/** | ||
* Next.js-specific telemetry collector that boosts sampling rates for component-mounted events | ||
* when running in keyless mode (Next.js app router only). | ||
*/ | ||
export class NextJSTelemetryCollector implements TelemetryCollectorInterface { | ||
#collector: TelemetryCollector; | ||
|
||
constructor(options: TelemetryCollectorOptions) { | ||
this.#collector = new TelemetryCollector(options); | ||
} | ||
|
||
get isEnabled(): boolean { | ||
return this.#collector.isEnabled; | ||
} | ||
|
||
get isDebug(): boolean { | ||
return this.#collector.isDebug; | ||
} | ||
|
||
record(event: TelemetryEventRaw): void { | ||
// Check if we're in keyless mode and this is a component-mounted event | ||
const isKeyless = typeof window !== 'undefined' && (window as any).__clerk_keyless === true; | ||
const isComponentMountedEvent = event.event === 'COMPONENT_MOUNTED'; | ||
|
||
// Boost sampling rate to 100% for component-mounted events in keyless mode. | ||
// Respect explicit 100% overrides; otherwise, override anything below 1.0. | ||
const shouldBoost = isKeyless && isComponentMountedEvent; | ||
const hasExplicitSampling = typeof event.eventSamplingRate === 'number'; | ||
const sampling = hasExplicitSampling ? event.eventSamplingRate : undefined; | ||
const needsBoost = sampling === undefined || sampling < 1.0; | ||
|
||
const boostedEvent: TelemetryEventRaw = shouldBoost && needsBoost ? { ...event, eventSamplingRate: 1.0 } : event; | ||
|
||
this.#collector.record(boostedEvent); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
import { | ||
eventComponentMounted, | ||
eventPrebuiltComponentMounted, | ||
eventPrebuiltComponentOpened, | ||
} from '../telemetry/events/component-mounted'; | ||
|
||
describe('Component-mounted telemetry events', () => { | ||
describe('eventPrebuiltComponentMounted', () => { | ||
test('uses default sampling rate', () => { | ||
const event = eventPrebuiltComponentMounted('SignIn'); | ||
|
||
expect(event.eventSamplingRate).toBe(0.1); | ||
}); | ||
|
||
test('respects explicit sampling rate override', () => { | ||
const event = eventPrebuiltComponentMounted('SignIn', {}, {}, 0.5); | ||
|
||
expect(event.eventSamplingRate).toBe(0.5); | ||
}); | ||
}); | ||
|
||
describe('eventPrebuiltComponentOpened', () => { | ||
test('uses default sampling rate', () => { | ||
const event = eventPrebuiltComponentOpened('GoogleOneTap'); | ||
|
||
expect(event.eventSamplingRate).toBe(0.1); | ||
}); | ||
|
||
test('respects explicit sampling rate override', () => { | ||
const event = eventPrebuiltComponentOpened('GoogleOneTap', {}, {}, 0.25); | ||
|
||
expect(event.eventSamplingRate).toBe(0.25); | ||
}); | ||
}); | ||
|
||
describe('eventComponentMounted', () => { | ||
test('uses default sampling rate', () => { | ||
const event = eventComponentMounted('CustomComponent'); | ||
|
||
expect(event.eventSamplingRate).toBe(0.1); | ||
}); | ||
|
||
test('respects explicit sampling rate override', () => { | ||
const event = eventComponentMounted('CustomComponent', {}, 0.75); | ||
|
||
expect(event.eventSamplingRate).toBe(0.75); | ||
}); | ||
|
||
test('includes component name and props in payload', () => { | ||
const event = eventComponentMounted('MyComponent', { custom: 'data' }); | ||
|
||
expect(event.payload).toMatchObject({ | ||
component: 'MyComponent', | ||
custom: 'data', | ||
}); | ||
}); | ||
}); | ||
|
||
describe('payload structure', () => { | ||
test('eventPrebuiltComponentMounted includes appearance tracking', () => { | ||
const event = eventPrebuiltComponentMounted('SignIn', { | ||
appearance: { | ||
baseTheme: 'dark', | ||
elements: { card: 'custom' }, | ||
variables: { colorPrimary: 'blue' }, | ||
}, | ||
}); | ||
|
||
expect(event.payload).toMatchObject({ | ||
component: 'SignIn', | ||
appearanceProp: true, | ||
baseTheme: true, | ||
elements: true, | ||
variables: true, | ||
}); | ||
}); | ||
|
||
test('eventPrebuiltComponentMounted tracks absence of appearance props', () => { | ||
const event = eventPrebuiltComponentMounted('SignIn', {}); | ||
|
||
expect(event.payload).toMatchObject({ | ||
component: 'SignIn', | ||
appearanceProp: false, | ||
baseTheme: false, | ||
elements: false, | ||
variables: false, | ||
}); | ||
}); | ||
|
||
test('includes additional payload data', () => { | ||
const event = eventPrebuiltComponentMounted('SignIn', {}, { customData: 'test' }); | ||
|
||
expect(event.payload).toMatchObject({ | ||
component: 'SignIn', | ||
customData: 'test', | ||
}); | ||
}); | ||
}); | ||
}); |
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.
🛠️ Refactor suggestion
Address ESLint violation while maintaining test functionality.
The test logic for verifying sampling rate boosting is sound, but the
require()
usage violates the ESLint rule. Since this is needed for dynamic re-importing after mocking, consider using dynamic import with proper typing.You'll need to make the test function
async
and addawait
before the import statement to maintain the same functionality while adhering to ESLint rules.🧰 Tools
🪛 ESLint
[error] 70-70: A
require()
style import is forbidden.(@typescript-eslint/no-require-imports)
🤖 Prompt for AI Agents