feat(MCP): Push MCP usage via OTel#7746
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7746 +/- ##
==========================================
+ Coverage 98.54% 98.56% +0.01%
==========================================
Files 1452 1459 +7
Lines 55821 56122 +301
==========================================
+ Hits 55011 55315 +304
+ Misses 810 807 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
khvn26
left a comment
There was a problem hiding this comment.
I'd rather not have user.organisations.first() heuristics at all than allow faulty data. I think it's important to provide exact data.
I would still much prefer setting the org id to a span attribute in the actual code that retrieves it for permission handling. It's the idiomatic OTel way IMO, with less potential failure modes and overall LoC to maintain.
I understand the need to provide org id explicitly so it's documented in the catalogue. However, I'd consider spending time on codegen + log tooling to allow adding select span attributes as OTLP event attributes (using e.g. a lazy getter callable or a sentinel object as a value). This would allow us to pave a way to provide common Flagsmith logging context (e.g. org id, user uuid, plan name, persona type etc) exactly from the source without having to maintain specific plumbing for each case.
Feel free to dismiss my review if you get another one while I'm OOO.
I agree. Would you please point out, in this PR, why you believe the code is not producing exact data?
a15009a — note that the permission layer does not cover all endpoints used by MCP, leading us to the single-org user fallback. Fixing that has a large blast radius, which I experimented with, and prefer leaving to a separate issue.
I understand the potential of span enrichment, and I think it warrants proper scoping. |
There was a problem hiding this comment.
Small NIT if ever there are additional back and forth but from my point of view the implementation is good and opened topics have been resolved or deferred.
Approving to unblock, leaving to your appreciation whether to wait on Kim' confirmation on the points he raised
Good point, I misread it initially. Yeah, optimising for the common case seems like the right call to me.
Aw shucks, I assumed otherwise. Thanks for uncovering this. Looks like a good bit of info for #7035.
Yes, agreed, there are decisions to make - for example, we might want to implement our own context layer instead of using spans directly. |
matthewelwell
left a comment
There was a problem hiding this comment.
Approving based on Kim's prior review and minor additional change.
For reference: (LLM content)
30 of 36 (83%) span-originated Tools from private modules aren't included. |
docs/if required so people know about the feature.Changes
Contributes to https://github.com/Flagsmith/flagsmith-private/issues/152
Filter incoming requests from MCP servers and emit an
mcp.tool.calledlog to OTel containing 1. the original W3C baggage from the request, and 2. the organisation ID.Telemetry can then be exported so we can track usage.
How did you test this code?
New unit + integration tests, and full local harness setup using an InfluxDB data storage as telemetry export target.