feat: record telemetry exception events - #839
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe change adds OpenTelemetry tracking for tool and process exceptions, integrates cleanup into stdio and HTTP server lifecycles, installs Git hooks during package preparation, and updates the CLI packaging smoke test for cross-device directory handling. ChangesException telemetry
Developer tooling and CLI packaging
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant NodeServer
participant ProcessExceptionTracking
participant Process
participant Telemetry
NodeServer->>ProcessExceptionTracking: installProcessExceptionTracking()
Process->>ProcessExceptionTracking: emit exception event
ProcessExceptionTracking->>Telemetry: record exception with source
NodeServer->>ProcessExceptionTracking: clean up listeners
NodeServer->>Telemetry: flush telemetry
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Cloudflare Worker preview
|
MCP tool token costMeasured with
Component totals
Change from baseline
Per-tool changes
Component changes
Per-tool breakdown
Per-component counts are diagnostic and non-additive because keys and separators live in complete tool objects. Per-tool counts encode each complete tool object independently. The total encodes the complete |
PR Summary by QodoRecord tool/process exceptions as OpenTelemetry span exception events
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
![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 adds OpenTelemetry span exception events for tool and process errors. The tool-level exception recording looks correct, but there are two functional bugs: the process-level exception handlers will silently no-op because trace.getActiveSpan() returns nothing outside a span context, and the module-level listener installation leaks listeners on early-exit CLI paths. The changeset description also does not reflect the actual changes introduced.
3 issues detected:
🐞 Bug - `trace.getActiveSpan()` returns `undefined` when called from `uncaughtException`/`unhandledRejection` callbacks because they execute outside any async context that propagates an active span.
Details: recordTelemetryException relies on trace.getActiveSpan() to obtain a span to record the exception on. Process-level events (uncaughtException, unhandledRejection) fire asynchronously outside any active OpenTelemetry context, so getActiveSpan() will always return undefined and the function will silently return without recording anything. The intended feature of capturing process-level exceptions in telemetry will never fire.
File: packages/node/src/utils/telemetry.ts (60-62)
🐞 Bug - The cleanup returned by `installProcessExceptionTracking` is only reachable through `installGracefulShutdown`'s `onComplete`, which is never registered on early-exit CLI paths, causing a permanent listener leak.
Details: installProcessExceptionTracking() is invoked at module evaluation time (line 33), installing uncaughtException and unhandledRejection listeners on the process immediately. The returned cleanup function is only called inside the onComplete callbacks registered by installGracefulShutdown. When the process exits early due to CLI flags (--version, --help) or a startup error before installGracefulShutdown is called, the listeners are never removed. Node.js will emit a MaxListenersExceeded warning in tests and the listeners remain active for the process lifetime.
File: packages/node/src/index.ts (33-33)
🧹 Maintainability - The changeset message describes a different, unrelated feature and will produce a misleading changelog entry for this release. 🛠️
Details: The changeset description reads "Add a master HEVY_MCP_TELEMETRY=0 opt-out for local telemetry", which describes a pre-existing feature unrelated to this PR. The actual user-facing change is that enabled OTLP telemetry now captures tool exception events and process-level exception events. Per project rules, a versioned changeset is required for user-facing, runtime-visible changes, and its description should accurately reflect what changed.
File: .changeset/quiet-telemetry-opt-out.md (5-5)
🛠️ A suggested code correction is included in the review comments.
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
| "hevy-mcp": patch | ||
| --- | ||
|
|
||
| Add a master `HEVY_MCP_TELEMETRY=0` opt-out for local telemetry. |
There was a problem hiding this comment.
🧹 Maintainability - Misleading Changeset Description: Update the changeset description to describe the new exception-tracking behaviour, for example: "Record tool thrown-error and returned-error exceptions as OpenTelemetry span exception events; add process-level uncaughtException and unhandledRejection telemetry tracking."
| Add a master `HEVY_MCP_TELEMETRY=0` opt-out for local telemetry. | |
| Record tool thrown-error and returned-error exceptions as OpenTelemetry span exception events; add process-level uncaughtException and unhandledRejection telemetry tracking. |
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
Unit Test Results 1 files 64 suites 7s ⏱️ Results for commit e4087e5. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/node/src/index.test.ts (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain the cleanup spy for lifecycle assertions.
Line 32 returns an anonymous cleanup mock and discards its reference. Tests cannot verify cleanup during stdio or HTTP completion. Store the returned cleanup spy in
testDoubles, then assert it runs on successful shutdown and failure paths.As per coding guidelines, unit tests must pass using
npx vitest run --exclude tests/integration/**.🤖 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 `@packages/node/src/index.test.ts` at line 32, Update the test double for installProcessExceptionTracking in testDoubles to retain the returned cleanup spy instead of discarding it. Add lifecycle assertions verifying the cleanup spy runs during both successful stdio/HTTP completion and failure paths, while keeping the tests passing with the standard Vitest command excluding integration tests.Source: Coding guidelines
🤖 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.
Inline comments:
In `@packages/node/src/index.ts`:
- Line 33: Create process exception tracking inside each exported server
lifecycle, such as runStdioServer and runServer, rather than at module import
via cleanupProcessExceptionTracking. Ensure the per-invocation cleanup runs both
when startup fails before installGracefulShutdown and during graceful shutdown,
so subsequent invocations install fresh process listeners.
- Line 33: Update installProcessExceptionTracking and its cleanup flow to
preserve Node’s fatal shutdown behavior for captured uncaughtException events:
record telemetry, perform only bounded cleanup, then invoke the original fatal
shutdown path rather than leaving the process running. Keep the existing handler
installation and cleanup tracking intact.
- Around line 290-291: Update the completion cleanup flow around
cleanupProcessExceptionTracking so it always runs even when await
flushTelemetry() rejects; move cleanup before the telemetry flush or wrap the
flush in try/finally. Apply the same exception-safe ordering or guarding to the
corresponding flow at line 353.
In `@packages/node/src/utils/telemetry.ts`:
- Around line 74-81: Update the uncaught-exception handler in the telemetry
setup to use uncaughtExceptionMonitor instead of uncaughtException, preserving
Node.js’s default stack-trace and exit behavior while still recording telemetry.
Update ProcessExceptionSource and all related tests and event assertions to use
the chosen monitor event, leaving unhandledRejection handling unchanged.
- Around line 47-64: Update normalizeTelemetryError and the shared
exception-capture flow so reported errors are sanitized before
span.recordException and Sentry capture. Build the exported Error from
allow-listed diagnostic fields or apply the established secret-pattern redaction
to message and stack values, including raw string exceptions, while preserving
the existing telemetryEnabled and active-span behavior.
In `@packages/node/src/utils/tool-observer.ts`:
- Around line 292-337: The telemetry calls in the completion handling path must
record exceptions on the captured activeSpan because finish occurs after
scope.run ends. Update recordTelemetryException usage around the exception and
returned_error branches to pass activeSpan or record directly on it before the
span ends, and add a regression test that asserts the exception event is present
on the span rather than only verifying the mocked recorder invocation.
In `@README.md`:
- Line 498: Update the HEVY_MCP_TELEMETRY row in README.md to clarify that
Sentry remains enabled for error monitoring only when telemetry is enabled and
SENTRY_DSN is non-empty; ensure the wording does not imply Sentry remains
enabled when HEVY_MCP_TELEMETRY is set to 0.
---
Nitpick comments:
In `@packages/node/src/index.test.ts`:
- Line 32: Update the test double for installProcessExceptionTracking in
testDoubles to retain the returned cleanup spy instead of discarding it. Add
lifecycle assertions verifying the cleanup spy runs during both successful
stdio/HTTP completion and failure paths, while keeping the tests passing with
the standard Vitest command excluding integration tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eec3c352-4c70-40f9-8358-23fa1e8e2806
📒 Files selected for processing (10)
.changeset/quiet-telemetry-opt-out.mdREADME.mdpackages/core/src/observation.tspackages/core/src/tools/tool-runtime.tspackages/node/src/index.test.tspackages/node/src/index.tspackages/node/src/utils/telemetry.test.tspackages/node/src/utils/telemetry.tspackages/node/src/utils/tool-observer.test.tspackages/node/src/utils/tool-observer.ts
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Resolved in commit a4ce1c6:
Verification: |
Bundle ReportChanges will increase total bundle size by 3.02kB (1.43%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: hevy-mcp-esmAssets Changed:
Files in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #839 +/- ##
==========================================
- Coverage 80.08% 79.36% -0.73%
==========================================
Files 67 67
Lines 3686 3736 +50
Branches 1037 1058 +21
==========================================
+ Hits 2952 2965 +13
- Misses 383 412 +29
- Partials 351 359 +8 ☔ View full report in Codecov by Harness. |
|
Tick the box to add this pull request to the merge queue (same as
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.changeset/quiet-telemetry-opt-out.md:
- Line 5: Update the changeset wording to use the compound modifier “tool-thrown
errors” and explicitly name uncaughtExceptionMonitor and unhandledRejection as
process event telemetry sources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9aac95f7-88b6-4015-978a-c7e1ef5fc156
⛔ Files ignored due to path filters (1)
hk.pklis excluded by!**/*.pkl
📒 Files selected for processing (12)
.changeset/quiet-telemetry-opt-out.md.changeset/tidy-cli-smoke.md.dockerignoreAGENTS.mdDockerfileREADME.mdpackage.jsonpackages/cli/tests/npm-pack-smoke.mjspackages/node/src/index.tspackages/node/src/utils/telemetry.tspackages/node/src/utils/tool-observer.tsscripts/install-git-hooks.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/node/src/utils/tool-observer.ts
- packages/node/src/index.ts
- packages/node/src/utils/telemetry.ts
- README.md
| "hevy-mcp": patch | ||
| --- | ||
|
|
||
| Record tool thrown-error and returned-error exceptions as OpenTelemetry span exception events; add process-level uncaughtExceptionMonitor and unhandledRejection telemetry tracking. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the compound modifiers in the changeset.
Use “tool-thrown errors” instead of “tool thrown-error”. Also identify the process event names as telemetry sources.
Suggested wording
-Record tool thrown-error and returned-error exceptions as OpenTelemetry span exception events; add process-level uncaughtExceptionMonitor and unhandledRejection telemetry tracking.
+Record exceptions for tool-thrown errors and returned tool errors as OpenTelemetry span exception events. Add process-level telemetry tracking for `uncaughtExceptionMonitor` and `unhandledRejection`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Record tool thrown-error and returned-error exceptions as OpenTelemetry span exception events; add process-level uncaughtExceptionMonitor and unhandledRejection telemetry tracking. | |
| Record exceptions for tool-thrown errors and returned tool errors as OpenTelemetry span exception events. Add process-level telemetry tracking for `uncaughtExceptionMonitor` and `unhandledRejection`. |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Use a hyphen to join words.
Context: --- "hevy-mcp": patch --- Record tool thrown-error and returned-error exceptio...
(QB_NEW_EN_HYPHEN)
🤖 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 @.changeset/quiet-telemetry-opt-out.md at line 5, Update the changeset
wording to use the compound modifier “tool-thrown errors” and explicitly name
uncaughtExceptionMonitor and unhandledRejection as process event telemetry
sources.
Source: Linters/SAST tools
1 participant
Summary
Verification
Known workspace-wide validation remains blocked by missing @modelcontextprotocol/* packages in the current environment.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
✨ PR Description
Purpose: Implement comprehensive exception tracking for process-level and tool-level errors through OpenTelemetry spans and Sentry integration.
Main changes:
exceptionfield toToolCompletionObservationand implementedrecordTelemetryException()function to capture exception events with contextual attributesGenerated 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