feat(core,cloudflare): Enable certain fields with env variables #19245
Conversation
| CfHostMetadata | ||
| >, | ||
| >(optionsCallback: (env: Env) => CloudflareOptions, handler: T): T { | ||
| >(optionsCallback: (env: Env) => CloudflareOptions | undefined, handler: T): T { |
There was a problem hiding this comment.
note: I made this return undefined as it was also working before theoretically. I adapted the types to also allow undefined just to have this case also handled
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/cloudflare/src/options.ts
Outdated
| : undefined; | ||
|
|
||
| return { release, ...userOptions }; | ||
| const tracesSampleRate = userOptions.tracesSampleRate ?? parseFloat(getEnvVar(env, 'SENTRY_TRACE_SAMPLE_RATE') ?? ''); |
There was a problem hiding this comment.
Env variable name typo: missing 'S' in TRACES
High Severity
The code reads SENTRY_TRACE_SAMPLE_RATE (missing the 'S' in "TRACES"), but the standard Sentry env variable name is SENTRY_TRACES_SAMPLE_RATE. The tests also use SENTRY_TRACES_SAMPLE_RATE. This mismatch means the traces sample rate env variable will never be read, making the feature non-functional. This also means the related tests will fail.
| tracesSampleRate: isFinite(tracesSampleRate) ? tracesSampleRate : undefined, | ||
| release, | ||
| ...userOptions, | ||
| }; |
There was a problem hiding this comment.
Spread operator overrides env-derived fallback values
Medium Severity
The ...userOptions spread at the end of the return object overrides the env-derived fallback values for dsn, environment, and tracesSampleRate whenever those keys exist in userOptions — even if their values are undefined. This means if a user passes { dsn: undefined }, the SENTRY_DSN env fallback is silently ignored. The ?? fallback logic and the isFinite guard are both bypassed by the spread.
Codecov Results 📊Generated by Codecov Action |
| const tracesSampleRate = userOptions.tracesSampleRate ?? parseFloat(getEnvVar(env, 'SENTRY_TRACE_SAMPLE_RATE') ?? ''); | ||
|
|
||
| return { | ||
| dsn: userOptions.dsn ?? getEnvVar(env, 'SENTRY_DSN'), |
There was a problem hiding this comment.
l: can we also add SENTRY_DEBUG? You could use envToBool for that from node-core, maybe lift that up into @sentry/core?
There was a problem hiding this comment.
Great idea. Added.
I also added tunnel to it
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| dsn: userOptions.dsn ?? getEnvVar(env, 'SENTRY_DSN'), | ||
| environment: userOptions.environment ?? getEnvVar(env, 'SENTRY_ENVIRONMENT'), | ||
| tracesSampleRate: isFinite(tracesSampleRate) ? tracesSampleRate : undefined, | ||
| debug: userOptions.debug ?? envToBool(getEnvVar(env, 'SENTRY_DEBUG')), |
There was a problem hiding this comment.
envToBool(undefined) forces debug: false when unset
Medium Severity
When SENTRY_DEBUG is absent from env, getEnvVar returns undefined, and envToBool(undefined) returns false (since Boolean(undefined) is false in loose mode). This means debug is always explicitly set to false in the returned options, even when neither the user nor the env specified it. This breaks multiple existing toEqual assertions (e.g., lines 29, 120, 132) that don't expect a debug property.
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
JPeer264
changed the title
| }; | ||
|
|
||
| // Mock env without DSN for tests that should not initialize the SDK | ||
| const MOCK_ENV_WITHOUT_DSN = { |
There was a problem hiding this comment.
note: It seems that the SDK was not initialized for the tests where this is used. By now also allowing SENTRY_DSN from the variables these tests fail. I'll create a follow up PR and remove this, so the SDK is actually initialized (I don't want to blow up this PR with other code changes)
JPeer264
force-pushed
the
jp/env-variables-in-cf
branch
from
b7264d0 to
6c2f692
Compare
2 participants
It was not possible to use env variables for the Cloudflare SDK. This adds a subset of env variables to enable certain features just with env variables (not everything is yet supported - we should wait what gets supported once that PR lands).
This PR is important for #19215.