feat(cloudflare): Instrument Flagship bindings in instrumentEnv#21244
feat(cloudflare): Instrument Flagship bindings in instrumentEnv#21244nehaprasad-dev wants to merge 2 commits into
Conversation
nehaprasad-dev
requested review from
JPeer264 and
andreiborza
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 11bed6e. Configure here.
| const flagKey = args[0]; | ||
| if (typeof flagKey === 'string') { | ||
| recordFlagEvaluation(flagKey, result); | ||
| } |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit 11bed6e. Configure here.
|
|
||
| expect(wrapped.appId).toBe('app-123'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 11bed6e. Configure here.
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
There are two more things:
- The new unit tests are failing in the CI, please make sure they are running through locally. You could go into
packages/cloudflareand run the tests directly in the folder withyarn test - The files need to be formatted, you can run
yarn fixin the root of your project.
Once all things were addressed I'll take another look.
|
|
||
| if (p === 'fetch' && typeof value === 'function') { | ||
| return instrumentFetcher((...args) => Reflect.apply(value, target, args)); | ||
| return instrumentFetcher((...args: unknown[]) => Reflect.apply(value, target, args)); |
There was a problem hiding this comment.
m: Has this been added on purpose? We should rather remove it and keep the types here
| 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 { |
There was a problem hiding this comment.
m: If you update the @cloudflare/workers-types to the latest version you could use the following:
| 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' && |
There was a problem hiding this comment.
m: All these type assertions can be removed, as they already have this type
| typeof (item as Record<string, unknown>).getBooleanValue === 'function' && | |
| typeof item.getBooleanValue === 'function' && |
| return async (...args: unknown[]) => { | ||
| const result = await Reflect.apply(original, target, args); | ||
|
|
||
| if (prop.endsWith('Details') && isEvaluationDetails(result)) { |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,116 @@ | |||
| import * as SentryCore from '@sentry/core'; | |||
There was a problem hiding this comment.
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
2 participants
Before submitting a pull request, please take a look at our Contributing guidelines and verify:
fix : #21184