feat(sentry): segment spans and errors by anon/authed state - #925
feat(sentry): segment spans and errors by anon/authed state#925thostetler wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #925 +/- ##
========================================
+ Coverage 67.8% 68.0% +0.2%
========================================
Files 378 380 +2
Lines 44164 44192 +28
Branches 2377 2384 +7
========================================
+ Hits 29937 30030 +93
+ Misses 14179 14113 -66
- Partials 48 49 +1
🚀 New features to boost your workflow:
|
Sentry had no user data at all. providers.tsx called Sentry.setUser with only an anonymous flag, and Sentry drops a user object carrying no indexed key, so the call never did anything — there was no way to tell whether an error or a slow pageload hit a logged-in researcher or an anonymous visitor. - Add an auth tag, values anon and authed, set from a client-readable scix_auth cookie at SDK init so the pageload span carries it - Tag the per-request isolation scope in updateUserStateSSR and in middleware so http.server and edge spans are segmented too - Replace the dead Sentry.setUser call with a setTag that corrects the value after a client-side login or logout - Read SENTRY_ENVIRONMENT in the server config
The client-side tag used `anonymous === false`, which disagrees with isAuthenticated() in two ways: a session with anonymous true and a username other than anonymous@ads is authenticated, and an unhydrated store has anonymous undefined. Both cases resolved to anon and overwrote the correct tag that middleware had already set from the session. - Drop authTagForUser and call isAuthenticated from the Telemetry effect, keeping auth-utils off the Sentry init path - Skip the effect entirely while the store user is unhydrated, so unknown never overwrites the cookie value - Export SENTRY_AUTH_TAG_NAME instead of repeating the tag key four times - Match the session cookie's sameSite strict on scix_auth - Cover the home-page early-return branch in the edge span tag test
9cad54e to
ff8ebc8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There’s a blocker-level issue in the new Sentry config tests due to resetModules() ordering that can prevent observing Sentry.init calls reliably.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Risk summary: Medium risk. Changes touch request-time middleware + SSR utilities + client telemetry initialization; incorrect tagging or a middleware regression could impact observability across most navigations.
This PR adds a low-cardinality Sentry tag (auth=authed|anon) to segment spans/events by anonymous vs authenticated state across edge middleware, SSR, and the client SDK init path, and wires SENTRY_ENVIRONMENT into the Node Sentry config.
Changes:
- Add
scix_authcookie + edge/SSR tagging to keep Sentryauthtag in sync with session state. - Tag initial client Sentry scope from cookie at SDK init, and update tag on client-side auth state changes.
- Add
SENTRY_ENVIRONMENTsupport for Node runtime and extend tests around Sentry config + middleware behavior.
Findings (priority order):
-
blocker —
sentry.config.test.tsmodule reset ordering can break the test’s ability to observeSentry.initcalls- Impact: Config tests may become flaky or consistently fail because the config imports can call a different mocked
initthan the one inspected. - Location:
sentry.config.test.ts:13-21 - Minimal fix: Call
vi.resetModules()before importing@sentry/nextjs, then import the config; optionally guard against missing calls. - Confidence: high
- Impact: Config tests may become flaky or consistently fail because the config imports can call a different mocked
-
medium — SSR tagging passes an optional session flag without boolean coercion
- Impact: If
session.isAuthenticatedis ever unset/undefined(e.g., unexpected session initialization gaps), tagging becomes implicit/ambiguous and can skew segmentation. - Location:
src/ssr-utils.ts:26 - Minimal fix: Coerce to a strict boolean (e.g.,
ctx.req.session.isAuthenticated === true) before passing toauthTagForSession. - Confidence: medium
- Impact: If
File summaries
| File | Description |
|---|---|
| src/ssr-utils.ts | Tags SSR request scope with auth based on session state. |
| src/providers.tsx | Updates Sentry auth tag after client-side auth changes once store is hydrated. |
| src/middleware.ts | Writes scix_auth cookie and tags edge isolation scope for all middleware-handled requests. |
| src/middlewares/tests/middleware.routes.integration.test.ts | Adds integration coverage for cookie emission and edge tagging without clobbering session cookies. |
| src/lib/sentryAuthTag.ts | Centralizes auth tag name, cookie name, and mapping helpers. |
| src/lib/sentryAuthTag.test.ts | Unit tests for tag mapping and cookie parsing behavior. |
| src/tests/ssr-utils.test.ts | Adds SSR test coverage verifying the Sentry tag is set per session state. |
| sentry.client.config.ts | Tags initial client scope from scix_auth cookie at SDK init. |
| sentry.server.config.ts | Reads SENTRY_ENVIRONMENT for Node runtime Sentry environment. |
| sentry.config.test.ts | New tests validating server environment and client cookie-based tagging. |
| .env.local.sample | Documents SENTRY_ENVIRONMENT env var. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const loadInitOptions = async (configPath: string): Promise<Record<string, unknown>> => { | ||
| const Sentry = await import('@sentry/nextjs'); | ||
| vi.mocked(Sentry.init).mockClear(); | ||
| vi.resetModules(); | ||
| await import(configPath); | ||
|
|
||
| const [options] = vi.mocked(Sentry.init).mock.calls[0]; | ||
| return { ...options }; | ||
| }; |
Next's ResponseCookies snapshots the set-cookie header when the response is constructed and rewrites the whole header from that snapshot on every set, while iron-session appends its session cookie straight to the headers. Writing scix_auth through response.cookies.set() after initSession therefore erased scix_session, so middleware stopped seeing anyone as authenticated — eight middleware e2e tests failed on the missing cookie and the redirects that followed from it. - Append the scix_auth set-cookie header directly instead - Add a regression test that appends a session cookie the way iron-session does, confirmed to fail against the previous implementation - Read raw set-cookie headers in the auth cookie tests, since res.cookies only reflects cookies written through the ResponseCookies API - Treat an undefined session isAuthenticated as anon in authTagForSession, since the iron-session shim types it optional - Reset modules before importing the Sentry mock in the config tests, so the assertions cannot read a stale mock instance
ff8ebc8 to
6476d2b
Compare
Sentry had no user data. The old setUser call carried only an anonymous flag, which Sentry drops for lacking an indexed key, so there was no way to tell whether an error or slow pageload hit a logged-in researcher or an anonymous visitor.