Skip to content

Conversation

@fali007
Copy link
Contributor

@fali007 fali007 commented Apr 23, 2025

Fixed #2662
This PR attempts to cover MCP instrumnetation into Traceloop.

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.
Screenshot 2025-04-23 at 10 26 53 AM

Important

Adds MCP OpenTelemetry instrumentation to Traceloop, enabling tracing of MCP framework workflows.

  • Behavior:
    • Adds McpInstrumentor class in instrumentation.py for MCP framework tracing.
    • Implements patch_mcp_server and patch_mcp_client functions for server and client request tracing.
    • Introduces serialize() function for request serialization.
    • Updates init_instrumentations() in tracing.py to initialize MCP instrumentation.
  • Configuration:
    • Adds .flake8, .python-version, README.md, poetry.toml, and pyproject.toml for MCP instrumentation package.
    • Updates pyproject.toml in traceloop-sdk to include MCP instrumentation.
  • Models:
    • Adds MCP-related attributes to SpanAttributes in semconv_ai/__init__.py.
  • Misc:
    • Adds MCP to Instruments enum in instruments.py.

This description was created by Ellipsis for a1eed91. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to a1eed91 in 2 minutes and 39 seconds. Click for details.
  • Reviewed 356 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:76
  • Draft comment:
    Index access assumptions (e.g. args[1]) are risky. Consider adding checks for argument count/structure.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a wrapper function used with wrapt, which guarantees a specific argument structure. The function is decorated with @with_tracer_wrapper which further enforces the argument structure. While array bounds checking is generally good practice, in this case the argument structure is enforced by the framework. The comment might lead to unnecessary defensive programming. I could be wrong about wrapt's guarantees. Maybe there are edge cases where the arguments could be different. Even if there are edge cases, this is framework code that expects a specific structure. Adding bounds checking would just obscure the intent. The comment should be removed as it suggests defensive programming in a context where the argument structure is guaranteed by the framework.
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:96
  • Draft comment:
    Similarly, accessing args[0].root in patch_mcp_client assumes a specific structure. Validate input structure to avoid potential IndexErrors.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a wrapper for MCP library's BaseSession.send_request method. The structure being accessed (args[0].root) is likely part of MCP's API contract. Since this is an instrumentation library specifically for MCP, we can reasonably assume the structure will be present. Adding validation would just add noise since this code only runs in the context of MCP library calls. The comment raises a valid point about defensive programming. If the MCP library changes its internal structure in future versions, this could break. The instrumentation explicitly declares it works with "mcp >= 1.3.0" in _instruments. If MCP makes breaking changes, they would need to update the version constraint anyway. The comment should be deleted. The structure assumptions are part of working with the MCP library's API, and version constraints handle compatibility.
3. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:50
  • Draft comment:
    The serialize function returns a JSON string; ensure that logging span attributes as f"{serialize(args[1])}" doesn’t lead to unexpected double encoding.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
4. packages/opentelemetry-instrumentation-mcp/pyproject.toml:10
  • Draft comment:
    Version mismatch: version in version.py is 0.39.1 but pyproject.toml specifies 0.39.0. Please align the versions.
  • Reason this comment was not posted:
    Marked as duplicate.
5. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:119
  • Draft comment:
    Sensitive request arguments are always logged. Consider honoring the TRACELOOP_TRACE_CONTENT flag before logging MCP_REQUEST_ARGUMENT.
  • Reason this comment was not posted:
    Marked as duplicate.
6. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:70
  • Draft comment:
    Avoid suppressing exceptions silently in the serialize function; consider logging the exception for easier debugging.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a tracing/instrumentation library where the serialize() function is used to capture request data as span attributes. Silent failure is actually appropriate here since we don't want instrumentation issues to affect the main application flow. Logging errors could spam logs with unimportant serialization failures. The function already handles failures gracefully by returning a partial result. Maybe some errors could indicate bugs in the instrumentation that should be fixed. Without logging, these issues could go unnoticed. The primary goal of this code is to be unobtrusive instrumentation. Logging serialization failures would likely create more problems (log spam) than it would solve, since serialization failures are expected and handled appropriately. The comment should be deleted. The current silent exception handling is appropriate for this instrumentation use case where we want to capture what data we can without affecting the main application.
7. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:76
  • Draft comment:
    The wrapper assumes a specific structure of the args array (e.g. args[1] for server and args[0] for client). Consider adding defensive checks to avoid potential IndexError if the underlying mcp API changes.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid concern about defensive programming. However, this is an instrumentation wrapper that specifically targets MCP's internal API methods. The args structure is defined by the MCP library's function signatures that we're wrapping. If those signatures changed, the instrumentation would need to be updated anyway. Adding defensive checks wouldn't really help since the instrumentation would be broken regardless. The comment raises a legitimate point about defensive programming. A change in the MCP library could cause crashes that would be harder to debug without checks. While defensive programming is good practice, in this case the args structure is part of the API contract with MCP. If it changes, the instrumentation needs to be updated anyway, so the checks would just obscure the real issue. Delete the comment. The args structure is part of MCP's API contract, and defensive checks would just mask bigger compatibility issues if that contract changes.
8. packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py:25
  • Draft comment:
    Typo: The constant PINECONE_DB_USAGE_WRITE_UNITS is set to "db.pinecone.usage_write_units". For consistency with PINECONE_DB_USAGE_READ_UNITS ("db.pinecone.usage.read_units"), consider changing it to "db.pinecone.usage.write_units".
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. packages/traceloop-sdk/pyproject.toml:18
  • Draft comment:
    It appears that the repository URL and documentation fields use 'openllmetry' (with two L's) while the dependency names use 'opentelemetry'. Please confirm if 'openllmetry' is intentional or if it should be corrected to 'opentelemetry' for consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:135
  • Draft comment:
    Minor typographical error: The method 'aupload_base64_image' used on image_uploader may be intended to be 'upload_base64_image'. Please verify and update if necessary.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_QcmoZLDsFqX86xHL

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

fali007 added 3 commits April 23, 2025 11:00
Signed-off-by: Felix George <[email protected]>
Signed-off-by: Felix George <[email protected]>
@fali007
Copy link
Contributor Author

fali007 commented Apr 24, 2025

@gyliu513 , @nirga , @galkleinman Please take a look at the PR.

Signed-off-by: Felix George <[email protected]>
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fali007 - can you fix the CI errors?

Signed-off-by: Felix George <[email protected]>
Copy link
Contributor

@gyliu513 gyliu513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, thanks @fali007 !

@fali007 fali007 requested a review from galkleinman April 25, 2025 04:59
@fali007
Copy link
Contributor Author

fali007 commented Apr 28, 2025

cc: @hk-bmi

@fali007
Copy link
Contributor Author

fali007 commented Apr 29, 2025

@galkleinman , @nirga can you review once when you have some time. Ive changed the patching of private method to public one.

@fali007
Copy link
Contributor Author

fali007 commented Apr 29, 2025

Screenshot 2025-04-29 at 4 03 48 PM Adding latest traces screen shot and trace file [trace.json](https://github.com/user-attachments/files/19957084/trace.json)

Copy link
Contributor

@galkleinman galkleinman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better. Even tho, spans are still super generic.
Let's treat this one as first iteration, and evolve and improve it in following PRs.

Copy link
Contributor

@gyliu513 gyliu513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @fali007 !

I think as a follow up, we need the following actions:

  1. add a sample app for mcp at https://github.com/traceloop/openllmetry/tree/gk/crewai-instrumentation/packages/sample-app/sample_app
  2. @galkleinman I think we probably need a blog to prompt this initial support of MCP observability

@nirga nirga merged commit 4a095a2 into traceloop:main Apr 29, 2025
9 checks passed
@dinmukhamedm
Copy link
Collaborator

Hey @fali007, any specific reason why this is python >=3.10, while all other instrumentations in this repo are >=3.9?

@dinmukhamedm
Copy link
Collaborator

Sorry, nevermind, I'm dumb, that's because the underlying package supports only 3.10 onwards

@fali007 fali007 deleted the dev-mcp branch June 10, 2025 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Support for Model Context Protocol (MCP)

5 participants