feat(analytics): AUTO-265 emit MCP usage events to Amplitude via @amplitude/mcp-analytics - #206
feat(analytics): AUTO-265 emit MCP usage events to Amplitude via @amplitude/mcp-analytics#206xsvfat wants to merge 9 commits into
Conversation
Co-Authored-By: Sean Yang <sean@browserless.io>
Co-Authored-By: Sean Yang <sean@browserless.io>
Co-Authored-By: Sean Yang <sean@browserless.io>
Co-Authored-By: Sean Yang <sean@browserless.io>
Original prompt from Sean Yang
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Sean Yang <sean@browserless.io>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 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 Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR adds optional Amplitude MCP analytics configuration, propagates authenticated account IDs, instruments MCP servers and tools, sets tool identity and rationale context, handles shutdown signals, and adds analytics tests and documentation. ChangesAmplitude MCP analytics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant FastMCP
participant Server
participant AmplitudeMCPAnalytics
MCPClient->>FastMCP: connect
FastMCP->>Server: create MCP session
Server->>AmplitudeMCPAnalytics: instrument on first connect
FastMCP->>AmplitudeMCPAnalytics: wrap tool execution
AmplitudeMCPAnalytics-->>MCPClient: return instrumented tool result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
devin-ai-integration
Bot
changed the title
devin-ai-integration
Bot
changed the title
![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
🤖 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 `@llms-install.md`:
- Around line 48-49: Remove AMPLITUDE_API_KEY from the main copy-paste
configuration in llms-install.md, leaving only the required settings and
BROWSERLESS_TOKEN. Add a separate opt-in note stating that users should add
AMPLITUDE_API_KEY only when they have a real project API key.
In `@src/lib/amplitude-analytics.ts`:
- Around line 106-109: Update getAmplitudeIdentity so the token fallback uses a
collision-resistant cryptographic digest or keyed HMAC instead of the 32-bit
djb2 hash, while preserving session.accountId as the preferred identity and the
existing token-prefixed format.
🪄 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
Run ID: 75e8b883-05ff-4c71-bd92-8d855cb2d1eb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
README.mdllms-install.mdpackage.jsonsrc/@types/types.d.tssrc/config.tssrc/index.tssrc/lib/account-resolver.tssrc/lib/amplitude-analytics.tssrc/lib/define-tool.tssrc/lib/http-auth.tstest/lib/amplitude-analytics.spec.ts
Co-Authored-By: Sean Yang <sean@browserless.io>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/lib/amplitude-analytics.spec.ts (2)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert a fixed identity value instead of duplicating the implementation.
The test recomputes the same hash expression as production, so an algorithm or encoding regression can be changed in both places without detection. Use a known token fixture with a hard-coded expected
token-*value.Also applies to: 40-41
🤖 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 `@test/lib/amplitude-analytics.spec.ts` at line 2, Update the tests using createHash to stop recomputing the production hash; replace that logic with a known token fixture and a hard-coded expected token-* identity value. Remove the now-unused createHash import while preserving assertions for the generated identity.
75-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd shutdown coverage.
The PR adds shutdown flushing, but the suite does not verify that
shutdownAmplitudeAnalyticsawaits successful shutdown or swallows shutdown failures. Add both cases to protect the process-termination 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 `@test/lib/amplitude-analytics.spec.ts` around lines 75 - 119, Extend the amplitude analytics tests around the existing instrumentation coverage to test shutdownAmplitudeAnalytics: verify it awaits a successful analytics shutdown, and separately verify it resolves without propagating an error when shutdown fails. Use the existing mock analytics setup and assert the shutdown method is invoked in both cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/lib/amplitude-analytics.spec.ts`:
- Line 2: Update the tests using createHash to stop recomputing the production
hash; replace that logic with a known token fixture and a hard-coded expected
token-* identity value. Remove the now-unused createHash import while preserving
assertions for the generated identity.
- Around line 75-119: Extend the amplitude analytics tests around the existing
instrumentation coverage to test shutdownAmplitudeAnalytics: verify it awaits a
successful analytics shutdown, and separately verify it resolves without
propagating an error when shutdown fails. Use the existing mock analytics setup
and assert the shutdown method is invoked in both cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5cd06d6-1565-43e4-ae32-5fdc8a1ef2c6
📒 Files selected for processing (3)
llms-install.mdsrc/lib/amplitude-analytics.tstest/lib/amplitude-analytics.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- llms-install.md
Co-Authored-By: Sean Yang <sean@browserless.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/amplitude-analytics.spec.ts`:
- Around line 119-143: Update the “awaits successful Amplitude shutdown” and
“swallows rejected Amplitude shutdown” tests to use deferred promises instead of
immediately settled Sinon stubs. Assert that shutdownAmplitudeAnalytics remains
pending before resolving or rejecting the deferred promise, then settle it and
verify the wrapper completes without throwing and shutdown is called once.
🪄 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
Run ID: 6caee05f-b142-4e2d-9dda-8c2e51928816
📒 Files selected for processing (1)
test/lib/amplitude-analytics.spec.ts
Co-Authored-By: Sean Yang <sean@browserless.io>
ashwinsingh2007
left a comment
ashwinsingh2007
left a comment
There was a problem hiding this comment.
Few comments from claude code -
-
shutdown() awaits the Amplitude flush with no timeout before process.exit(). A hung flush means the process never exits — worse than the old default termination and the opposite of the stated "exit fast on docker stop" goal. Race the flush against a short timeout, then exit regardless.
-
getCurrentContext() is called on every tool invocation, including when Amplitude is disabled (the default). The guard assumes it returns falsy outside an instrumented frame; if the SDK throws there, this line is outside the try/catch and every tool call breaks with analytics off. Confirm the SDK contract + add a disabled-mode test that actually runs a tool's execute.
-
accountId is threaded via hybridAuthenticate, which only runs for transport === 'httpStream'. The ticket cites "OAuth/Supabase sessions" — when auth: oauthProvider is active, does authenticate still run and set session.accountId? If not, OAuth sessions silently fall back to token- and the "verified account id" goal isn't met.
Rest looks fine
ashwinsingh2007
left a comment
ashwinsingh2007
left a comment
There was a problem hiding this comment.
Need to check the comment
Co-Authored-By: Sean Yang <sean@browserless.io>
|
@ashwinsingh2007 thanks — 1 and 2 were real, fixed in 86206d0. 3 checks out as-is. 1. Unbounded flush. Correct, a hung flush would have hung the process. 2. 3. OAuth sessions and 553 tests passing, CI green. |
2 participants
Summary
[MCP] …event set (Session Initialized/Ended,Tools Listed,Tool Call Response/Rejected) instead of only our customMCP Tool Requestevents.AMPLITUDE_API_KEYis set.ANALYTICS_ENABLED(SQS) is untouched.The interesting part is that the SDK is written for the raw
@modelcontextprotocol/sdkMcpServerand we run FastMCP, which gives us neither of the two hook points the SDK needs:1.
instrumentServer()must run beforeconnect(), and FastMCP owns session creation. FastMCP builds aFastMCPSessionper client connection and connects it internally;session.server(the low-level SDKServer) is only reachable fromserver.on('connect')— i.e. after the transport is up, at which point binding silently no-ops and every tool wrapper stays a passthrough. So we hook the class instead of the instance:The guard exists because
instrumentServercapturesinstance.connect.bind(instance)as "the original" — which resolves to our prototype wrapper. Without it the second entry recurses forever.This requires the SDK class we patch to be the same class fastmcp instantiates, so
@modelcontextprotocol/sdkbecomes a direct dependency pinned to fastmcp's own range (^1.24.3, deduped to a single1.30.0copy). Aconnect-timeinstanceofcheck warns once if a future dedupe break silently unbinds instrumentation.2. Tool wrapping. Rather than threading the client through
registerSurfaceand all ten tool modules,addToolis wrapped once so every registered tool is instrumented at the FastMCP boundary anddefineTool/tool modules are untouched:Inside the handler, identity and rationale use the SDK's context-scoped free functions, gated on
getCurrentContext()(they throw outside an instrumented frame, i.e. when Amplitude is disabled).user_idis the verifiedaccountIdfor OAuth/Supabase sessions — now threaded onto the session byresolveBrowserlessAuth— andtoken-<djb2 hash>otherwise; the raw Browserless token is never sent. The model-reported_prompt(alreadyredactSecrets-ed) becomes[MCP] Rationale.Note
instrumentToolreceives FastMCP's context object rather than the SDK'sRequestHandlerExtra; the SDK degrades gracefully on it (it readssessionId, which FastMCP provides), so nothing is faked.Because a SIGTERM/SIGINT listener suppresses Node's default termination, the shutdown handler awaits the flush and then exits explicitly (143/130) — otherwise
docker stopwould wait out the full grace period on every deploy.Related issues
n/a
Changes
src/lib/amplitude-analytics.ts(new): client construction, theServer.prototype.connectpre-connect hook, theaddToolwrapper, identity resolution, shutdown flush.src/index.ts: init + instrumentation + signal handling + theinstanceof Serversanity warning.src/lib/define-tool.ts:setIdentity/setRationaleinside the tool frame (18 added lines, no restructuring).src/lib/{http-auth,account-resolver}.ts: thread the verifiedaccountIdontoBrowserlessSession.@amplitude/mcp-analytics@^0.3.0(0.4.0 skipped — 3 days old), its peer@amplitude/analytics-node, and the direct@modelcontextprotocol/sdkpin described above.README.md/llms-install.md: documentAMPLITUDE_API_KEY.A follow-up PR on
browserless/browserless-workspace-templatewiresAMPLITUDE_API_KEYinto the local stack (empty by default, so the local MCP stays silent).Test plan
npm testpasses locally (549 passing, incl. new specs: disabled without a key, account-id vs hashed-token identity, connect hook binds without recursing,addToolwrapper enabled/disabled)npm run lintpasses locallynpm run coveragethresholds still metChecklist
Link to Devin session: https://app.devin.ai/sessions/7b9256f2b329451cb84eb1bfcd962ca1
Requested by: @xsvfat
Summary by CodeRabbit
New Features
AMPLITUDE_API_KEY.accountId, with a deterministic fallback when absent.SIGTERM/SIGINT(once per process).Documentation
AMPLITUDE_API_KEY.Tests