Skip to content

feat(mcp): add Braintrust tracing for the official MCP Go SDK - #219

Closed
Atharva Mhaske (atharvamhaske) wants to merge 7 commits into
braintrustdata:mainfrom
atharvamhaske:feat/mcp-tracing-integration
Closed

Atharva Mhaske (atharvamhaske) wants to merge 7 commits into
braintrustdata:mainfrom
atharvamhaske:feat/mcp-tracing-integration

Conversation

@atharvamhaske

Copy link
Copy Markdown
Contributor

Summary

Adds a new trace/contrib/mcp integration for
github.com/modelcontextprotocol/go-sdk (v1.7.0),
covering the MCP tool execution surface that agents use during a run.

  • InstrumentClient — traces ClientSession.ListTools and ClientSession.CallTool, including in-flight notifications/progress during tool calls
  • InstrumentServer — traces incoming tools/list and tools/call, with a separate handler child span (mcp.tools.handler [name]) under the RPC span
  • Orchestrion — auto-instruments mcp.NewClient / mcp.NewServer
  • Examplesexamples/mcp and examples/internal/mcp (in-memory transport; no provider API keys)
  • Tests — in-memory JSON-RPC (no VCR/API keys); orchestrion coverage via TestMCP

Why

The official MCP Go SDK had no Braintrust instrumentation. MCP is the standard protocol for agent tool discovery and execution; this matches existing patterns (e.g. ADK, LangChainGo) as a dedicated trace/contrib module.

Test plan

  • go test -C trace/contrib/mcp ./...
  • make ci (lint, mod-verify, test, build)
  • CI green on PR
    No new VCR cassettes or provider API keys required for MCP tests.

Notes

  • Client spans set metadata.api to ClientSession.CallTool / ClientSession.ListTools
  • Progress notifications are recorded as span events and in braintrust.output_json.progress
  • Registered in scripts/nested_modules.txt, go.work, and trace/contrib/all
  • I haven't check command make examples as i don't have braintrust api key to do so(will have to check before merging)

Closes #190

Instrument client and server middleware for tools/list and tools/call so agent tool execution is visible in Braintrust traces.
Trace CallTool progress notifications, split server handler spans, add server ListTools and orchestrion tests, and drop out-of-scope README/options metadata.
@AbhiPrasad

Copy link
Copy Markdown
Member

please try this out in braintrust and share a screenshot of the braintrust trace! I will review after that.

@atharvamhaske

Copy link
Copy Markdown
Contributor Author

please try this out in braintrust and share a screenshot of the braintrust trace! I will review after that.

sure but do i need api key or is it there included in free accounts?

@AbhiPrasad

Copy link
Copy Markdown
Member

you should be able to use a free braintrust account! Try out https://www.braintrust.dev/docs/tracing-quickstart

Signed-off-by: atharvamhaske <atharvamhaske76@gmail.com>
@atharvamhaske

Atharva Mhaske (atharvamhaske) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

you should be able to use a free braintrust account! Try out https://www.braintrust.dev/docs/tracing-quickstart

Tried this out with a real Braintrust trace like you asked. Found a bug while checking it: client spans (ListTools/CallTool) and server spans (tools/list, tools/call, handler) were landing in separate traces with no shared root. Turns out the MCP transport wasn't passing trace context from client to server, so the server side just started its own fresh trace instead of continuing the client's.

Fixed it by injecting a W3C traceparent into the request's _meta field on the client side, and reading it back out on the server side. Used our own propagation.TraceContext{} instead of the global otel propagator, since that one defaults to a no-op unless an app sets it up separately.

Added a test that checks the client span, server span, and handler span all share the same trace ID, and that the server span is a child of the client's call span.

Verified against a real trace, screenshots below in comment. Everything now nests correctly under one trace.

cc: Abhijeet Prasad (@AbhiPrasad)

@atharvamhaske

Copy link
Copy Markdown
Contributor Author

Screenshot 1

Screenshot 2026-09-10 at 8 46 00 PM

Screenshot 2

Screenshot 2026-09-10 at 8 50 22 PM

Now it works as expected, if anything doesn't work on your end feel free to drop me, happy to fix it.

…tegration

# Conflicts:
#	trace/contrib/all/go.mod
- Bump expected aspect count in `TestGenerateExcludesAllDirectory` from 18 to
  20, accounting for the 2 aspects `trace/contrib/mcp/orchestrion.yml` adds
  (`mcp-newclient-instrument`, `mcp-newserver-instrument`)
- Add `trace/contrib/mcp` to the fixture `replace` directive map in
  `rewriteFixtureReplaceDirectives`, fixing `TestOrchestrionInjection`
  failing to resolve `../mcp` inside the temp fixture module

Signed-off-by: atharvamhaske <atharvamhaske76@gmail.com>
Signed-off-by: atharvamhaske <atharvamhaske76@gmail.com>
@atharvamhaske

Copy link
Copy Markdown
Contributor Author

Abhijeet Prasad (@AbhiPrasad) previous ci failed due to missing imports(i have fixed it), can it be triggered again please?

@atharvamhaske

Copy link
Copy Markdown
Contributor Author

small suggestion about CI. right now every push on a fork PR needs manual approval to run.

as now i have one merged PR already (#225). gitHub has a setting that can fix this: change "require approval for all outside collaborators" to "require approval for first-time contributors".
with this setting, a known contributor does not need approval after the first PR. New contributors still need approval, so security stays the same.

this can save review time on future PRs. we can have it cuz it didnt flag my ci failure at first point when i pushed and it did when you ran the workflow, so i could catch it early.

just a suggestion, happy either ways with repo rules :)

@atharvamhaske

Atharva Mhaske (atharvamhaske) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Abhijeet Prasad (Abhijeet Prasad (@AbhiPrasad)) previous ci failed due to missing imports(i have fixed it), can it be triggered again please?

re run of ci needed Abhijeet Prasad (@AbhiPrasad)

@atharvamhaske

Copy link
Copy Markdown
Contributor Author

Abhijeet Prasad (@AbhiPrasad) can you ptal !!

@AbhiPrasad

Copy link
Copy Markdown
Member

I think this might take some time to merge, because we aren’t sure if we want to support this in the near term. We don’t have mcp instrumentation in any of the other sdks yet.

@atharvamhaske

Copy link
Copy Markdown
Contributor Author

I think this might take some time to merge, because we aren’t sure if we want to support this in the near term. We don’t have mcp instrumentation in any of the other sdks yet.

sure no issues

@AbhiPrasad

Copy link
Copy Markdown
Member

Sorry for the trouble Atharva Mhaske (@atharvamhaske) but I'm going to close this. I don't think we want to pursue mcp instrumentation at the current moment. but we might revisit in a couple of months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bot] Instrument MCP Go SDK (87,149,707 weekly downloads)

2 participants