Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { CloudflareOptions } from '../../client';
import { isDurableObjectNamespace, isJSRPC, isQueue } from '../../utils/isBinding';
import { isDurableObjectNamespace, isFlagship, isJSRPC, isQueue } from '../../utils/isBinding';
import { instrumentFlagship } from './instrumentFlagship';
import { appendRpcMeta } from '../../utils/rpcMeta';
import { getEffectiveRpcPropagation } from '../../utils/rpcOptions';
import { instrumentDurableObjectNamespace, STUB_NON_RPC_METHODS } from '../instrumentDurableObjectNamespace';
Expand Down Expand Up @@ -54,6 +55,12 @@ export function instrumentEnv<Env extends Record<string, unknown>>(env: Env, opt
return instrumented;
}

if (isFlagship(item)) {
const instrumented = instrumentFlagship(item);
instrumentedBindings.set(item, instrumented);
return instrumented;
}

if (!rpcPropagation) {
return item;
}
Expand All @@ -70,7 +77,7 @@ export function instrumentEnv<Env extends Record<string, unknown>>(env: Env, opt
const value = Reflect.get(target, p);

if (p === 'fetch' && typeof value === 'function') {
return instrumentFetcher((...args) => Reflect.apply(value, target, args));
return instrumentFetcher((...args: unknown[]) => Reflect.apply(value, target, args));
Copy link
Copy Markdown
Member

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

}

if (typeof value === 'function' && typeof p === 'string' && !STUB_NON_RPC_METHODS.has(p)) {
Expand Down
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)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 insertFlagToScope we ignore all other values than boolean. Since the future could bring more values, it is alright to already support them here, to not adapt multiple places.

recordFlagEvaluation(result.flagKey, result.value);
return result;
}

const flagKey = args[0];
if (typeof flagKey === 'string') {
recordFlagEvaluation(flagKey, result);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing boolean check before recording flag evaluations

High Severity

recordFlagEvaluation is called for all evaluation results without checking if the value is boolean. For the non-Details branch, a typeof result === 'boolean' check is missing before calling recordFlagEvaluation. For the Details branch, a typeof result.value === 'boolean' check is missing. The tests clearly expect only boolean values to be recorded (e.g., getStringValue and getStringDetails should NOT trigger recording), but the current code calls _INTERNAL_insertFlagToScope and _INTERNAL_addFeatureFlagToActiveSpan unconditionally — causing the test spies to register calls that the assertions expect not to happen.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 11bed6e. Configure here.


return result;
};
},
});
}
9 changes: 9 additions & 0 deletions packages/cloudflare/src/utils/isBinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: If you update the @cloudflare/workers-types to the latest version you could use the following:

Suggested change
export function isFlagship(item: unknown): boolean {
export function isFlagship(item: unknown): item is Flaghship {

return (
item != null &&
isNotJSRPC(item) &&
typeof (item as Record<string, unknown>).getBooleanValue === 'function' &&

Check failure on line 74 in packages/cloudflare/src/utils/isBinding.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-unnecessary-type-assertion)

This assertion is unnecessary since it does not change the type of the expression.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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>).getBooleanValue === 'function' &&
typeof item.getBooleanValue === 'function' &&

typeof (item as Record<string, unknown>).getStringValue === 'function' &&

Check failure on line 75 in packages/cloudflare/src/utils/isBinding.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-unnecessary-type-assertion)

This assertion is unnecessary since it does not change the type of the expression.
typeof (item as Record<string, unknown>).getBooleanDetails === 'function'

Check failure on line 76 in packages/cloudflare/src/utils/isBinding.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-unnecessary-type-assertion)

This assertion is unnecessary since it does not change the type of the expression.
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import * as SentryCore from '@sentry/core';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 cloudflare-worker E2E test and create a featureflag.test.ts with some tests that the instrumentation is actually working within miniflare

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

View workflow job for this annotation

GitHub Actions / Node (26) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from typed value methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "variant-a", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 42, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "enabled": true, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:59:27

Check failure on line 59 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (20) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from typed value methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "variant-a", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 42, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "enabled": true, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:59:27

Check failure on line 59 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (22) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from typed value methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "variant-a", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 42, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "enabled": true, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:59:27

Check failure on line 59 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (24) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from typed value methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "variant-a", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 42, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "enabled": true, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:59:27

Check failure on line 59 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (18) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from typed value methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "variant-a", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 42, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "enabled": true, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:59:27
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

View workflow job for this annotation

GitHub Actions / Node (26) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > records boolean values from get() when the result is boolean

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 1 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "string-flag", "not-a-boolean", ] Number of calls: 1 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:72:27

Check failure on line 72 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (20) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > records boolean values from get() when the result is boolean

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 1 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "string-flag", "not-a-boolean", ] Number of calls: 1 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:72:27

Check failure on line 72 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (22) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > records boolean values from get() when the result is boolean

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 1 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "string-flag", "not-a-boolean", ] Number of calls: 1 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:72:27

Check failure on line 72 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (24) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > records boolean values from get() when the result is boolean

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 1 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "string-flag", "not-a-boolean", ] Number of calls: 1 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:72:27

Check failure on line 72 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (18) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > records boolean values from get() when the result is boolean

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 1 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "string-flag", "not-a-boolean", ] Number of calls: 1 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:72:27
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

View workflow job for this annotation

GitHub Actions / Node (26) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from details methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "v2", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 5, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "size": 16, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:106:27

Check failure on line 106 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (20) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from details methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "v2", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 5, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "size": 16, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:106:27

Check failure on line 106 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (22) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from details methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "v2", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 5, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "size": 16, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:106:27

Check failure on line 106 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (24) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from details methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "v2", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 5, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "size": 16, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:106:27

Check failure on line 106 in packages/cloudflare/test/instrumentations/worker/instrumentFlagship.test.ts

View workflow job for this annotation

GitHub Actions / Node (18) Unit Tests

test/instrumentations/worker/instrumentFlagship.test.ts > instrumentFlagship > does not record non-boolean values from details methods

AssertionError: expected "_INTERNAL_insertFlagToScope" to not be called at all, but actually been called 3 times Received: 1st _INTERNAL_insertFlagToScope call: Array [ "checkout-flow", "v2", ] 2nd _INTERNAL_insertFlagToScope call: Array [ "max-retries", 5, ] 3rd _INTERNAL_insertFlagToScope call: Array [ "theme-config", Object { "size": 16, }, ] Number of calls: 3 ❯ test/instrumentations/worker/instrumentFlagship.test.ts:106:27
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');
});
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing integration or E2E test for feat PR

Low Severity

Per the review rules, feat PRs require at least one integration or E2E test. This PR only includes a unit test for instrumentFlagship. No integration test exercises the full flow through instrumentEnv detecting a Flagship binding and producing the expected telemetry, and no E2E test exists in dev-packages.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 11bed6e. Configure here.

Loading