-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(cloudflare): Instrument Flagship bindings in instrumentEnv #21244
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
base: develop
Are you sure you want to change the base?
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,65 @@ | ||
| import { | ||
| _INTERNAL_addFeatureFlagToActiveSpan, | ||
| _INTERNAL_insertFlagToScope, | ||
| } from '@sentry/core'; | ||
|
|
||
| const EVALUATION_METHODS = new Set([ | ||
| 'get', | ||
| 'getBooleanValue', | ||
| 'getStringValue', | ||
| 'getNumberValue', | ||
| 'getObjectValue', | ||
| 'getBooleanDetails', | ||
| 'getStringDetails', | ||
| 'getNumberDetails', | ||
| 'getObjectDetails', | ||
| ]); | ||
|
|
||
| type FlagshipEvaluationDetails = { | ||
| flagKey: string; | ||
| value: unknown; | ||
| }; | ||
|
|
||
| function isEvaluationDetails(value: unknown): value is FlagshipEvaluationDetails { | ||
| return ( | ||
| value != null && | ||
| typeof value === 'object' && | ||
| 'flagKey' in value && | ||
| typeof (value as FlagshipEvaluationDetails).flagKey === 'string' && | ||
| 'value' in value | ||
| ); | ||
| } | ||
|
|
||
| function recordFlagEvaluation(flagKey: string, value: unknown): void { | ||
| _INTERNAL_insertFlagToScope(flagKey, value); | ||
| _INTERNAL_addFeatureFlagToActiveSpan(flagKey, value); | ||
| } | ||
| export function instrumentFlagship<T extends object>(flagship: T): T { | ||
| return new Proxy(flagship, { | ||
| get(target, prop, receiver) { | ||
| const value = Reflect.get(target, prop, receiver); | ||
|
|
||
| if (typeof prop !== 'string' || !EVALUATION_METHODS.has(prop) || typeof value !== 'function') { | ||
| return value; | ||
| } | ||
|
|
||
| const original = value as (...args: unknown[]) => unknown; | ||
|
|
||
| return async (...args: unknown[]) => { | ||
| const result = await Reflect.apply(original, target, args); | ||
|
|
||
| if (prop.endsWith('Details') && isEvaluationDetails(result)) { | ||
|
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. Just for the record (this can be ignored for the PR changes): Currently we only support boolean flags, which means, inside |
||
| recordFlagEvaluation(result.flagKey, result.value); | ||
| return result; | ||
| } | ||
|
|
||
| const flagKey = args[0]; | ||
| if (typeof flagKey === 'string') { | ||
| recordFlagEvaluation(flagKey, result); | ||
| } | ||
|
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. Missing boolean check before recording flag evaluationsHigh Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 11bed6e. Configure here. |
||
|
|
||
| return result; | ||
| }; | ||
| }, | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -67,3 +67,12 @@ | |||||
| export function isQueue(item: unknown): item is Queue { | ||||||
| return item != null && isNotJSRPC(item) && typeof item.send === 'function' && typeof item.sendBatch === 'function'; | ||||||
| } | ||||||
| export function isFlagship(item: unknown): boolean { | ||||||
|
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. m: If you update the
Suggested change
|
||||||
| return ( | ||||||
| item != null && | ||||||
| isNotJSRPC(item) && | ||||||
| typeof (item as Record<string, unknown>).getBooleanValue === 'function' && | ||||||
|
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. m: All these type assertions can be removed, as they already have this type
Suggested change
|
||||||
| typeof (item as Record<string, unknown>).getStringValue === 'function' && | ||||||
| typeof (item as Record<string, unknown>).getBooleanDetails === 'function' | ||||||
| ); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| import * as SentryCore from '@sentry/core'; | ||
|
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. h: For our unit tests we don't use miniflare to verify that it would actually work in a Cloudflare environment. In theory a Cloudflare integration tests would be enough, but we can't update wrangler in our integration tests (as we are bound to a specific node version for now), so I would ask you to add a E2E test for it instead. You could add a new route in the existing |
||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { instrumentFlagship } from '../../../src/instrumentations/worker/instrumentFlagship'; | ||
|
|
||
| function createMockFlagship() { | ||
| return { | ||
| get: vi.fn().mockResolvedValue(true), | ||
| getBooleanValue: vi.fn().mockResolvedValue(true), | ||
| getStringValue: vi.fn().mockResolvedValue('variant-a'), | ||
| getNumberValue: vi.fn().mockResolvedValue(42), | ||
| getObjectValue: vi.fn().mockResolvedValue({ enabled: true }), | ||
| getBooleanDetails: vi.fn().mockResolvedValue({ flagKey: 'dark-mode', value: true }), | ||
| getStringDetails: vi.fn().mockResolvedValue({ flagKey: 'checkout-flow', value: 'v2' }), | ||
| getNumberDetails: vi.fn().mockResolvedValue({ flagKey: 'max-retries', value: 5 }), | ||
| getObjectDetails: vi.fn().mockResolvedValue({ flagKey: 'theme-config', value: { size: 16 } }), | ||
| }; | ||
| } | ||
|
|
||
| describe('instrumentFlagship', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('forwards evaluation calls to the underlying binding', async () => { | ||
| const flagship = createMockFlagship(); | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| await wrapped.getBooleanValue('new-checkout', false, { userId: 'user-42' }); | ||
|
|
||
| expect(flagship.getBooleanValue).toHaveBeenCalledWith('new-checkout', false, { userId: 'user-42' }); | ||
| }); | ||
|
|
||
| it('records boolean values from getBooleanValue on the active span and scope', async () => { | ||
| vi.spyOn(SentryCore, 'getActiveSpan').mockReturnValue({ setAttribute: vi.fn() } as any); | ||
| vi.spyOn(SentryCore, 'spanToJSON').mockReturnValue({ data: {} } as any); | ||
| const insertSpy = vi.spyOn(SentryCore, '_INTERNAL_insertFlagToScope'); | ||
| const spanSpy = vi.spyOn(SentryCore, '_INTERNAL_addFeatureFlagToActiveSpan'); | ||
|
|
||
| const flagship = createMockFlagship(); | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| await wrapped.getBooleanValue('new-checkout', false); | ||
|
|
||
| expect(insertSpy).toHaveBeenCalledWith('new-checkout', true); | ||
| expect(spanSpy).toHaveBeenCalledWith('new-checkout', true); | ||
| }); | ||
|
|
||
| it('does not record non-boolean values from typed value methods', async () => { | ||
| const insertSpy = vi.spyOn(SentryCore, '_INTERNAL_insertFlagToScope'); | ||
| const spanSpy = vi.spyOn(SentryCore, '_INTERNAL_addFeatureFlagToActiveSpan'); | ||
|
|
||
| const flagship = createMockFlagship(); | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| await wrapped.getStringValue('checkout-flow', 'v1'); | ||
| await wrapped.getNumberValue('max-retries', 3); | ||
| await wrapped.getObjectValue('theme-config', { size: 14 }); | ||
|
|
||
| expect(insertSpy).not.toHaveBeenCalled(); | ||
|
Check failure on line 59 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts
|
||
| expect(spanSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('records boolean values from get() when the result is boolean', async () => { | ||
| const insertSpy = vi.spyOn(SentryCore, '_INTERNAL_insertFlagToScope'); | ||
| const spanSpy = vi.spyOn(SentryCore, '_INTERNAL_addFeatureFlagToActiveSpan'); | ||
|
|
||
| const flagship = createMockFlagship(); | ||
| flagship.get.mockResolvedValueOnce('not-a-boolean'); | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| await wrapped.get('string-flag', 'default'); | ||
| expect(insertSpy).not.toHaveBeenCalled(); | ||
|
Check failure on line 72 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts
|
||
| expect(spanSpy).not.toHaveBeenCalled(); | ||
|
|
||
| await wrapped.get('bool-flag', false); | ||
| expect(insertSpy).toHaveBeenCalledWith('bool-flag', true); | ||
| expect(spanSpy).toHaveBeenCalledWith('bool-flag', true); | ||
| }); | ||
|
|
||
| it('records boolean values from details methods using the returned metadata', async () => { | ||
| vi.spyOn(SentryCore, 'getActiveSpan').mockReturnValue({ setAttribute: vi.fn() } as any); | ||
| vi.spyOn(SentryCore, 'spanToJSON').mockReturnValue({ data: {} } as any); | ||
| const insertSpy = vi.spyOn(SentryCore, '_INTERNAL_insertFlagToScope'); | ||
| const spanSpy = vi.spyOn(SentryCore, '_INTERNAL_addFeatureFlagToActiveSpan'); | ||
|
|
||
| const flagship = createMockFlagship(); | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| await wrapped.getBooleanDetails('dark-mode', false); | ||
|
|
||
| expect(insertSpy).toHaveBeenCalledWith('dark-mode', true); | ||
| expect(spanSpy).toHaveBeenCalledWith('dark-mode', true); | ||
| }); | ||
|
|
||
| it('does not record non-boolean values from details methods', async () => { | ||
| const insertSpy = vi.spyOn(SentryCore, '_INTERNAL_insertFlagToScope'); | ||
| const spanSpy = vi.spyOn(SentryCore, '_INTERNAL_addFeatureFlagToActiveSpan'); | ||
|
|
||
| const flagship = createMockFlagship(); | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| await wrapped.getStringDetails('checkout-flow', 'v1'); | ||
| await wrapped.getNumberDetails('max-retries', 3); | ||
| await wrapped.getObjectDetails('theme-config', { size: 14 }); | ||
|
|
||
| expect(insertSpy).not.toHaveBeenCalled(); | ||
|
Check failure on line 106 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts
|
||
| expect(spanSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('passes through non-evaluation properties unchanged', () => { | ||
| const flagship = { ...createMockFlagship(), appId: 'app-123' }; | ||
| const wrapped = instrumentFlagship(flagship); | ||
|
|
||
| expect(wrapped.appId).toBe('app-123'); | ||
| }); | ||
| }); | ||
|
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. Missing integration or E2E test for feat PRLow Severity Per the review rules, Triggered by project rule: PR Review Guidelines for Cursor Bot Reviewed by Cursor Bugbot for commit 11bed6e. Configure here. |
||


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.
m: Has this been added on purpose? We should rather remove it and keep the types here