SEP-414: Document OpenTelemetry Trace Context Propagation Conventions#414
SEP-414: Document OpenTelemetry Trace Context Propagation Conventions#414codefromthecrypt wants to merge 8 commits intomodelcontextprotocol:mainfrom
Conversation
|
I would love the text in the spec to include more of what you have written in the PR description, and that the recommendation is to use It could be labelled as "Optional: OpenTelemetry context". I hope that SDKs decide to implement it where they have existing OTel ecosystems that they can leverage. MCP clients are not required to send it, and MCP servers that do not support OTel can safely ignore those parameters. This may be too much for the index file, but they could be added to the typescript and documentation. |
|
@samsp-msft, thanks for your thoughtful feedback! I’ve updated the "Basic" section to keep it neutral and avoid implying MCP defines OpenTelemetry (OTel) specifics: Updated Rationale:This keeps Specifically, we want to make room in otel for what's likely an inevitable OTEP for MCP. I say inevitable because the other discussion includes transport details (e.g. OTLP which is not propagation). This feels a lot like past discussions that led to specification clarifications including env variable propagation and how to handle messaging. In fact, there's already work in Otel for semantic conventions (thanks for participating in that). Ideally an engineer will be able to see the otel details coherently in one place (e.g. an OTEP). Long story short, deferring OTel keeps MCP focused on protocol mechanics, not telemetry details. By adding an example, we hint of how to hand-over to otel for clarity on telemetry. Does this address your concern? |
6c55543 to
2081ae2
Compare
|
I took time to update the description with feedback in the comments and also relating to discussions by @ZengyiZhao here and @wdawson here. Hope we can land this soon! |
|
@dsp-ant @jspahrsummers So, for anecdotal context. What drove me to the discussion leading to this and the PR itself was your podcast on latent space with @FanaHOVA and @swyxio Was a knock-out episode, and since working in oss since 2008 this made me feel the best:
So, my goal with this change was to make the absolutely least change possible, with the highest rigor. Even if it is a no, all good. Thanks for inspiring me to give it a go. I love the do the work, then let's talk approach to change. |
LGTM There is a balance between what needs to go into the base specification - providing the place to pass context - and having agreement amongst the SDK as to how they will use the extensibility mechanisms to implement telemetry propagation. The details of which key values pairs should be used and their values can probably be delegated to docs in the OTel space. It can probably go in the docs for the semantic conventions. I am hoping that we can get common agreement amongst most MCP SDKs about how to incorporate telemetry so that they can interoperate nicely, and it doesn't need major retrofits later. |
|
@samsp-msft thanks for the support. PS I raised a PR to csharp-sdk to change the carrier from |
757a83e to
8a5e71f
Compare
|
Earlier I mis-attributed I've revised the spec change to link to the progress spec, and also shored up the description accordingly. |
|
@dsp-ant do you have any advice for us on how to progress this PR? I'm happy to revise it, but it seems stalled. |
|
Right now, by naming convention ( Specifically, this PR describes some use cases:
Whereas in the schema today, there are 13 occurrences of this advice on
The recent work by @findleyr and friends on the Go SDK design implies code generation, and there's a small gap you can see if you look carefully here.
There are a number of ways to address code generation coherency.. we could make a single "meta" type and use that for Finally, we can decide to not solve it strictly in the schema. Rather, stick with advice here and mention to code generator authors that there's a relationship with anything ending in Request, Response etc that should have special casing. I don't have a preferred way out, but I would like to help close out this topic. Any thoughts? |
|
added a section to the description that it is possible a future JSON-RPC 2.1 could formalize "_meta". That said, I don't expect this to change any current practice. cc @mpcm |
|
@codefromthecrypt See recently posted: https://groups.google.com/g/json-rpc/c/pFFuI0JN8Cs |
|
In the JSON-RPC group discussion, I mentioned that responses could benefit from a The protocol currently uses |
|
@jonathanhefner Thanks for the suggestion - personally, if the compatibility issue is acceptable I would suggest top level |
|
We have a similar need. I too would prefer something outside of the params, although it's a good way to experiment for the time being. |
|
going to close this out as it hasn't moved forward in a month. happy to re-open when maintainers are interested in a change. Meanwhile, per the description, there are enough artifacts here and there to suggest |
|
FWIW, i was able to abuse the protocol's sending: https://github.com/dylibso/mcp-otel/blob/2407c736c92d6a5e71b454845d839f33dabbbfca/src/agent.ts#L122 I think this should be safe if you do your best to avoid name collisions, but going to ask around. Doing the actual context propagation and naming of everything works, but is a little tedious if you're not familiar with it. This could perhaps be part of an otel adapter for the SDKs, or one day adopted with the SDKs if some things can be agreed upon. |
|
@bhelx thx for the feedback. I will add to the description your use of this pattern which aligns with others mentioned there including Arize Openinference which is an otel SDK. That way folks don't have to scroll through comments should there be a desire to formalize this later. |
|
@Kludex any process steps missing before this can merge? |
|
This looks good. It's just a formality of what is already happening in the ecosystem anyway. It looks good to me. I'll check if we can present on the next meeting. |
|
It seems the meeting is today, I'll see if I can join. |
| "location": "New York" | ||
| }, | ||
| "_meta": { | ||
| "traceparent": "00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01" |
There was a problem hiding this comment.
Hmm, _meta keys are supposed to have reverse DNS vendor prefix (e.g. io.opentelemetry/traceparent), so this is not compliant with how things are supposed to be. The problem is that it seems everyone is already doing this.... may need a pragmatic compromise.
There was a problem hiding this comment.
Q: How disruptive would it be to have OpenTelemetry update this? Perhaps add both during a deprecation period?
There was a problem hiding this comment.
There is a huge ecosystem already using opentelemetry that has adopted the traceparent convention, and changing that would have a very high impact. I think this change is unlikely to happen.
Namespacing with the vendor prefix makes perfect sense for things that are new, but I think it is important to acknowledge that there may be some widely adopted and well-established conventions already in the ecosystem that, if we want this to be as interoperable as possible and facilitate adoption, should be supported.
There was a problem hiding this comment.
added a commit to clarify this exception
| ### Why document this? | ||
|
|
||
| This is currently documented elsewhere, but not as an MCP specification. Doing so ensures that | ||
| SEPs depending on this pattern can complete, as well as other SDKs in and outside the MCP org |
There was a problem hiding this comment.
they are listed in the below section of SEPs
### Related SEPs
- [SEP-1788](https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1788) - reserved
keys in `_meta`; should be updated with `traceparent`, `tracestate`, and `baggage` when this
SEP is implemented
- [SEP-2028](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2028) - builds on
this SEP for forwarding `_meta` values to HTTP headers
|
This was accepted by Core Maintainers in the Core Maintainer meeting of Feb 4th and an async vote afterwards. @pwwpche had some conerns that we should address. Other than thatn, please ensure it's in a good shape. we need to have
finalized. Ping a core maintainer to review and merge. |
|
@pwwpche lemme know what we should revise here, and I'll rebase and clear the conflicts. thanks! |
@codefromthecrypt @pwwpche's concern was mostly to document why we cannot use the DNS prefix we usually enforce for |
Co-authored-by: Cliff Hall <cliff@futurescale.com>
c3e532e to
8712009
Compare
|
thanks @dsp-ant makes sense. I added text I believe highlights this more clearly in the last commit. |
Document OpenTelemetry trace context propagation conventions for _meta keys (traceparent, tracestate, baggage) in the draft spec, including the rationale for the DNS prefix exception. Signed-off-by: Adrian Cole <adrian@tetrate.io>
|
ok added the spec and changelog. 🚢 when ready or tell me what's next. Thanks, all! |
- Status: Final (accepted with reference implementations in org SDKs) - Created: 2025-04-25 (PR creation date per convention) Signed-off-by: Adrian Cole <adrian@tetrate.io>
|
Polish notes: I set this status to Final as it is accepted and reverse documents the existing c# SDK implementation, which at the same time serves as a reference. This is inline with other SEPs all marked final, as well. For the SEP date, I noticed another is using the PR creation date, so went with that. |
Summary
This PR introduces SEP-414, documenting OpenTelemetry Trace Context Propagation Conventions
SEP
seps/414-request-meta.mdSponsor
@Kludex
Notes
This PR converts the earlier doc change into a standards-track SEP. Discussion links and rationale are in the SEP itself.