fix: make mcp span names informative #1288
Conversation
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (77.49%) is below the target coverage (86.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1288 +/- ##
==========================================
+ Coverage 77.44% 77.49% +0.04%
==========================================
Files 123 123
Lines 15651 15684 +33
==========================================
+ Hits 12121 12154 +33
Misses 2901 2901
Partials 629 629 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Head branch was pushed to by a user without write access
|
changed to re-use MCP go naming conventions as that also matches the openinference names of these spans. |
|
now envoy ai gateway generated spans look similar! 
|
|
🤞 last commit deflakes |
|
wasn't enough to deflake I am flying to NYC now. appreciate if someone can take over otherwise will wait a while |
|
unified trace looking coooooooooooooooooooool |
|
I will rewrite the test so that not all tests also do tracing because it is too hairy ;) |
codefromthecrypt
force-pushed
the
mcp-names
branch
from
46a8f00 to
d9e043f
Compare
Signed-off-by: Adrian Cole <[email protected]>
codefromthecrypt
force-pushed
the
mcp-names
branch
from
d9e043f to
485a564
Compare
|
ok brutal work, but hopefully deflaked |
| // not being able to run in end-to-end tests. The test code must solely operate through the client side sessions. | ||
| t.Cleanup(func() { | ||
| require.NoError(t, ret.session.Close()) | ||
| _ = ret.session.Close() |
There was a problem hiding this comment.
should be a linter for this somewhere. @anuraaga come across any that notice when accidental use of require.RequreXX n a cleanup? t.LogXX is fine, but require is bad
There was a problem hiding this comment.
There's a close one but no cigar
https://github.com/Antonboom/testifylint?tab=readme-ov-file#go-require
Probably not too hard to adapt that to apply to both cases though
| {name: "Complete", testFn: testComplete}, | ||
| } | ||
|
|
||
| func TestMCP(t *testing.T) { |
There was a problem hiding this comment.
I broke these up vs having triple nested subtests
codefromthecrypt
left a comment
codefromthecrypt
left a comment
There was a problem hiding this comment.
made notes on the edge cases
| require.NoError(t, err) | ||
| require.False(t, res.IsError) | ||
| // Consume the span from the cleanup operation. | ||
| _ = m.collector.TakeSpan() |
There was a problem hiding this comment.
this bit hard
| case <-time.After(time.Microsecond * 500): | ||
| cancel() | ||
| // Wait for the goroutine to complete so its span doesn't leak to the next test. | ||
| <-doneCh |
There was a problem hiding this comment.
this also bit hard ;)
|
most of why this test is so hard is we share resources including otel everywhere in this file. It would probably be better to have two tests at some point. one that does all this behavioural stuff. The other simply tests each op creates the span expected. Combining both made this a huge amount of work to fix |
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
| } | ||
| err = m.handleSetLoggingLevel(ctx, s, w, msg, p, span) | ||
| case "ping": | ||
| // Ping is intentionally not traced as it's a lightweight health check. |
|
one sec I found I didn't assert our span events. |
|
you have my one sec! |
Signed-off-by: Adrian Cole <[email protected]>
|
k done :) |
| } | ||
| event.Attributes = filteredAttrs | ||
| } | ||
| require.Empty(t, cmp.Diff(expectedEvents, span.Events, protocmp.Transform())) |
There was a problem hiding this comment.
ps this is my favorite trick in tests!
|
thanks for the merge. since latest commit is pushed to docker, the new elastcisearch example can see this new data policy! elastic/observability-examples#90 |
|
**Description** Before everything was vaguely "mcp.request" which is not untrue, but not helpful. There are plenty of low-cardinality choices for span or metric names and the `req.Method` is the most intuitive. This uses the same naming conventions as mcp-go --------- Signed-off-by: Adrian Cole <[email protected]> Signed-off-by: Hrushikesh Patil <[email protected]>
**Description** Before everything was vaguely "mcp.request" which is not untrue, but not helpful. There are plenty of low-cardinality choices for span or metric names and the `req.Method` is the most intuitive. This uses the same naming conventions as mcp-go --------- Signed-off-by: Adrian Cole <[email protected]> Signed-off-by: Hrushikesh Patil <[email protected]>
**Description** Before everything was vaguely "mcp.request" which is not untrue, but not helpful. There are plenty of low-cardinality choices for span or metric names and the `req.Method` is the most intuitive. This uses the same naming conventions as mcp-go --------- Signed-off-by: Adrian Cole <[email protected]> Signed-off-by: Hrushikesh Patil <[email protected]>
**Description** Before everything was vaguely "mcp.request" which is not untrue, but not helpful. There are plenty of low-cardinality choices for span or metric names and the `req.Method` is the most intuitive. This uses the same naming conventions as mcp-go --------- Signed-off-by: Adrian Cole <[email protected]> Signed-off-by: Erica Hughberg <[email protected]>
5 participants
Description
Before everything was vaguely "mcp.request" which is not untrue, but not helpful. There are plenty of low-cardinality choices for span or metric names and the
req.Methodis the most intuitive. This uses the same naming conventions as mcp-go