feat: add user/app context in controlplane sentry #2335
Conversation
- also added appVersion to Sentry release
miklosbarabas
requested review from
JivusAyrus,
StarpTech,
thisisnithin and
wilsonrivera
as code owners
WalkthroughThis PR integrates Sentry error tracking into the controlplane with release version extraction, authentication context enrichment, and post-authentication hooks. Changes span Sentry initialization configuration, authentication service enhancement, build server setup, and configuration propagation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
miklosbarabas
changed the title
miklosbarabas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/util.ts (1)
46-49: Consider importing parsed env vars instead of re-parsing.The environment variables are already parsed in
sentry.config.ts(line 17). Parsingprocess.envtwice adds unnecessary overhead and could theoretically lead to inconsistencies.Consider exporting the parsed values from
sentry.config.tsor creating a shared module:// In env.schema.ts or sentry.config.ts export const sentryConfig = { SENTRY_ENABLED, SENTRY_DSN, // ... other Sentry vars };Then import in util.ts:
-const { - SENTRY_ENABLED, - SENTRY_DSN, -} = envVariables.parse(process.env); +import { sentryConfig } from './sentry.config.js'; +const { SENTRY_ENABLED, SENTRY_DSN } = sentryConfig;controlplane/src/core/sentry.config.ts (1)
20-27: Consider using a proper logger instead of console.debug.The codebase uses
FastifyBaseLoggerthroughout (e.g., line 59, 66 in util.ts). Usingconsole.debughere is inconsistent and may not be captured by the logging pipeline.Since this runs during module initialization before the logger is available,
console.debugmight be the only option. However, consider documenting this or usingconsole.errorfor better visibility:} catch (error) { - console.debug('Sentry: failed to read package.json version for release', error); + console.error('Sentry: failed to read package.json version for release', error); }Additionally, the path
../../package.jsonassumes a specific directory structure. This is generally fine but could break if the build output structure changes. Consider documenting this assumption or using a more robust path resolution if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/src/core/sentry.config.ts(2 hunks)controlplane/src/core/util.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/util.ts
🧬 Code graph analysis (1)
controlplane/src/core/util.ts (1)
controlplane/src/core/env.schema.ts (1)
envVariables(3-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
controlplane/src/core/sentry.config.ts (1)
29-31: LGTM: Release version properly integrated.The release parameter correctly passes the package version to Sentry. Handling
undefinedgracefully if the read fails is appropriate, as release is optional metadata.
- also added appVersion to Sentry release
- also added appVersion to Sentry release
- also added appVersion to Sentry release
- also added appVersion to Sentry release
- also added appVersion to Sentry release
- also added appVersion to Sentry release
- also added appVersion to Sentry release
- also added appVersion to Sentry release
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/controlplane-ci.yaml (1)
83-84: Error handling is optional but recommended for CI robustness.Verification confirms that
controlplane/.env.exampleexists and is committed to the repository, so the current code will work as-is. However, adding error handling remains a good practice in CI/CD workflows to fail fast and provide clear diagnostics if the file is ever moved or removed.If you'd like to make the step more defensive, consider:
- - name: Copy dummy env file - run: cp ./controlplane/.env.example ./controlplane/.env + - name: Copy dummy env file + run: test -f ./controlplane/.env.example && cp ./controlplane/.env.example ./controlplane/.env || { echo "ERROR: .env.example not found"; exit 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/controlplane-ci.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
controlplane/src/core/util.ts
Outdated
| const schemaTagRegex = /^(?![/-])[\d/A-Za-z-]+(?<![/-])$/; | ||
| const graphNameRegex = /^[\dA-Za-z]+(?:[./@_-][\dA-Za-z]+)*$/; | ||
| const pluginVersionRegex = /^v\d+$/; | ||
| const { SENTRY_ENABLED, SENTRY_DSN } = envVariables.parse(process.env); |
There was a problem hiding this comment.
There should be only the main entry file that interacts with globals directly.
There was a problem hiding this comment.
so how would you use those envvars in here?
controlplane/src/core/util.ts
Outdated
| try { | ||
| // NB: Fastify integration automatically creates request-specific scopes | ||
| if (authContext.userId) { | ||
| Sentry.setUser({ |
There was a problem hiding this comment.
Isn't sentry a noop if disabled?
There was a problem hiding this comment.
Not sure, but would be better not to run on this code path at all if Sentry is not enabled.
| - name: Setup Keycloak | ||
| run: nohup .github/scripts/setup-keycloak.sh & | ||
|
|
||
| - name: Copy dummy env file |
There was a problem hiding this comment.
Why is it necessary? To me it looks like it would be better to set necessary env in the test setup file.
There was a problem hiding this comment.
This was required for Vitest once I added this, it was requiring all envvars not just the imported/parsed two:
const { SENTRY_ENABLED, SENTRY_DSN } = envVariables.parse(process.env);
Do you have any suggestion instead?
- also added appVersion to Sentry release
controlplane/src/core/util.ts
Outdated
| }, | ||
| }); | ||
|
|
||
| if (SENTRY_ENABLED && SENTRY_DSN) { |
There was a problem hiding this comment.
What's the reason why we initialize Sentry within enrichLogger? How are they related?
There was a problem hiding this comment.
There're couple of reasons why it was chosen:
enrichLogger(ctx, logger, authContext)is the first point in each Connect handler where you have:authContext(user/org/api-key/graph info), and- ctx (request-scoped context tied to the Fastify request via contextValues in
src/core/build-server.ts:493).
- That's also the only shared helper called from all the bufservice handlers immediately after opts.authenticator.authenticate(...).
From a design/concerns perspective, conceptually, Sentry context would be better handled by a Sentry-specific helper, not a logging helper. Moving Sentry into Authentication or a Fastify hook in build-server.ts sounds attractive, but:
- Authentication doesn't have access to HandlerContext/Fastify logger.
- The Fastify contextValues hook in
build-server.tsdoesn’t have authContext (auth hasn’t run yet). - To do that properly we'd need a custom Connect interceptor/middleware layer, which is a more invasive change.
- also added appVersion to Sentry release
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/build-server.ts (1)
11-11: Sentry import and BuildConfig.sentry extension are reasonableImporting
@sentry/nodehere and extendingBuildConfigwith asentryblock is coherent with howbuild()later usesopts.sentry.enabled/opts.sentry.dsn.One thing to keep in mind:
sentryis now required onBuildConfig. TypeScript call sites are forced to provide it, but any plain JS caller of the compiledbuild()must also pass a compatiblesentryobject or they’ll see a runtime error when accessingopts.sentry.enabled. If you expect non-TS callers, consider either makingsentryoptional and guarding withopts.sentry?.enabledor defaulting it before use.Also applies to: 140-143
controlplane/src/core/services/Authentication.ts (1)
50-63: Decouple post-auth hook failures from authentication failuresThe hook is correctly invoked in all
AuthContextpaths (API key, access token, new/cached web session), which is exactly what you want for telemetry. However,authenticate()wraps the entire body in a single try/catch that maps any thrown error toAuthenticationError(ERROR_NOT_AUTHENTICATED, 'Not authenticated'). That means any futurePostAuthHookimplementation that throws will cause otherwise-valid authentications to fail.The current Sentry hook defensively wraps its own logic in a try/catch, so it’s safe, but this contract is easy to violate later.
Consider guarding the hook at the
Authenticationlevel, e.g.:private invokePostAuthHook(authContext: AuthContext) { if (!this.postAuthHook) return; try { this.postAuthHook(authContext); } catch { // optional: log at debug/info; avoid affecting auth result } }and then replacing each direct
this.postAuthHook(...)call withthis.invokePostAuthHook(...). That keeps authentication semantics stable even if a hook misbehaves.Also applies to: 79-83, 118-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controlplane/src/core/build-server.ts(3 hunks)controlplane/src/core/sentry.config.ts(2 hunks)controlplane/src/core/services/Authentication.ts(5 hunks)controlplane/src/index.ts(1 hunks)controlplane/test/authentication.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/sentry.config.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/services/Authentication.ts
🧬 Code graph analysis (2)
controlplane/src/core/build-server.ts (1)
controlplane/src/core/services/Authentication.ts (1)
Authentication(22-142)
controlplane/src/core/services/Authentication.ts (1)
controlplane/src/types/index.ts (1)
AuthContext(478-487)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
controlplane/test/authentication.test.ts (1)
75-78: Explicit Sentry config in test setup looks goodProviding an explicit
sentryblock here keeps the test aligned with the updatedBuildConfigshape and ensures Sentry stays disabled during this test run. Nothing to change.controlplane/src/index.ts (1)
161-164: Sentry env → BuildConfig wiring is consistentMapping
SENTRY_ENABLED/SENTRY_DSNintooptions.sentrymatches the updatedBuildConfigshape and keeps Sentry enablement centrally controlled via env + config. No changes needed here.controlplane/src/core/services/Authentication.ts (1)
15-16: PostAuthHook type and constructor extension are cleanExposing
PostAuthHook = (authContext: AuthContext) => voidand threading it into theAuthenticationconstructor gives a clear, minimal extension point for post-auth side effects (like Sentry enrichment) without touching call sites ofauthenticate().No issues here; the signature is straightforward and keeps the core auth API intact.
Also applies to: 25-32
| const authenticator = new Authentication( | ||
| webAuth, | ||
| apiKeyAuth, | ||
| accessTokenAuth, | ||
| graphKeyAuth, | ||
| organizationRepository, | ||
| (authContext) => { | ||
| if (opts.sentry.enabled && opts.sentry.dsn) { | ||
| try { | ||
| Sentry.setUser({ | ||
| id: authContext.userId, | ||
| username: authContext.userDisplayName, | ||
| }); | ||
|
|
||
| Sentry.setTag('org.id', authContext.organizationId); | ||
|
|
||
| if (authContext.organizationSlug) { | ||
| Sentry.setTag('org.slug', authContext.organizationSlug); | ||
| } | ||
|
|
||
| if (authContext.apiKeyName) { | ||
| Sentry.setTag('api.key', authContext.apiKeyName); | ||
| } | ||
|
|
||
| Sentry.setTag('auth.kind', authContext.auth); | ||
| } catch (error) { | ||
| logger.debug({ err: error }, 'Failed to enrich Sentry user context'); | ||
| } | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Post-auth Sentry context enrichment looks correct; confirm PII/data-handling expectations
The Sentry enrichment hook is defensively implemented:
- It’s gated on
opts.sentry.enabled && opts.sentry.dsn. - All Sentry calls are in a try/catch with debug logging on failure, so instrumentation can’t break auth.
- Fields used from
authContext(userId, userDisplayName, organizationId, organizationSlug, apiKeyName, auth) match theAuthContextshape and are guarded where optional.
Functionally this achieves the goal of enriching Sentry with user/org/auth context. The main consideration is that you’re now sending user identifiers (userId, userDisplayName) and org-identifying data (organizationId, organizationSlug, apiKeyName) to Sentry on every authenticated request. Please double-check that this aligns with your privacy/PII and data-retention policies for production Sentry projects (e.g., whether user display names are acceptable, or if they should be pseudonymized).
🏁 Script executed:
#!/bin/bash
# Verify the code snippet in build-server.ts and check surrounding context
echo "=== Lines 257-287 in build-server.ts ==="
sed -n '257,287p' controlplane/src/core/build-server.ts
echo -e "\n=== Lines around 257-287 for context ==="
sed -n '250,295p' controlplane/src/core/build-server.tsLength of output: 2624
🏁 Script executed:
#!/bin/bash
# Find AuthContext type definition and check field optionality
echo "=== Searching for AuthContext type definition ==="
rg -n "type AuthContext|interface AuthContext" --type ts
echo -e "\n=== Context around AuthContext definition ==="
rg -nA 15 "type AuthContext|interface AuthContext" --type tsLength of output: 1116
🏁 Script executed:
#!/bin/bash
# Search for PII, privacy, or data-retention related documentation
echo "=== Searching for PII/privacy related comments or docs ==="
rg -n "PII|private identif|data.retention|privacy" --type ts --type md -i controlplane/
echo -e "\n=== Searching for Sentry configuration or documentation ==="
fd -e md -e txt -e json | xargs rg -l "sentry|PII" 2>/dev/null | head -10Length of output: 438
🏁 Script executed:
#!/bin/bash
# Check if this code is new or existing, and verify field guards
echo "=== Checking for optional field guards around organizationSlug and apiKeyName ==="
sed -n '275,283p' controlplane/src/core/build-server.tsLength of output: 371
🏁 Script executed:
#!/bin/bash
# Examine the Sentry configuration to understand PII handling
echo "=== sentry.config.ts full content ==="
cat controlplane/src/core/sentry.config.ts
echo -e "\n=== Check env.schema.ts for SENTRY_SEND_DEFAULT_PII definition ==="
sed -n '195,210p' controlplane/src/core/env.schema.tsLength of output: 2269
🏁 Script executed:
#!/bin/bash
# Search for how enrichment context is used elsewhere and if PII flag is checked
echo "=== Check if enrichment callback checks SENTRY_SEND_DEFAULT_PII anywhere ==="
rg -n "SENTRY_SEND_DEFAULT_PII|authContext.*setUser|authContext.*setTag" --type ts controlplane/src/core/
echo -e "\n=== Check Sentry docs or comments about user/tag PII ==="
rg -n "setUser|setTag" --type ts -B 2 -A 2 controlplane/src/core/build-server.tsLength of output: 1105
🏁 Script executed:
#!/bin/bash
# Find where opts.sentry is constructed/initialized
echo "=== Search for where opts.sentry is created ==="
rg -n "opts\.sentry\s*=" --type ts -B 3 -A 3 controlplane/src/core/build-server.ts | head -40
echo -e "\n=== Search for opts object initialization ==="
rg -n "const opts|opts\s*:" --type ts controlplane/src/core/build-server.ts | head -20Length of output: 155
🏁 Script executed:
#!/bin/bash
# Check the full opts.sentry structure to see if sendDefaultPii is available
echo "=== Look for sentry configuration in opts object ==="
sed -n '1,256p' controlplane/src/core/build-server.ts | grep -n "sentry" -A 2 -B 2
echo -e "\n=== Check if opts includes sendDefaultPii anywhere ==="
rg -n "sendDefaultPii" --type ts controlplane/src/core/Length of output: 483
🏁 Script executed:
#!/bin/bash
# Understand the type of opts parameter
echo "=== Search for opts parameter type definition ==="
rg -n "function buildServer|async function.*opts|opts.*:" --type ts controlplane/src/core/build-server.ts | head -5
echo -e "\n=== Look at function signature of buildServer ==="
sed -n '1,50p' controlplane/src/core/build-server.tsLength of output: 3259
🏁 Script executed:
#!/bin/bash
# Find BuildConfig type definition
echo "=== Search for BuildConfig type definition ==="
rg -n "type BuildConfig|interface BuildConfig" --type ts
echo -e "\n=== Get full BuildConfig definition ==="
rg -nA 50 "type BuildConfig|interface BuildConfig" --type tsLength of output: 3447
🏁 Script executed:
#!/bin/bash
# Verify sendDefaultPii is not passed to enrichment context
echo "=== Check how Authentication class receives the enrichment callback ==="
rg -n "class Authentication|constructor.*enrichment|enrichmentContext" --type ts -A 5 controlplane/src/services/Authentication.ts | head -40Length of output: 155
🏁 Script executed:
#!/bin/bash
# Find Authentication class file
echo "=== Search for Authentication class ==="
find controlplane/src -name "*.ts" | xargs rg -l "class Authentication" 2>/dev/null
echo -e "\n=== Alternative: search by pattern ==="
rg -n "export.*class Authentication" --type ts controlplane/src/Length of output: 373
🏁 Script executed:
#!/bin/bash
# Confirm opts.sentry structure doesn't include sendDefaultPii
echo "=== Full BuildConfig.sentry structure ==="
sed -n '140,145p' controlplane/src/core/build-server.tsLength of output: 101
Enrichment hook bypasses Sentry's sendDefaultPii security setting; PII is sent unconditionally
The code is defensively implemented (try-catch, gated on opts.sentry.enabled && opts.sentry.dsn), and field guards are correct for optional fields. However, verification reveals a PII handling inconsistency:
- Global Sentry config (sentry.config.ts) sets
sendDefaultPii: SENTRY_SEND_DEFAULT_PII, which defaults tofalse - BuildConfig.sentry only exposes
{ enabled: boolean; dsn?: string }— thesendDefaultPiiflag is not accessible to the enrichment callback - The hook sends userId, userDisplayName, and organizationId to Sentry without checking the
sendDefaultPiisetting - Result: user identifiers and org data are sent to Sentry regardless of the global privacy configuration
The enrichment hook should either:
- Have access to and respect the
sendDefaultPiiflag before calling Sentry.setUser/setTag, or - Document why this data is acceptable to send unconditionally, or
- Avoid sending user identifiers entirely if PII minimization is intended
🤖 Prompt for AI Agents
In controlplane/src/core/build-server.ts around lines 257 to 287, the enrichment
callback unconditionally calls Sentry.setUser and Sentry.setTag with user/org
identifiers which bypasses the global sendDefaultPii privacy setting; change the
code so the callback respects that flag by obtaining sendDefaultPii from the
sentry config or BuildConfig (extend BuildConfig.sentry to include
sendDefaultPii if needed and propagate it to opts), then wrap Sentry.setUser and
any tags that contain PII (userId, userDisplayName, organizationId, apiKeyName)
in a conditional that only runs when sendDefaultPii is true; non-PII tags such
as auth.kind may remain unconditional or be explicitly filtered, and update
affected types/interfaces and tests to reflect the added sentry flag.
Summary by CodeRabbit
New Features
Chores
Checklist