feat: make OTLP metrics multi-process safe - #848
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds structured telemetry across cache, Hevy API, MCP tools, sessions, and server lifecycle paths. It adds privacy-safe metrics, stable service-instance identity, delta temporality, cache and request observations, structured failures, tests, ClickStack queries, and Changesets metadata. ChangesTelemetry observability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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 |
![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 rich observability instrumentation (cache observation, request lifecycle scopes, retry wait spans, SDK error tracking) and multi-process telemetry safety. The core logic is well-structured with best-effort error isolation throughout. Three issues worth addressing before merge.
3 issues detected:
🐞 Bug - A connect-phase error is recorded on both the inner connect span and the outer run span, producing duplicate telemetry events. 🛠️
Details: When server.connect(transport) throws, recordLifecycleFailure is called twice for the same error: once inside the inner mcp.server.connect span (line 532) with phase "connect", and again in the outer mcp.server.run catch block (line 557) with phase "connect" (because connectAttempted is true). This emits a duplicate mcp.lifecycle.failure event and calls recordTelemetryException twice on two different spans for the same underlying error.
File: packages/node/src/index.ts (557-557)
🛠️ A suggested code correction is included in the review comments.
🧹 Maintainability - Patching a private `_requestHandlers` map bypasses the SDK's public API and will silently stop working if the SDK changes its internal structure.
Details: installSdkErrorTracking accesses (protocol as unknown as SdkProtocolInternals)._requestHandlers — a private, underscore-prefixed internal field of the MCP TypeScript SDK — to intercept and wrap the tools/call and server/discover handlers. This is the same category of SDK-internal dependency that the user instructions already call out as requiring special care ("rerun the stdio observability test suite because it depends on SDK stdio internals"). If the SDK renames, removes, or restructures _requestHandlers, this code will silently stop instrumenting tool calls without any compile-time or runtime error, making the failure invisible until tracing gaps are noticed in production.
File: packages/node/src/index.ts (299-342)
🎯 Scope - New public exports in published packages are treated as internal-only changes, bypassing semver signalling for downstream consumers.
Details: All three changesets (multi-process-metrics.md, opaque-telemetry-correlation.md, steady-failure-events.md) contain only the front-matter delimiters ---\n--- with no package bumps. The PR exports new public types from @hevy-mcp/core (CacheObserver, CacheObservationScope, McpSpanCategory, McpToolFailureEvent, createMcpToolFailureEvent, etc.) and from @hevy-mcp/hevy-client (HevyApiOutcome, HevyRequestObservationScope, HevyRequestStart, HevyRetryWait, HevyRetryWaitScope). Per project conventions, user-facing runtime-visible changes to published packages require versioned changesets, not empty ones.
File: .changeset/multi-process-metrics.md (1-2)
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
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
PR Summary by QodoMake OTLP metrics multi-process safe and improve trace/error fidelity
AI Description
Diagram
Files changed (28)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/node/src/index.ts (1)
496-569: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the failure-phase label for post-connect errors in
runStdioServer.
connectAttemptedis set totrueright after the transport is created, beforeserver.connecteven runs, and it staystrueafterward. IfscheduleUpdateCheckorinstallGracefulShutdownthrows after a successfulserver.connect, the outer catch still recordsmcp.failure.phase: "connect". This mislabels a post-connect setup failure as a connection failure.Track connect success separately from "connect attempted" so the phase label reflects where the failure actually happened.
🐛 Proposed fix to track connect success separately
let connectAttempted = false; + let connectSucceeded = false; await tracer.startActiveSpan( "mcp.server.run", { attributes: { "mcp.span.category": "startup", "mcp.transport": "stdio", }, }, async (span) => { try { const cfg = parseConfig(process.env); const apiKey = cfg.apiKey; assertApiKey(apiKey); const server = await createNodeMcpServer({ apiKey }); console.error("Starting MCP server in stdio mode"); const transport = createInstrumentedStdioTransport( new StdioServerTransport(), ); connectAttempted = true; await tracer.startActiveSpan( "mcp.server.connect", { attributes: { "mcp.span.category": "session", "mcp.transport": "stdio", }, }, async (connectSpan) => { try { await server.connect(transport); + connectSucceeded = true; connectSpan.setStatus({ code: SpanStatusCode.OK }); } catch (e) { recordLifecycleFailure(connectSpan, e, "connect"); connectSpan.setStatus({ code: SpanStatusCode.ERROR }); throw e; } finally { connectSpan.end(); } }, ); ... span.setStatus({ code: SpanStatusCode.OK }); } catch (e) { - recordLifecycleFailure(span, e, connectAttempted ? "connect" : "run"); + recordLifecycleFailure( + span, + e, + connectSucceeded ? "run" : connectAttempted ? "connect" : "run", + ); recordMcpSessionTermination( connectAttempted ? "connect_failure" : "startup_failure", );🤖 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.ts` around lines 496 - 569, Update runStdioServer to track connection success separately from connectAttempted: set the new success indicator only after server.connect completes successfully, then use it in the outer catch to label failures as "connect" only when connection failed and "run" for post-connect setup errors. Preserve connectAttempted for distinguishing startup failures from connection failures in session termination.
🧹 Nitpick comments (4)
packages/hevy-client/src/hevy-client-kubb.ts (2)
487-529: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
error.categoryis always"HevyHttpError", so"NetworkError"is never emitted.Line 524 sets the category to a constant. Network failures reach this branch as a synthesized
HevyHttpErrorwith nostatusand a network code such asECONNRESET. The"NetworkError"member of theHevyRequestObservation["error"]["category"]union at Line 73 is therefore dead, and consumers cannot separate transport failures from HTTP failures by category.Derive the category from whether the original cause was an HTTP response.
♻️ Proposed change
const error = isHevyHttpError(cause) ? cause : new HevyHttpError( normalized.signal?.aborted ? "Hevy API request was canceled" : "Hevy API network request failed", { method, endpoint, code: normalized.signal?.aborted ? HEVY_REQUEST_ABORTED_ERROR_CODE : getNetworkCode(cause), cause, }, ); + const errorCategory = + error.status === undefined ? "NetworkError" : "HevyHttpError"; @@ error: { status: error.status, code: typeof error.code === "string" && SAFE_OBSERVATION_CODES.has(error.code) ? error.code : undefined, - category: "HevyHttpError", + category: errorCategory, },🤖 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/hevy-client/src/hevy-client-kubb.ts` around lines 487 - 529, Update the observation construction in the request error-handling flow to derive error.category from whether the original error includes an HTTP response: use "HevyHttpError" for response-backed failures and "NetworkError" for transport failures such as ECONNRESET without a status. Keep the existing status, code, and retry behavior unchanged.
138-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe safe error-code allowlist is defined twice. Both packages maintain their own copy of the same eleven-entry set. The producer already filters
observation.error.codethrough its copy, so the consumer copy is redundant defense whose only practical effect is drift: if one list gains a code and the other does not, that code is silently dropped from telemetry. The node copy also restates"HEVY_REQUEST_ABORTED"and"HEVY_RETRY_EXHAUSTED"as string literals, even though@hevy-mcp/hevy-clientexportsHEVY_REQUEST_ABORTED_ERROR_CODEandHEVY_RETRY_EXHAUSTED_ERROR_CODE.
packages/hevy-client/src/hevy-client-kubb.ts#L138-L150: export the set (for example asSAFE_OBSERVATION_CODES) and re-export it frompackages/hevy-client/src/index.tsso it becomes the single source of truth.packages/node/src/utils/hevy-client-observability.ts#L17-L29: delete the local set and import the exported one from@hevy-mcp/hevy-client; this also removes the two hard-coded code literals.🤖 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/hevy-client/src/hevy-client-kubb.ts` around lines 138 - 150, Export SAFE_OBSERVATION_CODES from hevy-client-kubb.ts and re-export it through packages/hevy-client/src/index.ts as the single source of truth. In packages/node/src/utils/hevy-client-observability.ts, remove the local allowlist and import the exported set from `@hevy-mcp/hevy-client`, eliminating the duplicated hard-coded error codes.packages/core/src/utils/error-handler.ts (1)
114-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider serializing the failure event before writing it to stderr.
console.errorreceives an object. Node formats it withutil.inspect, not JSON. Log collectors that parse stderr lines cannot read the event as structured data.debugLoginpackages/node/src/utils/debug.tswritesJSON.stringifyoutput for the same reason.If you want the event to stay machine-readable, serialize it here.
Note that the assertion in
packages/core/src/utils/error-handler.test.tsat Line 39 asserts the object form. Update it if you change the call.♻️ Proposed change
- console.error( - createMcpToolFailureEvent(context ?? "unknown", policy.type, diagnostic), - ); + console.error( + JSON.stringify( + createMcpToolFailureEvent(context ?? "unknown", policy.type, diagnostic), + ), + );🤖 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/core/src/utils/error-handler.ts` around lines 114 - 116, Serialize the failure event returned by createMcpToolFailureEvent before passing it to console.error, matching the JSON output behavior of debugLog. Update the related assertion in the error-handler test to expect the serialized string rather than the object form.packages/node/src/utils/telemetry.ts (1)
84-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider keeping the stack trace when you record an exception.
normalizeTelemetryErrorreturns only{ name }.recordTelemetryExceptionpasses this object torecordException, so the recorded exception event has nomessageand nostack. Droppingmessageis reasonable, since messages often embed request data. Droppingstackremoves debugging value. A Node.js stack trace usually contains only function names and file paths, not user data.Keep
stackon the normalized error when the input is anErrorinstance. This keeps the privacy control onmessagewhile restoring stack-based debugging in the collector.♻️ Proposed change to preserve stack traces
-function normalizeTelemetryError(error: unknown): { name: string } { +function normalizeTelemetryError(error: unknown): { name: string; stack?: string } { const candidate = error instanceof Error && typeof error.name === "string" ? error.name : undefined; const name = candidate && SAFE_EXCEPTION_TYPES.has(candidate) ? candidate : "UnknownError"; - return { name }; + return { + name, + stack: error instanceof Error ? error.stack : undefined, + }; }Also applies to: 105-123
🤖 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/utils/telemetry.ts` around lines 84 - 94, Update normalizeTelemetryError to include the input Error instance’s stack alongside the sanitized name, while continuing to omit message and use “UnknownError” for untrusted or non-Error names. Ensure recordTelemetryException passes this normalized stack through recordException without exposing request data.
🤖 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 `@docs/clickstack-metrics.md`:
- Around line 104-107: Update the histogram explanation in both referenced
sections to clarify that the N+1th +Inf bucket must not be treated as the last
ExplicitBounds value; describe it as capped or indeterminate, and note that
bounds should cover expected latency ranges to avoid understated p95/p99
estimates.
In `@packages/node/src/utils/hevy-client-observability.ts`:
- Around line 43-59: Update startApiSpan and the onRequestStart/onRetryWait flow
so the API span remains available as the parent for request processing and later
retry-wait spans; do not return a span created by the short-lived `(span) =>
span` callback as if its context remained active. Either retain and propagate an
explicit API-span scope through the request lifecycle or track the API span and
use it when creating later spans such as onRetryWait, while preserving cleanup
when processing completes.
---
Outside diff comments:
In `@packages/node/src/index.ts`:
- Around line 496-569: Update runStdioServer to track connection success
separately from connectAttempted: set the new success indicator only after
server.connect completes successfully, then use it in the outer catch to label
failures as "connect" only when connection failed and "run" for post-connect
setup errors. Preserve connectAttempted for distinguishing startup failures from
connection failures in session termination.
---
Nitpick comments:
In `@packages/core/src/utils/error-handler.ts`:
- Around line 114-116: Serialize the failure event returned by
createMcpToolFailureEvent before passing it to console.error, matching the JSON
output behavior of debugLog. Update the related assertion in the error-handler
test to expect the serialized string rather than the object form.
In `@packages/hevy-client/src/hevy-client-kubb.ts`:
- Around line 487-529: Update the observation construction in the request
error-handling flow to derive error.category from whether the original error
includes an HTTP response: use "HevyHttpError" for response-backed failures and
"NetworkError" for transport failures such as ECONNRESET without a status. Keep
the existing status, code, and retry behavior unchanged.
- Around line 138-150: Export SAFE_OBSERVATION_CODES from hevy-client-kubb.ts
and re-export it through packages/hevy-client/src/index.ts as the single source
of truth. In packages/node/src/utils/hevy-client-observability.ts, remove the
local allowlist and import the exported set from `@hevy-mcp/hevy-client`,
eliminating the duplicated hard-coded error codes.
In `@packages/node/src/utils/telemetry.ts`:
- Around line 84-94: Update normalizeTelemetryError to include the input Error
instance’s stack alongside the sanitized name, while continuing to omit message
and use “UnknownError” for untrusted or non-Error names. Ensure
recordTelemetryException passes this normalized stack through recordException
without exposing request data.
🪄 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: 4f94eb7f-1ca7-46e7-8b2c-aef245be18e1
📒 Files selected for processing (28)
.changeset/multi-process-metrics.md.changeset/opaque-telemetry-correlation.md.changeset/steady-failure-events.mddocs/clickstack-metrics.mdpackages/core/src/index.tspackages/core/src/server.tspackages/core/src/utils/cache.test.tspackages/core/src/utils/cache.tspackages/core/src/utils/error-handler.test.tspackages/core/src/utils/error-handler.tspackages/core/src/utils/exercise-template-catalog.tspackages/core/src/utils/tool-taxonomy.tspackages/hevy-client/src/hevy-client-kubb.tspackages/hevy-client/src/hevy-client.test.tspackages/hevy-client/src/index.tspackages/node/src/index.test.tspackages/node/src/index.tspackages/node/src/utils/hevy-client-observability.test.tspackages/node/src/utils/hevy-client-observability.tspackages/node/src/utils/mcp-session-observability.test.tspackages/node/src/utils/mcp-session-observability.tspackages/node/src/utils/sentry-privacy.test.tspackages/node/src/utils/stdio-observability.test.tspackages/node/src/utils/stdio-observability.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
a1207a1 to
9598541
Compare
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 |
Unit Test Results 1 files 65 suites 7s ⏱️ Results for commit 3d77375. ♻️ This comment has been updated with latest results. |
|
Addressed the review findings in be486be: fail-open guard for the SDK handler map; separate connect success from connect attempted and avoid duplicate connect failure events; valid package changesets; corrected +Inf histogram documentation and capped quantile naming; API request/retry work now runs under the API span context; shared safe observation-code export; NetworkError classification; serialized MCP failure stderr events; and preserved sanitized exception stacks. Validation: npm run build, npm run check, npm run check:types, npm run check:changeset, and npm run test:unit (65 files, 681 tests). |
Bundle ReportChanges will increase total bundle size by 17.19kB (8.0%) ⬆️
Affected Assets, Files, and Routes:view changes for bundle: hevy-mcp-esmAssets Changed:
Files in
|
|
Tick the box to add this pull request to the merge queue (same as
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #848 +/- ##
==========================================
+ Coverage 79.36% 80.88% +1.51%
==========================================
Files 67 68 +1
Lines 3746 3996 +250
Branches 1068 1144 +76
==========================================
+ Hits 2973 3232 +259
+ Misses 412 376 -36
- Partials 361 388 +27 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/hevy-client/src/hevy-client.test.ts (1)
203-225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that an expected 404 does not retry.
The change at
packages/hevy-client/src/hevy-client-kubb.tsline 560 makes expected 404s terminate immediately. This test does not check the fetch call count, so a regression that retries expected 404s would still pass. Add an assertion onfetchMock.💚 Proposed assertion
expect(observations).toEqual([ { outcome: "expected", expectedReason: "end_of_list" }, ]); + expect(fetchMock).toHaveBeenCalledTimes(1); });🤖 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/hevy-client/src/hevy-client.test.ts` around lines 203 - 225, Add an assertion in the test case around client.getWorkouts to verify fetchMock is called exactly once after the expected 404. Preserve the existing rejection and observations assertions while ensuring expected 404 responses do not trigger retries.packages/hevy-client/src/hevy-client-kubb.ts (1)
517-544: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving the underlying network code on retry exhaustion.
Line 520 replaces
error.codewithHEVY_RETRY_EXHAUSTED_ERROR_CODE. The observation then reports the retry code instead of the transport code, for exampleETIMEDOUT. The test atpackages/hevy-client/src/hevy-client.test.tslines 257-287 confirms this loss. If you want transport-level diagnosis in metrics, capture the original code before you overwrite it and report it in the observation.
🤖 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/hevy-client/src/hevy-client-kubb.ts`:
- Around line 282-292: Update runRequestObservation to track whether operation
has started before invoking scope.run, marking it when execution begins. In the
synchronous catch, call operation as a fallback only if it has not started;
otherwise rethrow the original error to prevent duplicate POST or PUT requests.
In `@packages/node/src/index.test.ts`:
- Around line 303-312: Strengthen the assertion in the malformed getUserInfo
rejection test around createNodeMcpServer so it verifies the actual sanitized
console.error diagnostic, rather than merely finding one call without
"not-a-status". Assert the expected output for a response with no HTTP status,
ensuring the diagnostic omits any status value while preserving the existing
server-resolution behavior.
In `@packages/node/src/utils/hevy-client-observability.ts`:
- Around line 82-92: Update the request lifecycle around observationScope and
scope.run in hevy-client-kubb.ts so finishRequestObservation runs from the
finally path for every request outcome, including caller cancellation, timeouts,
and failures thrown by scope.run. Ensure aborted or timed-out requests pass a
valid error/failure observation, while preserving the existing
successful-response and caught-error observation behavior.
---
Nitpick comments:
In `@packages/hevy-client/src/hevy-client.test.ts`:
- Around line 203-225: Add an assertion in the test case around
client.getWorkouts to verify fetchMock is called exactly once after the expected
404. Preserve the existing rejection and observations assertions while ensuring
expected 404 responses do not trigger retries.
🪄 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: 2e4c3944-ed30-4e1f-ad3f-345c703c8bd6
📒 Files selected for processing (28)
.changeset/multi-process-metrics.md.changeset/opaque-telemetry-correlation.md.changeset/steady-failure-events.mddocs/clickstack-metrics.mdpackages/core/src/index.tspackages/core/src/server.tspackages/core/src/utils/cache.test.tspackages/core/src/utils/cache.tspackages/core/src/utils/error-handler.test.tspackages/core/src/utils/error-handler.tspackages/core/src/utils/exercise-template-catalog.tspackages/core/src/utils/tool-taxonomy.tspackages/hevy-client/src/hevy-client-kubb.tspackages/hevy-client/src/hevy-client.test.tspackages/hevy-client/src/index.tspackages/node/src/index.test.tspackages/node/src/index.tspackages/node/src/utils/hevy-client-observability.test.tspackages/node/src/utils/hevy-client-observability.tspackages/node/src/utils/mcp-session-observability.test.tspackages/node/src/utils/mcp-session-observability.tspackages/node/src/utils/sentry-privacy.test.tspackages/node/src/utils/stdio-observability.test.tspackages/node/src/utils/stdio-observability.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
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/core/src/utils/error-handler.test.ts
- packages/core/src/utils/error-handler.ts
- packages/node/src/utils/sentry-privacy.test.ts
- packages/core/src/utils/cache.test.ts
- .changeset/multi-process-metrics.md
- packages/core/src/utils/tool-taxonomy.ts
- packages/hevy-client/src/index.ts
- packages/node/src/utils/mcp-session-observability.test.ts
- packages/core/src/server.ts
- packages/core/src/utils/exercise-template-catalog.ts
- packages/node/src/utils/tool-observer.test.ts
- packages/node/src/utils/mcp-session-observability.ts
- packages/node/src/utils/tool-observer.ts
- packages/core/src/utils/cache.ts
- packages/node/src/utils/stdio-observability.ts
- packages/node/src/utils/telemetry.ts
| onRequestStart(start) { | ||
| const span = startApiSpan(start); | ||
| return { | ||
| run<T>(operation: () => Promise<T>) { | ||
| return context.with(trace.setSpan(context.active(), span), operation); | ||
| }, | ||
| finish(observation) { | ||
| finishApiSpan(span, observation); | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the Hevy client uses onRequestStart scopes.
rg -n -C 10 'onRequestStart|\.finish\(|\.run\(' packages/hevy-client/src/hevy-client-kubb.tsRepository: chrisdoc/hevy-mcp
Length of output: 3591
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant section of hevy-client-kubb.ts =="
sed -n '400,490p' packages/hevy-client/src/hevy-client-kubb.ts
echo
echo "== full finishRequestObservation and runRequestObservation definitions/usages =="
rg -n -C 4 'finishRequestObservation|runRequestObservation|observationScope|controller|AbortController|abort|fetch\(' packages/hevy-client/src/hevy-client-kubb.ts
echo
echo "== observability create/usage =="
sed -n '1,120p' packages/node/src/utils/hevy-client-observability.tsRepository: chrisdoc/hevy-mcp
Length of output: 10932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/hevy-client/src/hevy-client-kubb.ts")
text = p.read_text()
start = text.find("while (true) {")
finish = text.find("}) as KubbClient;", start)
block = text[start:finish]
print("finishRequestObservation occurrences:", text.count("finishRequestObservation(observationScope"))
print("observationScope references in request loop:", [i+1 for i,b in enumerate(block) if b=='o' and text[i:i len("observationScope")]== observationScope")
print("finally close:", "finally {" in block, "finishRequestObservation(observationScope, observation);" in block, block.endswith("}) as KubbClient;"))
# Lightweight lexical count around the body to show all non-observationScope calls cannot close the span.
print("timeout.clear references:", ["clearTimeout(timeout)" in block, block.count("clearTimeout(timeout)")])
PYRepository: chrisdoc/hevy-mcp
Length of output: 485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/hevy-client/src/hevy-client-kubb.ts")
text = p.read_text()
block = text[text.find("while (true) {"):text.find("}) as KubbClient;", text.find("while (true) {"))]
print("finishRequestObservation(scope, observation) occurrences:", block.count("finishRequestObservation(observationScope, observation)"))
print("clear timeout statements:", block.count("clearTimeout(timeout)"))
print("finally present:", "finally {" in block)
print("scope abort hook present:", "scope" in block and ".abort" in block, block.count(".abort"))
# Extract request body and show whether any abort handler calls finishRequestObservation or observationScope.
lines = block.splitlines()
for i, line in enumerate(lines, start=text[:block].count("\n") + 1):
if "addEventListener" in line or "abort" in line.lower():
print(f"line {i}: {line.strip()}")
PYRepository: chrisdoc/hevy-mcp
Length of output: 451
Call scope?.finish(observation) for aborted or timed-out requests.
packages/hevy-client/src/hevy-client-kubb.ts only calls finishRequestObservation(observationScope, observation) on successful responses and caught request failures. When AbortController.abort() is triggered by caller cancellation or timeout, or scope.run() itself throws, the scope is discarded and the OpenTelemetry span never ends. Move finishRequestObservation into the finally block, but pass a valid error/failure observation for aborts/timeouts so spans are closed in every request path.
🤖 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/utils/hevy-client-observability.ts` around lines 82 - 92,
Update the request lifecycle around observationScope and scope.run in
hevy-client-kubb.ts so finishRequestObservation runs from the finally path for
every request outcome, including caller cancellation, timeouts, and failures
thrown by scope.run. Ensure aborted or timed-out requests pass a valid
error/failure observation, while preserving the existing successful-response and
caught-error observation behavior.
1 participant
Summary
Validation
npm run buildnpm run test:unit(65 files, 674 tests)npm run checknpm run check:typesCloses #846
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
✨ PR Description
Purpose: Enable multi-process safe OTLP metrics collection and add structured observability for cache operations, API requests, and lifecycle failures.
Main changes:
Generated 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