feat: emit activity-only user metrics #917 - #918
Conversation
|
3 clusters identified |
|
ⓘ 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: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
📝 WalkthroughWalkthroughThe change adds activity-only telemetry for tool, resource, and prompt calls. It instruments Hevy resources, records pseudonymous user activity, expands observation types, and documents DAU, WAU, and MAU dashboards with a separate all-span diagnostic metric. ChangesMCP activity telemetry
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant ToolObserver
participant Telemetry
participant ActivityMetric
MCPClient->>ToolObserver: invoke tool, resource, or prompt
ToolObserver->>Telemetry: read telemetry user hash
Telemetry-->>ToolObserver: user hash
ToolObserver->>ActivityMetric: record activity kind and user hash
ToolObserver-->>MCPClient: complete observed operation
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 QodoEmit activity-only user metrics for tools, resources, and prompts
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
charliecreates
Bot
changed the title
![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 wires mcp.user.activity through tool, resource, and prompt primitives and the withResourceObservation wrapper is well-structured. One functional correctness issue stands out in the new resource observation layer, and a leftover dead variable was introduced in the span-attribute helper.
2 issues detected:
🐞 Bug - `readResource` converts all thrown errors into a returned value, making the `catch` block in `withResourceObservation` unreachable and causing every failure to be counted as a successful invocation.
Details: readResource catches every thrown error internally and returns a ReadResourceResult error payload — it never re-throws. Because of this, the catch (error) block in withResourceObservation is dead code for all four registered resources. Any underlying API failure will be observed with outcome: "success" and the hardcoded isError: false, silently hiding failures in the mcp.tool.outcomes and activity metrics.
File: packages/core/src/resources/hevy.ts (68-88)
🧹 Maintainability - `isPrompt` is assigned but never read; it is leftover from the refactor that introduced the three-way `kind` conditional. 🛠️
Details: isPrompt is declared at line 70 but is never referenced anywhere in createAttributes. The three-way inline conditional at lines 77-81 replaced the old isPrompt usage directly, leaving the assignment as unreferenced dead code that may produce a lint warning and misleads future readers.
File: packages/node/src/utils/tool-observer.ts (69-70)
🛠️ 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
| const kind = invocation.kind ?? "tool"; | ||
| const isPrompt = kind === "prompt"; |
There was a problem hiding this comment.
🧹 Maintainability - Dead Variable: Remove the const isPrompt = kind === "prompt"; line entirely.
| const kind = invocation.kind ?? "tool"; | |
| const isPrompt = kind === "prompt"; | |
| const kind = invocation.kind ?? "tool"; |
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 75 suites 22s ⏱️ Results for commit e4f78d7. ♻️ 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: 6
🤖 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/telemetry-dashboards.md`:
- Around line 17-21: Update the DAU question in the telemetry dashboard table to
describe the 1d query window as the rolling “last 24 hours” rather than “today”;
keep the existing source and query window unchanged.
In `@docs/telemetry-data-dictionary.md`:
- Around line 74-79: The telemetry documentation must consistently define
user_hash handling and retention. Update the metric privacy statement to exempt
only mcp.user.activity, then revise the activity-counter retention text to
explicitly specify 30-day retention and link to the exact 30-day trace/user-hash
policy section in telemetry-dashboards.md; retain 90-day retention for other
aggregate metrics.
In `@packages/core/src/resources/hevy.ts`:
- Around line 52-90: Update withResourceObservation and readResource so
retrieval failures remain identifiable after conversion to
createResourceErrorResult: return or propagate a discriminated error result,
then have withResourceObservation finish with outcome "thrown_error" and isError
true for that path while preserving the MCP error response. Add coverage
verifying a rejected resource retrieval produces the error response and error
telemetry.
In `@packages/node/src/utils/metrics.ts`:
- Around line 16-19: Add and stage a non-empty Changeset entry for the package
containing packages/node/src/utils/metrics.ts, documenting the runtime-visible
telemetry change introduced by activityInvocations. Use the repository’s
existing Changeset format and select the appropriate release impact for that
package.
In `@packages/node/src/utils/tool-observer.ts`:
- Around line 69-82: The resource error paths currently use the tool name
attribute instead of the resource-specific attribute. In the observer
implementation, reuse one kind-specific name-attribute mapping for the main span
attributes and all failure/exception handling paths around the resource error
sites, including the logic near lines 318, 354, and 368; add a test verifying
resource failures record mcp.resource.name rather than mcp.tool.name.
- Around line 256-264: Update the observation flow around
recordMcpToolInvocation to compute activityKind before that call and identify
resource operations. Skip all legacy tool metrics—invocation, outcome, error,
and duration—for resource activity, while continuing to emit mcp.user.activity
for every activity kind.
🪄 Autofix
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: a598abcb-6019-468a-b8fc-b85e9a942de8
📒 Files selected for processing (10)
docs/telemetry-dashboards.mddocs/telemetry-data-dictionary.mdpackages/core/src/observation.tspackages/core/src/resources/hevy.test.tspackages/core/src/resources/hevy.tspackages/core/src/server.tspackages/node/src/utils/metrics.tspackages/node/src/utils/telemetry.tspackages/node/src/utils/tool-observer.test.tspackages/node/src/utils/tool-observer.ts
| | Panel | Source | Query window | Question answered | | ||
| | -------------------------- | ------------------- | ------------ | --------------------------------------------------------------- | | ||
| | Daily active users (DAU) | `mcp.user.activity` | `1d` | How many unique users called a tool, resource, or prompt today? | | ||
| | Weekly active users (WAU) | `mcp.user.activity` | `7d` | How many unique users were active in the last week? | | ||
| | Monthly active users (MAU) | `mcp.user.activity` | `30d` | How many unique users were active in the last month? | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the 1d window as rolling.
The query uses [1d], which measures the previous 24 hours. The table wording “today” implies a calendar-day value. Replace it with “last 24 hours” or document a calendar-day query.
🤖 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 `@docs/telemetry-dashboards.md` around lines 17 - 21, Update the DAU question
in the telemetry dashboard table to describe the 1d query window as the rolling
“last 24 hours” rather than “today”; keep the existing source and query window
unchanged.
| The pseudonymous user hash is also emitted on the dedicated user-activity | ||
| counter solely to calculate unique DAU, WAU, and MAU. It is not emitted on | ||
| request-volume, outcome, duration, API, or session metrics. Activity panels must | ||
| aggregate by user hash and must not expose per-user behavior histories or saved | ||
| per-user views. The activity counter is subject to the 30-day trace/user-hash | ||
| retention policy below. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Define one retention rule for user_hash metrics.
Line 58 says that metrics never contain a user hash, but this change adds user_hash to mcp.user.activity. Update that statement to exempt only the dedicated activity metric.
Line 78 references a 30-day policy “below”, but this file has no retention section. docs/telemetry-dashboards.md Lines 96-102 specify 90-day retention for aggregate metrics and 30-day retention only for traces containing user_hash. State explicitly that activity metric series containing user_hash use 30-day retention, and link the exact policy section.
🤖 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 `@docs/telemetry-data-dictionary.md` around lines 74 - 79, The telemetry
documentation must consistently define user_hash handling and retention. Update
the metric privacy statement to exempt only mcp.user.activity, then revise the
activity-counter retention text to explicitly specify 30-day retention and link
to the exact 30-day trace/user-hash policy section in telemetry-dashboards.md;
retain 90-day retention for other aggregate metrics.
| function withResourceObservation( | ||
| name: string, | ||
| observer: ToolObserver | undefined, | ||
| handler: (uri: URL, context: ServerContext) => Promise<ReadResourceResult>, | ||
| ): (uri: URL, context: ServerContext) => Promise<ReadResourceResult> { | ||
| return async (uri, context) => { | ||
| const startedAt = Date.now(); | ||
| let scope; | ||
| try { | ||
| scope = memoizeObservationScope( | ||
| observer?.start({ name, kind: "resource" }), | ||
| ); | ||
| } catch { | ||
| scope = undefined; | ||
| } | ||
|
|
||
| try { | ||
| const result = await (scope | ||
| ? scope.run(() => handler(uri, context)) | ||
| : handler(uri, context)); | ||
| void scope?.finish({ | ||
| outcome: "success", | ||
| durationMs: Date.now() - startedAt, | ||
| result: { | ||
| isError: false, | ||
| hasStructuredContent: false, | ||
| contentCountBucket: bucketCount(result.contents.length), | ||
| }, | ||
| }); | ||
| return result; | ||
| } catch (error) { | ||
| void scope?.finish({ | ||
| outcome: "thrown_error", | ||
| durationMs: Date.now() - startedAt, | ||
| }); | ||
| throw error; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record handled resource failures as failures.
readResource catches retrieval errors and returns createResourceErrorResult at Lines 45-49. Line 72 then records that returned error result as outcome: "success" and Line 76 hard-codes isError: false. API failures therefore increment successful outcomes and do not enter error telemetry.
Move resource-error conversion into this wrapper, or return a discriminated result from readResource. Preserve the MCP error response, but finish the observation with an error outcome. Add coverage for a rejected resource retrieval.
🤖 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/resources/hevy.ts` around lines 52 - 90, Update
withResourceObservation and readResource so retrieval failures remain
identifiable after conversion to createResourceErrorResult: return or propagate
a discriminated error result, then have withResourceObservation finish with
outcome "thrown_error" and isError true for that path while preserving the MCP
error response. Add coverage verifying a rejected resource retrieval produces
the error response and error telemetry.
| /** User activity across tool, resource, and prompt calls. */ | ||
| export const activityInvocations = meter.createCounter("mcp.user.activity", { | ||
| description: "MCP user activity by primitive kind", | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
changesets="$(git diff --name-only origin/main...HEAD -- .changeset)"
if [ -z "$changesets" ]; then
echo "No Changeset was added for this package-source change."
exit 1
fi
printf '%s\n' "$changesets"Repository: chrisdoc/hevy-mcp
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'changed packages/node/src/utils/metrics.ts?\n'
git diff --name-only origin/main...HEAD -- packages/node/src/utils/metrics.ts || true
printf '\nnew/changed .changeset files vs origin/main?\n'
git diff --name-status origin/main...HEAD -- .changeset || true
printf '\nall .changeset files currently tracked?\n'
git ls-files '.changeset/*' || trueRepository: chrisdoc/hevy-mcp
Length of output: 562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'All .changeset entries:\n'
for f in .changeset/*.md; do
echo "--- $f"
sed -n '1,240p' "$f"
doneRepository: chrisdoc/hevy-mcp
Length of output: 1236
Add a non-empty Changeset for this package change.
packages/node/src/utils/metrics.ts adds runtime-visible telemetry behavior, but the current changeset contents do not cover it. Add and stage an appropriate non-empty .changeset/*.md entry before merge.
🤖 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/metrics.ts` around lines 16 - 19, Add and stage a
non-empty Changeset entry for the package containing
packages/node/src/utils/metrics.ts, documenting the runtime-visible telemetry
change introduced by activityInvocations. Use the repository’s existing
Changeset format and select the appropriate release impact for that package.
Source: Coding guidelines
| const kind = invocation.kind ?? "tool"; | ||
| const isPrompt = kind === "prompt"; | ||
| const sessionId = getCurrentMcpSessionId(); | ||
| const attributes: Record<string, AttributeValue> = { | ||
| "mcp.span.category": DISCOVERY_TOOL_NAMES.has(invocation.name) | ||
| ? "discovery" | ||
| : "tool", | ||
| [isPrompt ? "mcp.prompt.name" : "mcp.tool.name"]: invocation.name, | ||
| "mcp.operation.kind": invocation.kind ?? "tool", | ||
| "mcp.span.category": | ||
| kind === "tool" && DISCOVERY_TOOL_NAMES.has(invocation.name) | ||
| ? "discovery" | ||
| : kind, | ||
| [kind === "prompt" | ||
| ? "mcp.prompt.name" | ||
| : kind === "resource" | ||
| ? "mcp.resource.name" | ||
| : "mcp.tool.name"]: invocation.name, | ||
| "mcp.operation.kind": kind, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use resource-specific attributes for resource errors.
Lines 77-81 write mcp.resource.name for resource spans. Resource failures still write mcp.tool.name at Line 318, Line 354, and Line 368. Resource error diagnostics will therefore appear as tool operations.
Use one kind-specific name-attribute mapping for span attributes, failure events, and exception attributes. Add a resource failure test.
🤖 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 69 - 82, The resource
error paths currently use the tool name attribute instead of the
resource-specific attribute. In the observer implementation, reuse one
kind-specific name-attribute mapping for the main span attributes and all
failure/exception handling paths around the resource error sites, including the
logic near lines 318, 354, and 368; add a test verifying resource failures
record mcp.resource.name rather than mcp.tool.name.
| const userHash = getTelemetryUserHash(); | ||
| const activityKind = invocation.kind ?? "tool"; | ||
| const activityMetrics = userHash | ||
| ? { activity_kind: activityKind, user_hash: userHash } | ||
| : undefined; | ||
| bestEffort(() => toolInvocations.add(1, metrics)); | ||
| if (activityMetrics) { | ||
| bestEffort(() => activityInvocations.add(1, activityMetrics)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep resource reads out of legacy tool metrics.
Line 254 calls recordMcpToolInvocation, and Line 261 increments mcp.tool.invocations for every observation scope. Resource handlers now create these scopes. The existing tool invocation, outcome, error, and duration metrics will include resource reads, which changes the existing request-volume and error panels.
Compute activityKind before Line 254. Skip legacy tool* metric emission for resource operations. Continue to emit mcp.user.activity for all activity kinds.
🤖 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 256 - 264, Update the
observation flow around recordMcpToolInvocation to compute activityKind before
that call and identify resource operations. Skip all legacy tool
metrics—invocation, outcome, error, and duration—for resource activity, while
continuing to emit mcp.user.activity for every activity kind.
Code Review by Qodo
1. Resources counted in mcp.tool.*
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| try { | ||
| scope = memoizeObservationScope( | ||
| observer?.start({ name, kind: "resource" }), | ||
| ); |
There was a problem hiding this comment.
1. Resources counted in mcp.tool.* 📎 Requirement gap ≡ Correctness
registerHevyResources() now routes resource reads through the shared ToolObserver, but the Node observer records every observed invocation into the existing mcp.tool.invocations/outcome/duration/error metrics and increments session tool-call counts regardless of invocation kind. This will mix resource traffic into tool reliability/session-shape metrics and change request-volume and error dashboard semantics, violating the requirement that those dashboards remain unchanged.
Agent Prompt
## Issue description
Resource reads are now observed via the shared `ToolObserver` (using `kind: "resource"`), but `createNodeToolObserver()` currently records *all* observed invocations into the existing `mcp.tool.*` metrics (and session tool-call counts) regardless of `invocation.kind`. This pollutes tool reliability/session-shape metrics with resource activity and changes request-volume/error dashboard semantics, which must remain unchanged.
## Issue Context
The PR wires resource reads into the observer (e.g., `registerHevyResources(server, runtime, options.observer)` and resource reads calling `observer.start({ name, kind: "resource" })`) to support activity-only DAU/WAU/MAU. However, Node’s observer treats every invocation as a tool for metrics/session accounting by always calling `recordMcpToolInvocation()` and emitting `toolInvocations/toolOutcomes/toolDuration/toolErrors`, so resource observations will be counted as tools and will also increment `session.toolCalls`.
## Fix Focus Areas
- packages/core/src/resources/hevy.ts[52-89]
- packages/core/src/server.ts[71-75]
- packages/node/src/utils/tool-observer.ts[249-265]
- packages/node/src/utils/tool-observer.ts[252-294]
- packages/node/src/utils/tool-observer.ts[401-419]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| outcome: "success", | ||
| durationMs: Date.now() - startedAt, | ||
| result: { | ||
| isError: false, |
There was a problem hiding this comment.
2. Resource errors marked success 🐞 Bug ◔ Observability
withResourceObservation always finishes with outcome: "success" and result.isError: false, even when the resource read fails. Because readResource converts thrown exceptions into an error payload, the observation wrapper never sees an exception and misclassifies failures as successes.
Agent Prompt
## Issue description
Resource handlers wrap failures into a returned `ReadResourceResult` via `readResource(...)`, but `withResourceObservation(...)` treats any returned result as success and sets `isError: false`. This makes resource failure telemetry indistinguishable from success.
## Issue Context
- `readResource` catches exceptions and returns `createResourceErrorResult(uri, error)`.
- `withResourceObservation` only reports errors if the handler throws; it does not detect returned error payloads.
## Fix Focus Areas
- packages/core/src/resources/hevy.ts[41-49]
- packages/core/src/resources/hevy.ts[68-88]
## Suggested fix
Option A (recommended): Let `withResourceObservation` own error conversion.
1. Remove the inner `readResource(...)` try/catch usage from each registered resource handler.
2. In `withResourceObservation`, wrap `handler(uri, context)` in `try/catch`.
3. On success: `finish({ outcome: "success", ... , result: { isError: false, ... } })`.
4. On caught error: call `finish({ outcome: "returned_error" /* or thrown_error */, durationMs, result: { isError: true, ... } })`, then `return createResourceErrorResult(uri, error)` (do not rethrow).
Option B: If you keep `readResource`, add a reliable error signal to its return value so `withResourceObservation` can detect and report `returned_error` + `isError: true` when it returns an error payload.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Bundle ReportChanges will increase total bundle size by 1.59kB (0.62%) ⬆️. 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 #918 +/- ##
==========================================
- Coverage 83.28% 83.21% -0.07%
==========================================
Files 83 83
Lines 5640 5659 +19
Branches 1577 1582 +5
==========================================
+ Hits 4697 4709 +12
- Misses 435 439 +4
- Partials 508 511 +3 ☔ View full report in Codecov by Harness. |
|
Charlie could not safely repair the merge conflict. I re-fetched PR #918 at head I stopped without pushing because choosing whether to restore that identity path or redesign active-user counting is a product/security decision, not mechanical conflict resolution. Please reconcile the telemetry/privacy contract, then update this PR branch. No branch update was made. — charlied/pr-merge-conflict-repair |
1 participant
Primary changes
mcp.user.activitywith boundedtool,resource, andpromptactivity kindsuser_hashonly to this dedicated active-user metricReviewer walkthrough
resourceactivity and wires resource registrations through the existing privacy-safe observer.Correctness and invariants
tool,resource, andprompt; lifecycle, discovery/list, API child, cache, and error-only spans do not count.user_hashvalues over rolling1d,7d, and30dwindows, withuser_hashattached only to the dedicated active-user metric.Testing and QA
npm run check:typesgit diff --check✨ PR Description
Purpose: Add user activity metrics tracking across tools, resources, and prompts to calculate DAU/WAU/MAU while preserving privacy through pseudonymous user hashing.
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
Resolves #917