Skip to content

Conversation

@jboolean
Copy link
Contributor

@jboolean jboolean commented Nov 5, 2025

What does this PR do?

Adds contrib library for mcp-go. Uses it to record LLMObs Tool spans for tool calls.

This is the base of a Graphite stack.

image.png

Motivation

Tech spec for this project

Closes MLOB-4372

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Testing

In addition to automated tests, this was tested locally with Datadog MCP Server

Unsure? Have a question? Request a review!

Copy link
Contributor Author

jboolean commented Nov 5, 2025

@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Nov 5, 2025
@pr-commenter
Copy link

pr-commenter bot commented Nov 5, 2025

Benchmarks

Benchmark execution time: 2025-11-13 19:07:27

Comparing candidate commit 38e2a00 in PR branch jb/contrib-mcp-go-base with baseline commit 2b60c19 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 metrics, 0 unstable metrics.

@jboolean jboolean changed the title Initial mcp-go tracer implementation Initial mcp-go tracer implementation Nov 5, 2025
@datadog-official
Copy link
Contributor

datadog-official bot commented Nov 5, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 38e2a00 | Docs | Datadog PR Page | Was this helpful? Give us feedback!

// Hooks provides Datadog tracing for MCP servers.
type Hooks struct {
// Stores data between start and end of tool calls. In-memory works because it must be on the same instance.
toolCache *ttlcache.Cache[any, *llmobs.ToolSpan]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The in-memory cache just stores data between the start and end hooks. Creating a middleware would have avoided this, but would have been harder to automatically add to all tools. We'll also use hooks for session initialization spans (another PR on this stack).

Unfortunately this cache implementation does need to be shut down manually.

I experimented with but decided against cache customization options (max size, ttl).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another downside I see from this hooks option is that it does not allow to pass down the newly created context with the span to the handler, so if the user starts spans in the handler they won't be children of the spans we are creating here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, especially since users may want to add tags to the existing span in the handler. Just know that there will be asymmetry/inconsistency between tools and initialization because I don't see any initialization middleware.

@jboolean jboolean changed the title Initial mcp-go tracer implementation feat(contrib/mcp-go): Initial mcp-go tracer implementation Nov 5, 2025
@jboolean jboolean requested a review from a team November 5, 2025 21:15
@jboolean jboolean requested a review from tomshen November 5, 2025 21:16
@jboolean jboolean requested a review from rarguelloF November 5, 2025 21:16
@jboolean jboolean marked this pull request as ready for review November 5, 2025 21:18
@jboolean jboolean requested review from a team as code owners November 5, 2025 21:18
@tomshen tomshen removed their assignment Nov 6, 2025
@tomshen tomshen removed their request for review November 6, 2025 15:35
Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looking good! just a few nits / comments


// onBeforeCallTool is called before a tool is executed.
func (h *Hooks) onBeforeCallTool(ctx context.Context, id any, request *mcp.CallToolRequest) {
toolSpan, _ := llmobs.StartToolSpan(ctx, request.Params.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if other tracers are providing any kind of info about these spans coming from the contrib package (in APM we would set tag component: "mark3labs/mcp-go").

Since this call also creates an APM span, we should do it at least for the APM part (we might need to change the llmobs package a little bit or add a new option).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sabrenner says that's just for APM spans. But LLMObs does set an integration tag so I'll add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, @rarguelloF I see integration as a private field on the Span struct but no actual way to set it. I'll add that to llmobs.

Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looking good! just a few nits / comments

@jboolean jboolean force-pushed the jb/contrib-mcp-go-base branch from 95af4d3 to 4def1e6 Compare November 7, 2025 19:01
@jboolean jboolean requested a review from rarguelloF November 7, 2025 19:02
Copy link
Member

@genesor genesor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, two nits

"DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED": {},
"DD_TRACE_ABANDONED_SPAN_TIMEOUT": {},
"DD_TRACE_AGENT_PORT": {},
"DD_TRACE_V1_PAYLOAD_FORMAT_ENABLED": {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also not get deleted (see below)! probably a rebase issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now reset these files to main and rebased the branch and the tests are failing. The tests tell me to run go run ./scripts/configinverter/main.go generate and when I do it deletes this line.

See here: https://github.com/DataDog/dd-trace-go/runs/55310947575

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put the changes back in order to pass tests.

It's not deleted, it's just moved below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch @jboolean I think the env var was actually manually added to both files without thinking this would break some stuff down the road.

Copy link

@sabrenner sabrenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mainly looked at mcpgo.go and double-checked the sdk changes to add the integration tag, looks good to me content-wise!

Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor Author

@jboolean jboolean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/merge

@jboolean jboolean requested a review from hannahkm November 13, 2025 16:58
@jboolean
Copy link
Contributor Author

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Nov 13, 2025

View all feedbacks in Devflow UI.

2025-11-13 17:06:21 UTC ℹ️ Start processing command /merge


2025-11-13 17:06:37 UTC ℹ️ MergeQueue: waiting for PR to be ready

This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2025-11-13 19:19:19 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 18m (p90).


2025-11-13 19:39:29 UTC ℹ️ MergeQueue: This merge request was merged

@jboolean jboolean force-pushed the jb/contrib-mcp-go-base branch from c177b06 to 38e2a00 Compare November 13, 2025 18:32
@dd-mergequeue dd-mergequeue bot merged commit 4be1146 into main Nov 13, 2025
268 of 269 checks passed
@dd-mergequeue dd-mergequeue bot deleted the jb/contrib-mcp-go-base branch November 13, 2025 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apm:ecosystem contrib/* related feature requests or bugs mergequeue-status: done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants