feat: improve OpenTelemetry trace fidelity - #847
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
🚧 Files skipped from review as they are similar to previous changes (24)
📝 WalkthroughWalkthroughThe PR adds privacy-safe MCP session correlation, structured Hevy request outcomes, cache lifecycle observation, bounded failure telemetry, span taxonomy, service identity, and related tests across the core, client, and Node packages. ChangesTelemetry observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant NodeServer
participant HevyClient
participant OpenTelemetry
MCPClient->>NodeServer: initialize or invoke tool
NodeServer->>OpenTelemetry: create session, protocol, or tool span
NodeServer->>HevyClient: execute API request
HevyClient->>OpenTelemetry: report request, retry, and outcome observations
NodeServer->>OpenTelemetry: finish tool span or record bounded failure
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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 substantially improves OTel trace fidelity with well-structured observer patterns, privacy-safe session IDs, and bounded metadata. Three issues are worth addressing before merging.
3 issues detected:
🐞 Bug - `startActiveSpan` with an immediately-returning callback creates a span but activates its context only for a zero-duration synchronous frame; the real async request work is never inside that active context.
Details: startApiSpan calls tracer.startActiveSpan with a callback that immediately returns the span ((span) => span). startActiveSpan sets the span as the active context only for the duration of the callback, which completes synchronously and instantly. All subsequent async work (the actual HTTP request) runs outside that context, so child spans won't be parented correctly and the span won't be the "active" span during the request. tracer.startSpan is the correct API when the caller owns the span lifecycle.
File: packages/node/src/utils/hevy-client-observability.ts (43-59)
🧹 Maintainability - Raw string literals `"HEVY_REQUEST_ABORTED"` and `"HEVY_RETRY_EXHAUSTED"` duplicate the canonical constants without a compile-time link, so a rename in hevy-client-kubb.ts won't be caught.
Details: SAFE_OBSERVATION_CODES on lines 17-29 hardcodes the string literals "HEVY_REQUEST_ABORTED" and "HEVY_RETRY_EXHAUSTED" instead of referencing the exported constants HEVY_REQUEST_ABORTED_ERROR_CODE and HEVY_RETRY_EXHAUSTED_ERROR_CODE from @hevy-mcp/hevy-client. If those constants are ever renamed, the node-side filter will silently pass codes it should suppress, potentially leaking internal error identifiers into metric dimensions.
File: packages/node/src/utils/hevy-client-observability.ts (17-29)
🧹 Maintainability - Accessing an underscore-prefixed private property of the MCP SDK Protocol class means any SDK upgrade can silently drop all protocol-level spans without throwing an error.
Details: installSdkErrorTracking reads protocol._requestHandlers (lines 299-301), a private Map that is not part of the MCP SDK's public API. The guard if (!handlers) return silently disables the entire tools/call and server/discover instrumentation whenever the SDK restructures this internal (e.g. after an SDK upgrade). Per user instructions, the stdio observability suite must be rerun after SDK upgrades — this is precisely why: silent fallback means missing spans with no test failure.
File: packages/node/src/index.ts (299-342)
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
PR Summary by QodoImprove OpenTelemetry trace fidelity across MCP server telemetry
AI Description
Diagram
Files changed (26)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/utils/tool-observer.ts (1)
202-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrompt failures are tagged under
mcp.tool.name.Line 205 sets
mcp.tool.namefor every invocation. Line 215 still branches oninvocation.kind === "prompt"for the context key, and the fingerprint at line 221 usesmcp-prompt-failure. A prompt failure therefore reports a prompt name under a tool-name tag. Sentry search and grouping bymcp.tool.namewill mix prompts and tools.Line 202 also computes
isPrompt, and line 215 recomputes the same condition inline. Reuse the local.🐛 Proposed fix
Sentry.withScope((scope) => { - scope.setTag("mcp.tool.name", invocation.name); + scope.setTag(isPrompt ? "mcp.prompt.name" : "mcp.tool.name", invocation.name); scope.setTag("error.type", completion.errorType ?? "UNKNOWN_ERROR"); @@ - scope.setContext(invocation.kind === "prompt" ? "mcpPrompt" : "mcpTool", { + scope.setContext(isPrompt ? "mcpPrompt" : "mcpTool", { context: invocation.name,🤖 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/tool-observer.ts` around lines 202 - 218, Update the Sentry scope in the failure-reporting flow to tag the invocation name as a prompt-specific key when isPrompt is true, while retaining mcp.tool.name for tools. Reuse the existing isPrompt local for the context key instead of recomputing invocation.kind, and ensure the prompt fingerprint uses the same prompt classification so prompt failures remain separated from tool failures.
🧹 Nitpick comments (14)
packages/node/src/index.ts (1)
231-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the session ID lookup at lines 262-264.
getCurrentMcpSessionId()is called twice in the same expression. Both calls read the same AsyncLocalStorage store in the same tick, so the result is identical. Every other site in this PR assigns the value to a localsessionIdfirst. Match that pattern.♻️ Proposed change
} else { + const sessionId = getCurrentMcpSessionId(); tracer.startActiveSpan( "mcp.sdk.failure", { attributes: { "mcp.span.category": "protocol", - ...(getCurrentMcpSessionId() - ? { "mcp.session.id": getCurrentMcpSessionId() } - : {}), + ...(sessionId ? { "mcp.session.id": sessionId } : {}), }, },🤖 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 231 - 297, In installSdkErrorTracking, assign getCurrentMcpSessionId() once to a local sessionId before tracer.startActiveSpan, then reuse that variable for the conditional mcp.session.id attribute instead of calling the lookup twice.packages/node/src/utils/telemetry.ts (2)
189-202: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrefer a direct random fallback over an HMAC keyed with
Math.random().The fallback places all entropy in the HMAC key (
pidplusMath.random()) and hashes a constant message (name:version). This works, but it is an unusual use of HMAC and depends on the non-cryptographicMath.random().randomBytescomes from the samenode:cryptomodule that the fallback already imports, so it is available on this path.♻️ Proposed simpler fallback
- return createHmac("sha256", `${process.pid}:${Math.random()}`) - .update(`${name}:${version}`) - .digest("hex") - .slice(0, 32); + return randomBytes(16).toString("hex");Add
randomBytesto the existingnode:cryptoimport.🤖 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 189 - 202, Update createServiceInstanceId’s fallback to use randomBytes from the existing node:crypto import, replacing the HMAC construction and its Math.random()-based key with a direct random opaque identifier while preserving the existing 32-character return length.
128-145: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMove
span.end()into afinallyblock.If
recordTelemetryExceptionthrows, the callback skipsspan.end()and the span never closes. The outertryswallows the error, so the leak stays silent.recordTelemetryExceptioncurrently guards its own body, so this is defensive only.♻️ Proposed defensive fix
(span) => { - const normalized = normalizeTelemetryError(error); - const code = getSafeExceptionCode(error); - recordTelemetryException( - error, - { - "exception.source": source, - "error.category": normalized.name, - ...(code ? { "error.code": code } : {}), - }, - span, - ); - span.end(); + try { + const normalized = normalizeTelemetryError(error); + const code = getSafeExceptionCode(error); + recordTelemetryException( + error, + { + "exception.source": source, + "error.category": normalized.name, + ...(code ? { "error.code": code } : {}), + }, + span, + ); + } finally { + span.end(); + } },🤖 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 128 - 145, Update the callback passed to tracer.startActiveSpan around recordTelemetryException so span.end() executes in a finally block, including when normalization, code extraction, or exception recording throws. Preserve the existing telemetry attributes and outer error-handling behavior.packages/node/src/utils/tool-observer.test.ts (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the session ID reaches the span attributes.
The mock returns
"session-1", but thestartActiveSpantest double at line 18 discards_options. No test in this file captures span attributes, so nothing verifies thatcreateAttributesattachesmcp.session.id. The PR objective requires a deterministic test for session propagation onto tool spans.Capture the options argument and assert the attribute.
💚 Proposed change
startActiveSpan: vi.fn( ( _name: string, - _options: unknown, + options: unknown, callback: (span: unknown) => unknown, ) => { testDoubles.activeSpanDepth += 1; + testDoubles.lastSpanOptions = options; return Promise.resolve(callback(testDoubles.span)).finally(() => { testDoubles.activeSpanDepth -= 1; }); }, ),Add
lastSpanOptions: undefined as unknowntotestDoubles, then assert in a new test:it("attaches the MCP session ID to the tool span", async () => { const scope = startScope(); await scope.run(() => Promise.resolve("ok")); expect(testDoubles.lastSpanOptions).toMatchObject({ attributes: { "mcp.session.id": "session-1", "mcp.span.category": "tool" }, }); });Also applies to: 78-78
🤖 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/tool-observer.test.ts` at line 46, Update the startActiveSpan test double and its testDoubles state to capture the span options argument, then add a deterministic test that runs the tool scope and asserts the captured attributes include mcp.session.id set to session-1 and mcp.span.category set to tool. Apply the same assertion coverage to the related occurrence.packages/node/src/utils/telemetry.test.ts (1)
353-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
createServiceInstanceIdfallback branches.The test only covers the happy path with an injected generator. The new validation logic in
createServiceInstanceIdhas three untested branches: an empty generator result, a result longer than 128 characters, and a generator that throws. ThecreateHmac,hmacUpdate, andhmacDigesttest doubles already exist in this file, so asserting the fallback is cheap.💚 Proposed additional cases
it("supports deterministic service-instance IDs for tests", async () => { vi.resetModules(); const mod = await import("./telemetry.js"); expect(mod.createServiceInstanceId(() => "instance-test-id")).toBe( "instance-test-id", ); }); + + it("falls back to a process-local ID for invalid or failing generators", async () => { + vi.resetModules(); + const mod = await import("./telemetry.js"); + + expect(mod.createServiceInstanceId(() => "")).toBe("abcdef0123456789"); + expect(mod.createServiceInstanceId(() => "x".repeat(129))).toBe( + "abcdef0123456789", + ); + expect( + mod.createServiceInstanceId(() => { + throw new Error("no entropy"); + }), + ).toBe("abcdef0123456789"); + });🤖 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.test.ts` around lines 353 - 361, Add tests alongside “supports deterministic service-instance IDs for tests” covering createServiceInstanceId when the injected generator returns an empty string, returns a value exceeding 128 characters, and throws. For each case, assert that the function falls back to the expected HMAC-based ID using the existing createHmac, hmacUpdate, and hmacDigest test doubles.packages/node/src/utils/tool-observer.ts (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the discovery category from the taxonomy instead of a hardcoded name list.
DISCOVERY_TOOL_NAMESholds one literal tool name. Any new discovery tool must be added to this Set by hand, and nothing connects the Set to the tool registry. The invocation already carriesinvocation.taxonomywithfeature,kind, andoperationfields. Using the taxonomy keeps one source of truth for the span category.If the taxonomy has no field that identifies discovery operations, add one in
packages/corerather than maintaining the name list here.Also applies to: 62-66
🤖 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/tool-observer.ts` at line 25, Replace the hardcoded DISCOVERY_TOOL_NAMES check in the tool-observer classification flow with the discovery indicator from invocation.taxonomy, preserving the existing span category behavior for discovery operations. If the taxonomy defined in packages/core lacks a field identifying discovery operations, add that field there and update the tool registry to populate it, keeping taxonomy as the single source of truth.packages/core/src/utils/cache.test.ts (1)
143-147: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the complete observer input.
The callback destructures
state, so this test does not detect a futurekey
property on the observation object. Store each observation before destructuring it.
Then assert that each object contains onlystate.Proposed test update
- const events: string[] = []; + const events: string[] = []; + const observations: Array<{ state: string }> = []; const cache = new AsyncTtlCache<string, string>({ ttlMs: 60_000, maxSize: 2, observer: { - start: ({ state }) => { + start: (observation) => { + observations.push(observation); + const { state } = observation; events.push(`start:${state}`); return { finish: () => events.push(`finish:${state}`) }; }, }, });Add an exact
observationsassertion formiss,inflight_wait,hit, and
refresh.🤖 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/cache.test.ts` around lines 143 - 147, Update the observer callback in the cache test to record each complete observation object before reading state, then assert exact observation objects for miss, inflight_wait, hit, and refresh containing only state. Keep the existing event assertions while ensuring any unexpected key property causes the test to fail.packages/node/src/utils/hevy-client-observability.test.ts (2)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert with
SpanStatusCode.OKinstead of the literal1.The literal couples the test to the numeric value of the enum and hides the intent.
@opentelemetry/apiis already a dependency of this test file's subject.♻️ Proposed change
- expect(testDoubles.span.setStatus).toHaveBeenCalledWith({ code: 1 }); + expect(testDoubles.span.setStatus).toHaveBeenCalledWith({ + code: SpanStatusCode.OK, + });Add
import { SpanStatusCode } from "@opentelemetry/api";to the imports.🤖 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.test.ts` at line 76, Update the status assertion in the observability test to compare against SpanStatusCode.OK rather than the numeric literal 1, and import SpanStatusCode from `@opentelemetry/api` alongside the existing imports.
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
telemetry.jsmock omitsstartSpan, so the retry-wait and cache paths cannot be tested.
packages/node/src/utils/hevy-client-observability.tscallstracer.startSpanat line 142 foronRetryWaitand at line 164 forcreateNodeCacheObserver. This mock provides onlystartActiveSpan. Any test that exercises those paths fails with "startSpan is not a function". Both paths are new behavior in this PR and currently have no coverage here. AddstartSpanto the mock and add cases for the retry-wait span attributes and the cache span metadata.♻️ Proposed mock extension
const testDoubles = vi.hoisted(() => ({ span: { addEvent: vi.fn(), end: vi.fn(), setAttribute: vi.fn(), setStatus: vi.fn(), }, startActiveSpan: vi.fn((...args: unknown[]) => { const callback = args.at(-1) as (span: unknown) => unknown; return callback(testDoubles.span); }), + startSpan: vi.fn(() => testDoubles.span), apiCallsAdd: vi.fn(), apiDurationRecord: vi.fn(), })); vi.mock("./telemetry.js", () => ({ - tracer: { startActiveSpan: testDoubles.startActiveSpan }, + tracer: { + startActiveSpan: testDoubles.startActiveSpan, + startSpan: testDoubles.startSpan, + },🤖 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.test.ts` around lines 20 - 21, Extend the telemetry mock used by the observability tests to provide tracer.startSpan alongside startActiveSpan, then add coverage for onRetryWait and createNodeCacheObserver that verifies their span attributes and cache metadata. Use the existing test doubles and assertion patterns in hevy-client-observability.test.ts.packages/hevy-client/src/hevy-client.test.ts (2)
156-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA single microtask tick may not be enough to reach the request-start callback.
await Promise.resolve()yields one microtask. The generated client andwithTimeoutadd await points before line 401 runsemitRequestStart. If any of those points consume more than one tick,eventsis empty and the assertion fails. Usevi.waitForso the test waits for the start event instead of assuming a fixed tick count.♻️ Proposed change
const request = client.getUserInfo(); - await Promise.resolve(); - expect(events).toEqual(["start"]); + await vi.waitFor(() => expect(events).toEqual(["start"])); releaseBody(); await request;🤖 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 156 - 158, Update the getUserInfo request-start test to use vi.waitFor around the events assertion, waiting until the “start” event is emitted instead of relying on a single Promise.resolve microtask. Keep the expected events value unchanged.
198-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises read-endpoint coverage that the body does not provide.
The title says "marks supported read and later-page 404s as expected outcomes". The body only exercises
getWorkouts({ page: 2 }), which producesend_of_list. No case coversEXPECTED_READ_404_ENDPOINTSand thenot_foundreason. Add a read case, for examplegetWorkout("id")asserting{ outcome: "expected", expectedReason: "not_found" }, or narrow the title to the later-page case.🤖 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 198 - 220, The test named “marks supported read and later-page 404s as expected outcomes” must cover both behaviors promised by its title. Extend the test using a supported single-read client call such as getWorkout("id") and assert an expected observation with expectedReason "not_found", while preserving the existing page-2 end_of_list assertion; alternatively, narrow the test name to describe only the later-page case.packages/node/src/utils/hevy-client-observability.ts (2)
17-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the exported error-code constants instead of repeating the literals.
@hevy-mcp/hevy-clientexportsHEVY_REQUEST_ABORTED_ERROR_CODEandHEVY_RETRY_EXHAUSTED_ERROR_CODE. This set hardcodes"HEVY_REQUEST_ABORTED"and"HEVY_RETRY_EXHAUSTED". If either constant value changes, this copy silently drops the code from telemetry and no compiler error appears. The whole set also duplicatesSAFE_OBSERVATION_CODESinpackages/hevy-client/src/hevy-client-kubb.tslines 138-150; consider exporting the set once from the client package and importing it here.♻️ Minimum change
+import { + HEVY_REQUEST_ABORTED_ERROR_CODE, + HEVY_RETRY_EXHAUSTED_ERROR_CODE, +} from "`@hevy-mcp/hevy-client`"; + const SAFE_OBSERVATION_CODES = new Set([ "EAI_AGAIN", "ECONNABORTED", "ECONNREFUSED", "ECONNRESET", "ENETUNREACH", "ENOTFOUND", "ERR_NETWORK", "ERR_SOCKET_TIMEOUT", "ETIMEDOUT", - "HEVY_REQUEST_ABORTED", - "HEVY_RETRY_EXHAUSTED", + HEVY_REQUEST_ABORTED_ERROR_CODE, + HEVY_RETRY_EXHAUSTED_ERROR_CODE, ]);🤖 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 17 - 29, Update SAFE_OBSERVATION_CODES to import and use HEVY_REQUEST_ABORTED_ERROR_CODE and HEVY_RETRY_EXHAUSTED_ERROR_CODE from `@hevy-mcp/hevy-client` instead of hardcoded values. Prefer reusing an exported safe-observation-code set from the client package if available, while preserving the existing telemetry behavior.
43-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
startSpanfor the API request span.
startApiSpanreturns the span from the callback before the HTTP attempt runs, so the active context does not cover the request. The retry-wait span created byonRetryWait()and any auto-instrumented HTTP spans attach to the enclosing tool span instead of this API attempt. Replacetracer.startActiveSpan(..., (span) => span)withtracer.startSpan(...)unless the request work is explicitly moved into the span’s context.♻️ Minimum clarity change
function startApiSpan(start: HevyRequestStart) { const sessionId = getCurrentMcpSessionId(); - return tracer.startActiveSpan( - `hevy.api.${start.method}`, - { - attributes: { - "mcp.span.category": "api", - "http.method": start.method, - "hevy.api.endpoint": start.endpoint, - "hevy.api.retry_count_bucket": bucketCount(start.retryCount), - "mcp.transport": getCurrentMcpTransport(), - ...(sessionId ? { "mcp.session.id": sessionId } : {}), - }, - }, - (span) => span, - ); + return tracer.startSpan(`hevy.api.${start.method}`, { + attributes: { + "mcp.span.category": "api", + "http.method": start.method, + "hevy.api.endpoint": start.endpoint, + "hevy.api.retry_count_bucket": bucketCount(start.retryCount), + "mcp.transport": getCurrentMcpTransport(), + ...(sessionId ? { "mcp.session.id": sessionId } : {}), + }, + }); }Note: Update
packages/node/src/utils/hevy-client-observability.test.tslines 63-75 if this matches the existing assertion.🤖 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 43 - 59, Update startApiSpan to create and return the API request span with tracer.startSpan instead of startActiveSpan with a callback, preserving the existing span name and attributes. Adjust the corresponding test assertion in the observability test if it currently expects the active-span callback behavior.packages/hevy-client/src/hevy-client-kubb.ts (1)
123-137: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider adding
/v1/exercise_history/:exerciseTemplateIdto the expected read 404 set.
SAFE_DYNAMIC_ENDPOINTSmaps/v1/exercise_history/to/v1/exercise_history/:exerciseTemplateId, butEXPECTED_READ_404_ENDPOINTSomits it. A 404 for an unknown exercise template therefore classifies asterminal_failureand raises a span error, while the equivalent 404 for/v1/workouts/:workoutIdclassifies asexpected. If the omission is deliberate, no change is needed.♻️ Proposed addition
const EXPECTED_READ_404_ENDPOINTS = new Set([ "/v1/body_measurements/:date", + "/v1/exercise_history/:exerciseTemplateId", "/v1/exercise_templates/:exerciseTemplateId", "/v1/routine_folders/:folderId", "/v1/routines/:routineId", "/v1/workouts/:workoutId", ]);🤖 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 123 - 137, Add "/v1/exercise_history/:exerciseTemplateId" to EXPECTED_READ_404_ENDPOINTS so unknown exercise-template history reads classify 404 responses as expected, consistent with the SAFE_DYNAMIC_ENDPOINTS mapping and other read endpoints.
🤖 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/steady-failure-events.md:
- Around line 1-2: Update the changeset metadata by declaring source-code bumps
for `@hevy-mcp/core` and `@hevy-mcp/hevy-client`, and include hevy-mcp when those
packages are part of the public bundled release. Ensure the relevant changeset
file contains valid package entries instead of empty front matter.
In `@packages/hevy-client/src/hevy-client-kubb.ts`:
- Around line 499-526: Update the HevyRequestObservation construction in the
request error handling flow to classify retry-exhausted requests as
terminal_failure by checking the exhaustion condition before canRetry. In the
same observation error object, derive category from whether error.status is
absent so network failures are reported as NetworkError while HTTP failures
remain HevyHttpError; preserve the existing safe code handling.
In `@packages/node/src/index.ts`:
- Around line 194-229: Update markSdkToolFailure to accept an optional
error-type parameter that defaults to the safe diagnostic category, and use it
for both the mcp.tool.failure event and span error.type attribute and telemetry
fields. At the createToolError validation call site, pass VALIDATION_ERROR
explicitly; leave the tools/call catch path on the diagnostic-category default.
In `@packages/node/src/utils/hevy-client-observability.ts`:
- Around line 61-70: Update finishApiSpan to set the HTTP status only when
observation.status > 0, preventing fallback status 0 from being recorded.
Replace the deprecated http.status_code attribute with http.response.status_code
and use http.request.method for method attributes wherever this telemetry flow
defines them, retaining legacy names only when required by existing collectors.
In `@packages/node/src/utils/mcp-session-observability.test.ts`:
- Around line 51-69: Update the test case around createMcpSessionContext and
runWithMcpSessionContext to call recordMcpSessionStart within a scoped session
context before checking testDoubles.sessionStartedAdd. Keep the existing
assertion that the opaque telemetry ID is not passed to the metric, ensuring the
metric path is actually exercised.
---
Outside diff comments:
In `@packages/node/src/utils/tool-observer.ts`:
- Around line 202-218: Update the Sentry scope in the failure-reporting flow to
tag the invocation name as a prompt-specific key when isPrompt is true, while
retaining mcp.tool.name for tools. Reuse the existing isPrompt local for the
context key instead of recomputing invocation.kind, and ensure the prompt
fingerprint uses the same prompt classification so prompt failures remain
separated from tool failures.
---
Nitpick comments:
In `@packages/core/src/utils/cache.test.ts`:
- Around line 143-147: Update the observer callback in the cache test to record
each complete observation object before reading state, then assert exact
observation objects for miss, inflight_wait, hit, and refresh containing only
state. Keep the existing event assertions while ensuring any unexpected key
property causes the test to fail.
In `@packages/hevy-client/src/hevy-client-kubb.ts`:
- Around line 123-137: Add "/v1/exercise_history/:exerciseTemplateId" to
EXPECTED_READ_404_ENDPOINTS so unknown exercise-template history reads classify
404 responses as expected, consistent with the SAFE_DYNAMIC_ENDPOINTS mapping
and other read endpoints.
In `@packages/hevy-client/src/hevy-client.test.ts`:
- Around line 156-158: Update the getUserInfo request-start test to use
vi.waitFor around the events assertion, waiting until the “start” event is
emitted instead of relying on a single Promise.resolve microtask. Keep the
expected events value unchanged.
- Around line 198-220: The test named “marks supported read and later-page 404s
as expected outcomes” must cover both behaviors promised by its title. Extend
the test using a supported single-read client call such as getWorkout("id") and
assert an expected observation with expectedReason "not_found", while preserving
the existing page-2 end_of_list assertion; alternatively, narrow the test name
to describe only the later-page case.
In `@packages/node/src/index.ts`:
- Around line 231-297: In installSdkErrorTracking, assign
getCurrentMcpSessionId() once to a local sessionId before
tracer.startActiveSpan, then reuse that variable for the conditional
mcp.session.id attribute instead of calling the lookup twice.
In `@packages/node/src/utils/hevy-client-observability.test.ts`:
- Line 76: Update the status assertion in the observability test to compare
against SpanStatusCode.OK rather than the numeric literal 1, and import
SpanStatusCode from `@opentelemetry/api` alongside the existing imports.
- Around line 20-21: Extend the telemetry mock used by the observability tests
to provide tracer.startSpan alongside startActiveSpan, then add coverage for
onRetryWait and createNodeCacheObserver that verifies their span attributes and
cache metadata. Use the existing test doubles and assertion patterns in
hevy-client-observability.test.ts.
In `@packages/node/src/utils/hevy-client-observability.ts`:
- Around line 17-29: Update SAFE_OBSERVATION_CODES to import and use
HEVY_REQUEST_ABORTED_ERROR_CODE and HEVY_RETRY_EXHAUSTED_ERROR_CODE from
`@hevy-mcp/hevy-client` instead of hardcoded values. Prefer reusing an exported
safe-observation-code set from the client package if available, while preserving
the existing telemetry behavior.
- Around line 43-59: Update startApiSpan to create and return the API request
span with tracer.startSpan instead of startActiveSpan with a callback,
preserving the existing span name and attributes. Adjust the corresponding test
assertion in the observability test if it currently expects the active-span
callback behavior.
In `@packages/node/src/utils/telemetry.test.ts`:
- Around line 353-361: Add tests alongside “supports deterministic
service-instance IDs for tests” covering createServiceInstanceId when the
injected generator returns an empty string, returns a value exceeding 128
characters, and throws. For each case, assert that the function falls back to
the expected HMAC-based ID using the existing createHmac, hmacUpdate, and
hmacDigest test doubles.
In `@packages/node/src/utils/telemetry.ts`:
- Around line 189-202: Update createServiceInstanceId’s fallback to use
randomBytes from the existing node:crypto import, replacing the HMAC
construction and its Math.random()-based key with a direct random opaque
identifier while preserving the existing 32-character return length.
- Around line 128-145: Update the callback passed to tracer.startActiveSpan
around recordTelemetryException so span.end() executes in a finally block,
including when normalization, code extraction, or exception recording throws.
Preserve the existing telemetry attributes and outer error-handling behavior.
In `@packages/node/src/utils/tool-observer.test.ts`:
- Line 46: Update the startActiveSpan test double and its testDoubles state to
capture the span options argument, then add a deterministic test that runs the
tool scope and asserts the captured attributes include mcp.session.id set to
session-1 and mcp.span.category set to tool. Apply the same assertion coverage
to the related occurrence.
In `@packages/node/src/utils/tool-observer.ts`:
- Line 25: Replace the hardcoded DISCOVERY_TOOL_NAMES check in the tool-observer
classification flow with the discovery indicator from invocation.taxonomy,
preserving the existing span category behavior for discovery operations. If the
taxonomy defined in packages/core lacks a field identifying discovery
operations, add that field there and update the tool registry to populate it,
keeping taxonomy as the single source of truth.
🪄 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: d5b19b36-7355-441b-9bce-55629118fab2
📒 Files selected for processing (26)
.changeset/opaque-telemetry-correlation.md.changeset/steady-failure-events.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
|
Resolved in b670f6d: initialize spans now receive the generated session ID; changesets are valid package bumps with summaries; exhausted retries are terminal and network failures use NetworkError; SDK validation failures are labeled explicitly; HTTP span attributes use stable semantic names and omit status 0; session metric privacy test exercises the metric path; API spans use startSpan and retry/cache observer coverage was added. Focused review suites: 70 tests passed. |
b670f6d to
0ea209d
Compare
Cloudflare Worker preview
|
Bundle ReportChanges will increase total bundle size by 16.47kB (7.66%) ⬆️
Affected Assets, Files, and Routes:view changes for bundle: hevy-mcp-esmAssets Changed:
Files in
|
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
==========================================
- Coverage 79.36% 77.56% -1.80%
==========================================
Files 67 68 +1
Lines 3746 3981 +235
Branches 1068 1141 +73
==========================================
+ Hits 2973 3088 +115
- Misses 412 514 +102
- Partials 361 379 +18 ☔ View full report in Codecov by Harness. |
|
Tick the box to add this pull request to the merge queue (same as
|
Unit Test Results 1 files 65 suites 7s ⏱️ Results for commit 0ea209d. |
1 participant
Summary
Closes #845
Validation
npm run buildnpm run test:unitnpm run test:stdionpm run checknpm run check:typesnpm run check:changesetremains blocked by the local checkout's missingorigin/mainbaseline object.Summary by CodeRabbit
✨ PR Description
Purpose: Enhance OpenTelemetry trace fidelity by adding exception tracking, cache observation spans, and structured failure events across API, cache, and SDK layers.
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