-
Notifications
You must be signed in to change notification settings - Fork 488
feat(contrib/mcp-go): Trace MCP session initializations with MLObs spans #4101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: jb/contrib-mcp-go-base
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BenchmarksBenchmark execution time: 2025-11-07 19:57:54 Comparing candidate commit 5f20214 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 0 unstable metrics. |
| span.AnnotateTextIO(string(inputJSON), err.Error()) | ||
| } | ||
| if annotator, ok := span.(textIOAnnotator); ok { | ||
| inputJSON, _ := json.Marshal(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this expected to happen often? I think at least we should log something (debug if this is expected to happen somewhat frequently, or warn if not).
also, should we annotate the input with the error or some special message to let customers know something went wrong directly while viewing the span?
contrib/mark3labs/mcp-go/hooks.go
Outdated
| inputJSON, _ := json.Marshal(request) | ||
| var outputText string | ||
| if result != nil { | ||
| resultJSON, _ := json.Marshal(result) | ||
| outputText = string(resultJSON) | ||
| } | ||
| resultJSON, _ := json.Marshal(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, i think we should handle these errors somehow
| ) | ||
| toolCache.OnEviction(func(ctx context.Context, reason ttlcache.EvictionReason, item *ttlcache.Item[any, *llmobs.ToolSpan]) { | ||
| spanCache.OnEviction(func(ctx context.Context, reason ttlcache.EvictionReason, item *ttlcache.Item[any, llmobs.Span]) { | ||
| if span := item.Value(); span != nil { | ||
| if reason == ttlcache.EvictionReasonExpired { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also finish spans that were evicted for other reasons?
498ae3a to
792093f
Compare
95af4d3 to
4def1e6
Compare
27c29ed to
f2727e3
Compare
f2727e3 to
5f20214
Compare
|

NB: This is part of a stack
What does this PR do?
In addition to tool calls, record task spans for session initialization.
Motivation
We want to record data like clientInfo during session Initialization and this seems like the best way to do it.
I considered a metric, but in the future we will also want to tag the session ID, and metrics do not do well with unique tags. Also, this allows viewing everything together in the LLMObs UI.
A writeup with more context on this design
Relates to MLOB-4373
Future
In future PRs I will add tags for client info and session ID.
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!