fix: propagate user hash to startup spans - #683
Conversation
Code Review by Qodo
1. Global user hash leakage
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTelemetry identity handling changes from user IDs to deterministic API-key hashes. The hash is propagated through OpenTelemetry span processors, request instrumentation, server startup spans, and Sentry context, with tests and a patch Changesets entry updated. An unrelated server-manifest integration test is removed. ChangesUser hash telemetry
Server manifest test cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant runServer
participant Telemetry
participant UserHashSpanProcessor
participant OpenTelemetry
participant Sentry
runServer->>Telemetry: fingerprint API key
runServer->>Telemetry: setCurrentUserHash
runServer->>Sentry: setUser with hashed identifier
runServer->>OpenTelemetry: start mcp.server.run span
UserHashSpanProcessor->>OpenTelemetry: attach user.hash to started spans
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
PR Summary by QodoPropagate
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
MCP tool token costMeasured with
Change from baseline
Per-tool changes
Per-tool breakdown
Per-tool counts encode each complete tool object independently. The total encodes the complete |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #683 +/- ##
==========================================
+ Coverage 95.33% 95.43% +0.10%
==========================================
Files 47 47
Lines 2080 2083 +3
Branches 570 571 +1
==========================================
+ Hits 1983 1988 +5
+ Misses 34 32 -2
Partials 63 63 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Unit Test Results 1 files 49 suites 4s ⏱️ Results for commit 0e14abf. ♻️ This comment has been updated with latest results. |
![gitstream-cm[bot]](https://avatars.githubusercontent.com/in/230441?s=60&v=4){kind=small}
There was a problem hiding this comment.
✨ PR Review
The PR cleanly migrates from user.id to user.hash, introduces a UserHashSpanProcessor to propagate the hash to every span automatically, and seeds the user context before startup validation. The logic is sound and well-tested. A few minor issues are noted below.
3 issues detected:
🐞 Bug - `getCurrentUserHash()` is invoked twice — once to guard the conditional and once to populate the value — instead of being stored in a variable. 🛠️
Details: getCurrentUserHash() is called twice in the ternary — once for the truthiness check and once to read the value. Although currentUserHash is module-level synchronous state and is unlikely to change between the two calls within a single event-loop tick, the pattern is fragile and produces an unnecessary second function call.
File: src/utils/hevy-client-observability.ts (24-26)
🛠️ A suggested code correction is included in the review comments.
🧹 Maintainability - The branch on lines 251-254 that updates the span attribute and re-sets the user hash can never execute because `initialUserHash` and `userHash` are always derived from the same env var value.
Details: The userHash !== initialUserHash guard (lines 251-254) can never be true in practice. Both initialUserHash and the hash computed inside the span derive from process.env.HEVY_API_KEY. configuredApiKey reads the env var directly; parseConfig returns env.HEVY_API_KEY || "", and assertApiKey would have called process.exit(1) on an empty/missing key before reaching line 250. The two hashes will therefore always be equal when both are defined, making the conditional update branch dead code.
File: src/index.ts (250-254)
🧹 Maintainability - `user.hash` is set both by the `UserHashSpanProcessor` on every span and by manual `attributes` spreads in multiple call sites, creating duplicated and potentially diverging logic.
Details: UserHashSpanProcessor.onStart already stamps user.hash on every span automatically. The same attribute is then also explicitly set in buildServer, runServer, and withTelemetry span attribute objects. The explicit sets are redundant, making the UserHashSpanProcessor the only necessary mechanism. Keeping both paths risks the two getting out of sync if the attribute key is ever renamed or the processor is removed.
File: src/utils/telemetry.ts (82-94)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| ...(getCurrentUserHash() | ||
| ? { "user.hash": getCurrentUserHash() } | ||
| : {}), |
There was a problem hiding this comment.
🐞 Bug - Double State Read: Capture the return value in a local variable before the spread:
const userHash = getCurrentUserHash();
...(userHash ? { "user.hash": userHash } : {}),| ...(getCurrentUserHash() | |
| ? { "user.hash": getCurrentUserHash() } | |
| : {}), | |
| ...((() => { const userHash = getCurrentUserHash(); return userHash ? { "user.hash": userHash } : {}; })()), |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
|
Tick the box to add this pull request to the merge queue (same as
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the telemetry implementation from using user.id to the OpenTelemetry user.hash semantic convention, ensuring the user hash is propagated to every recorded span. It introduces a UserHashSpanProcessor to automatically append the user hash to spans and updates the corresponding tests and utility files. The feedback suggests also seeding and updating the Sentry user context alongside the OpenTelemetry user hash during server startup and configuration parsing to ensure consistent user tracking across both platforms.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (initialUserHash) { | ||
| setCurrentUserHash(initialUserHash); | ||
| } |
There was a problem hiding this comment.
To ensure that Sentry error events captured during startup validation failures (e.g., when validateApiKey throws an error) are also associated with the correct user context, we should seed the Sentry user ID alongside the OpenTelemetry user hash.
| if (initialUserHash) { | |
| setCurrentUserHash(initialUserHash); | |
| } | |
| if (initialUserHash) { | |
| setCurrentUserHash(initialUserHash); | |
| Sentry.setUser({ id: initialUserHash }); | |
| } |
| if (userHash !== initialUserHash) { | ||
| setCurrentUserHash(userHash); | ||
| span.setAttribute("user.hash", userHash); | ||
| } |
There was a problem hiding this comment.
Similarly, if the parsed API key differs from the initial environment API key, we should update the Sentry user context to match the new user hash.
| if (userHash !== initialUserHash) { | |
| setCurrentUserHash(userHash); | |
| span.setAttribute("user.hash", userHash); | |
| } | |
| if (userHash !== initialUserHash) { | |
| setCurrentUserHash(userHash); | |
| Sentry.setUser({ id: userHash }); | |
| span.setAttribute("user.hash", userHash); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/telemetry.ts (1)
82-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplicit
user.hashspan attributes are redundant.Because
UserHashSpanProcessorautomatically injects theuser.hashattribute into every started span via itsonStarthook, explicitly passinguser.hashin theattributesobject when starting spans is architecturally redundant.Consider removing the manual attribute definitions to reduce duplication:
src/utils/telemetry.ts#L82-L94: this processor is the root mechanism that applies the hash globally.src/utils/hevy-client-observability.ts#L24-L25: optionally remove this manual attribute injection.src/utils/telemetry-wrapper.ts#L130-L130: optionally remove this manual attribute injection.src/index.ts#L174-L174: optionally remove this manual attribute injection.src/index.ts#L242-L242: optionally remove this manual attribute injection.Note: If you choose to remove these explicit attributes, you will also need to update the corresponding unit tests (e.g., in
hevy-client-observability.test.ts,telemetry-wrapper.test.ts, andindex.test.ts). The tests currently mock the tracer and assert thatuser.hashis explicitly passed into theattributesargument, which won't be true once it's solely handled by the processor's post-creation lifecycle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/telemetry.ts` around lines 82 - 94, The UserHashSpanProcessor already applies user.hash globally, so remove the redundant explicit user.hash attributes from src/utils/hevy-client-observability.ts:24-25, src/utils/telemetry-wrapper.ts:130, and src/index.ts:174 and 242; update the corresponding tests to stop asserting those attributes are passed directly, while leaving UserHashSpanProcessor in src/utils/telemetry.ts:82-94 as the sole injection mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/telemetry.ts`:
- Around line 82-94: The UserHashSpanProcessor already applies user.hash
globally, so remove the redundant explicit user.hash attributes from
src/utils/hevy-client-observability.ts:24-25,
src/utils/telemetry-wrapper.ts:130, and src/index.ts:174 and 242; update the
corresponding tests to stop asserting those attributes are passed directly,
while leaving UserHashSpanProcessor in src/utils/telemetry.ts:82-94 as the sole
injection mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cf21131-d569-4d0e-ae82-5c1bc2b27546
📒 Files selected for processing (10)
.changeset/user-hash-spans.mdsrc/index.test.tssrc/index.tssrc/utils/hevy-client-observability.test.tssrc/utils/hevy-client-observability.tssrc/utils/stdio-observability.test.tssrc/utils/telemetry-wrapper.test.tssrc/utils/telemetry-wrapper.tssrc/utils/telemetry.test.tssrc/utils/telemetry.ts
Code Review by Qodo
Context used✅ Compliance rules (platform):
65 rules 1. Global user hash leakage
|
There was a problem hiding this comment.
✨ PR Review
The PR cleanly renames userId → userHash throughout the span-attribute pipeline, adds a UserHashSpanProcessor to stamp every span automatically, and seeds the hash before startup config validation. The implementation is largely correct, but the two still-open previous issues (double getCurrentUserHash() call and redundant explicit user.hash sets) remain; issue_2 (dead-code guard) appears to have been dropped in the new code and is now resolved.
2 issues detected:
🐞 Bug - `getCurrentUserHash()` is called twice — once for the truthiness check and once for the value — when a single call into a local variable would suffice. 🛠️
Details: getCurrentUserHash() is invoked twice in the ternary: once for the truthiness check and once to read the value. Although the module-level variable is synchronous, calling the getter twice is fragile and unnecessary.
File: src/utils/hevy-client-observability.ts (24-26)
🛠️ A suggested code correction is included in the review comments.
🧹 Maintainability - `user.hash` is set both by the span processor and by explicit span attribute objects, duplicating the mechanism and introducing a future divergence risk.
Details: UserHashSpanProcessor.onStart already stamps user.hash on every span automatically (including mcp.server.build, mcp.server.run, and mcp.tool.*). The same attribute is also explicitly set in buildServer, runServer, and withTelemetry. This dual-write path means the attribute key exists in two places and the two can drift out of sync if the processor is ever removed or the key is renamed.
File: src/utils/telemetry.ts (82-94)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| ...(getCurrentUserHash() | ||
| ? { "user.hash": getCurrentUserHash() } | ||
| : {}), |
There was a problem hiding this comment.
🐞 Bug - Double Hash Lookup: Capture the result in a local variable and use it in both branches:
const userHash = getCurrentUserHash();
...(userHash ? { "user.hash": userHash } : {}),| ...(getCurrentUserHash() | |
| ? { "user.hash": getCurrentUserHash() } | |
| : {}), | |
| ...((() => { const userHash = getCurrentUserHash(); return userHash ? { "user.hash": userHash } : {}; })()), |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
1 participant
Summary
user.hashbefore startup configuration validationVerification
npx vitest run src/index.test.ts src/utils/telemetry.test.ts src/utils/telemetry-wrapper.test.ts src/utils/hevy-client-observability.test.tsnpm run buildnpm run check:typesnpm run checknpm run test:unitnpm run check:changesetSummary by CodeRabbit
user.hashacross OpenTelemetry spans (including server build/run and request/workflow spans).user.hashbehavior.hevy-mcp.✨ PR Description
Purpose: Rename user identification from
user.idtouser.hashsemantic convention and propagate it to all spans via a custom span processor for consistent trace correlation.Main changes:
setCurrentUserId/getCurrentUserIdwithsetCurrentUserHash/getCurrentUserHashthroughout codebaseUserHashSpanProcessorto automatically injectuser.hashattribute into every started spanrunServer()before config validation for startup failure trace correlationGenerated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how