feat: add basic tracing to mcp

[liukatkat]

What:

This PR adds basic tracing to mcp.

Why:

We want to make mcp more robust and increase observability!

Test plan:

You can see traces getting sent!

[liukatkat]

• feat: connect traces to semgrep rpc process #95
• feat: add basic tracing to mcp #93 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[flaper87]

Thanks a lot for working on adding tracing. This is quite important 🙏

[flaper87]

I'm not sure we want to call this here. Similarly to the other PR, if this start_tracing is called here, we will have a single span used for every request sent to the server. The server_lifespan is called only once, and we really want to call start_as_current_span when a new request is received.

That being said, since FastMCP is based on FastAPI, I wonder if it'd be enough to follow these docs 🤔

[brandonspark]

Hi Flavio, could you confirm your concern with this behavior? My impression (which I have confirmed via local testing) is that the server_lifespan manager is invoked once per session registered to the MCP. Meaning, if we run one server, we could service several clients who want to use the MCP, for instance if we had numerous editors open, or if we were in mcp.semgrep.ai and many users wanted to use the MCP.

In which case, my understanding is this code will cause one top-level span to be registered for each user's session, and each request of semgrep_scan_rpc therein will also produce a span.

I personally believe this to be the desirable behavior--do you disagree?

[flaper87]

mmh, that may not be what we want. Thikning outloud here.

The way tracing of HTTP requests is normally done is that we would have a trace per request and multiple spans as part of that trace. That way, it is possible to analyze traces independently.

If we create a span per user session, that means all subsequent calls (usages of MCP/tools/etc) for that session will be under the same trace. This may make things like comparing multiple calls to security_check, as they will all belong to the same trace.

By creating a trace per request (tool call, etc), we can forward that trace.id to semgrep CLI and have semgrep's create spans under the same trace.

TL;DR: What I'm proposing is that we should not have a root span per user session, but rather per request.

Does it make sense? Happy to discuss this further

[liukatkat]

that makes sense. but i think that having a top-level span gives us more context when analyzing the calls. with the newly added semgrep mcp command, we pull and cache rules per each session. so all the calls in the same session will be using the same set of cached rules. we might also add more features in the future where the context of the session will matter for each call. so, this case might be slightly different from HTTP requests in the sense that each request is not necessarily independent.

i also think that we won't necessarily lose anything (in fact, i think we gain info) by nesting the calls under a parent span. but happy to know what you think about this!

[brandonspark]

Good work! w.r.t. whether we have a top-level span or not, this seems to be a two way door. With the release ideally happening today, let's bias towards getting this in.

two-way door, we want to get this out for now. can always revisit later