Draft : MCP open telemetry instrumentation #2812
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 2caa60b in 3 minutes and 0 seconds
More details
- Looked at
379lines of code in13files - Skipped
0files when reviewing. - Skipped posting
15drafted comments based on config settings.
1. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py:167
- Draft comment:
Minor: Extra blank line removed. This is cosmetic—ensure consistency across similar wrappers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/opentelemetry-instrumentation-mcp/README.md:1
- Draft comment:
The title mentions 'CrewAI Instrumentation' but this package targets MCP instrumentation. Please update for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to update the title for clarity. This falls under the rule of not asking the PR author to update the PR description or title. Therefore, this comment should be removed.
3. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:78
- Draft comment:
Reliance on positional arguments (args[1]) for the server patch may be fragile. Confirm the signature remains stable. - 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 code quality concern - using positional args indexing can be fragile. However, this is an instrumentation wrapper using wrapt's established pattern where args contains the original function arguments. The code is specifically wrapping Server._handle_request which presumably has a stable interface. Without seeing the mcp library's interface contract, we can't be certain this is actually problematic.
I may be overconfident in assuming the mcp library's interface stability. The comment could be highlighting a real maintainability risk.
While interface stability is a fair concern, this appears to be standard instrumentation code following wrapt's patterns. Without evidence that the mcp interface is unstable, this seems like speculation.
Delete the comment as it's speculative without clear evidence of an actual problem. The code follows standard wrapt instrumentation patterns.
4. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:96
- Draft comment:
Similarly, in the client patch rely on args[0] and args[1]. Consider documenting or validating signature assumptions. - 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%
The code does make assumptions about parameter structure, but this appears to be part of the MCP protocol/library interface. The function is a wrapper specifically for MCP client requests, so these assumptions are likely by design. Adding validation might add unnecessary overhead, and documentation would be redundant since this is clearly an MCP-specific implementation.
I might be underestimating the value of explicit documentation. Future maintainers might benefit from understanding the expected structure.
However, this is an internal implementation detail of an instrumentation wrapper. The structure is dictated by the MCP library, not this code.
The comment should be deleted as it suggests documenting/validating what are actually fixed protocol requirements, not arbitrary assumptions.
5. packages/opentelemetry-instrumentation-mcp/pyproject.toml:14
- Draft comment:
The repository URL appears to have a typo ('openllmetry' instead of 'opentelemetry'); please confirm and correct if needed. - Reason this comment was not posted:
Marked as duplicate.
6. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:35
- Draft comment:
Consider replacing the plain print statement in the exception handler with a proper logging call (e.g. logging.error) for consistency with other instrumentors. - Reason this comment was not posted:
Marked as duplicate.
7. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:77
- Draft comment:
The patch functions assume fixed positions for arguments (e.g. args[1] and args[0]); if the API changes, this could break. Consider adding checks or comments documenting the expected argument structure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a legitimate concern about code robustness. The code assumes specific argument positions which could break if the MCP API changes. However, this is an instrumentation package specifically for MCP, and these wrapper functions are internal implementation details. The argument structure is likely dictated by the MCP API and wrapt library conventions.
I might be underestimating the value of defensive programming. Even if the API is stable now, future maintainers would benefit from understanding the expected argument structure.
While documentation could help, this is an implementation detail of an instrumentation package. The argument structure is tied to the MCP API and wrapt library, which already define these contracts.
The comment raises a valid point but suggests changes that are overly defensive for an internal implementation detail that follows established API contracts.
8. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:50
- Draft comment:
The recursive serialize function may encounter non-serializable or cyclic objects; consider adding cycle detection or a fallback to avoid potential recursion issues. - 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%
The function already has safeguards: 1) depth limit prevents infinite recursion 2) exception handling prevents crashes 3) is_serializable check catches many problematic cases upfront. While cycle detection could be added, the current implementation is pragmatic and safe for its use case of debug tracing. The suggestion is more of an optimization than a critical issue.
The current implementation could potentially miss some object attributes when it encounters cycles, leading to incomplete tracing data. The empty exception handler could hide useful debugging information.
For tracing purposes, having incomplete data is acceptable - it's better than crashing or bloating traces with deep object structures. The safeguards make the function robust enough for its intended use.
While the suggestion has merit, the current implementation is sufficiently robust for its tracing use case. The comment suggests an optimization rather than fixing a real problem.
9. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py:553
- Draft comment:
Typographical issue: The docstring for the on_llm_start function reads "Run when Chat Model starts running." which is misleading. This function is meant for handling LLM start events. Please update the docstring to correctly reflect its purpose, e.g., "Run when LLM starts running." - 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/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/__init__.py:4
- Draft comment:
There is no newline at the end of the file. While this won't break anything, adding a trailing newline is a common convention for better readability and consistency. Please consider adding one. - 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%
While having a newline at the end of files is indeed a common convention, this is a very minor stylistic issue. It doesn't affect functionality at all. Most modern editors handle this automatically, and many linters/formatters would catch this if it's part of the project's style guide. This feels like the kind of minor, obvious issue we're supposed to avoid commenting on.
The missing newline could cause issues with some tools or make diffs less clean. It is a real convention that many projects enforce.
While true, this is exactly the kind of minor issue that should be handled by automated tooling rather than manual review comments. If it's important to the project, it should be enforced by a linter.
Delete this comment. It's too minor of an issue and would be better handled by automated tooling if the project cares about it.
11. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:36
- Draft comment:
Minor typographical issue: in the print statement on line 36, consider changingprint(f"Error : {e}")toprint(f"Error: {e}")(remove the extra space before the colon) for consistency. - 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%
The comment is about a very minor stylistic issue that doesn't affect functionality. While technically correct, it's an extremely trivial change that doesn't meaningfully improve code quality. Our rules state we should not make purely informative comments or comments about obvious/unimportant issues. This seems to fall into that category.
The consistent formatting of error messages could be considered part of code quality. Some teams might have strict style guides about error message formatting.
While consistency is good, this is too minor to warrant a PR comment. If there are formatting standards, they should be enforced by automated linting rather than manual review comments.
This comment should be deleted as it addresses an extremely minor stylistic issue that doesn't meaningfully impact code quality or functionality.
12. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/version.py:1
- Draft comment:
Trivial: The file is missing a newline at the end. While this is a minor formatting issue, adding a newline at the end of the file is a common convention that can prevent issues in some environments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is about a minor formatting issue, specifically a missing newline at the end of a file. While this is a common convention, the comment is purely informative and does not suggest a specific code change or improvement. It also does not address a potential bug or issue that could arise from this formatting choice. Therefore, it seems to violate the rule against making purely informative comments.
13. packages/opentelemetry-instrumentation-mcp/pyproject.toml:14
- Draft comment:
Typographical error: The repository URL contains a typo. It is written as 'openllmetry' instead of 'opentelemetry'. Please correct the URL. - 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%
The repository URL appears to be part of a larger project. The fact that it's "openllmetry" could be intentional - it might be a play on words combining "opentelemetry" with "LLM". Without access to the actual repository or more context, I can't be certain this is actually a typo. The comment assumes it's a mistake without evidence.
I might be overthinking this - typos in URLs are generally worth fixing. But what if this is the actual, intended repository name?
Given that this could be an intentional naming choice (combining OpenTelemetry and LLM concepts), and without being able to verify the correct repository URL, making assumptions about it being a typo could be misleading.
The comment should be deleted because we don't have strong evidence that this is actually a typo versus an intentional naming choice.
14. packages/traceloop-sdk/pyproject.toml:18
- Draft comment:
Typo found in the repository URL. The segment 'openllmetry' appears to contain a typo. It likely should be 'opentelemetry' to match the naming used in other dependencies. - 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.
15. packages/traceloop-sdk/pyproject.toml:19
- Draft comment:
Typo found in the documentation URL. The segment 'openllmetry' appears to be a misspelling; it likely should be 'opentelemetry' for consistency with the other related naming. - 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_2PXmYwITUQlvcZZD
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
...emetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Outdated
Show resolved
Hide resolved
|
Fixed #2662 |
nirga
reviewed
Apr 14, 2025
|
|
||
| carrier = None | ||
| ctx = None | ||
| if hasattr(args[0].root.params, 'arguments'): |
There was a problem hiding this comment.
I'm not a big fan of this as it creates a dependency between the langchain instrumentation and the MCP one (it took me a while to understand that the __meta__ argument is being injected there and then read here. This is exactly why we have the Python context that OpenTelemetry is already using. What we need is to instrument the MCP tool call method within langchain and set the trace context there. Then, we can just read it it here directly from the context
There was a problem hiding this comment.
I understand that this is unconventional. But I couldn't find any other way (I am also not very happy with this implementation). StructuredTool has a coroutine to call below function in MCP client. As you can see it only accepts arguments. The original message where I can add trace contexts are created in the MCP Client code. I am sending trace contexts using __meta__ field inside arguments and removes it in MCP client. If you know any other way to remove this please guide me.
async def call_tool(
self, name: str, arguments: dict[str, Any] | None = None
) -> types.CallToolResult:
"""Send a tools/call request."""
return await self.send_request(
types.ClientRequest(
types.CallToolRequest(
method="tools/call",
params=types.CallToolRequestParams(name=name, arguments=arguments),
)
),
types.CallToolResult,
)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Outdated
Show resolved
Hide resolved
galkleinman
reviewed
Apr 18, 2025
|
|
||
| _W( | ||
| "mcp.server.lowlevel.server", | ||
| "Server._handle_request", |
There was a problem hiding this comment.
a bit smelly and prone to be break the fact we're wrapping "private" method. is there any "public" wrapper for it?
Comment on lines
52
to
53
| def serialize(request, depth=0): | ||
| if depth > 2: |
There was a problem hiding this comment.
Suggested change
| def serialize(request, depth=0): | |
| if depth > 2: | |
| def serialize(request, depth=0, max_depth=2): | |
| if depth > max_depth: |
| if depth > 2: | ||
| return {} | ||
| depth += 1 | ||
| def is_serializable(request): |
There was a problem hiding this comment.
[STYLE] newline above imo for the sake of readability
| def serialize(request, depth=0): | ||
| if depth > 2: | ||
| return {} | ||
| depth += 1 |
There was a problem hiding this comment.
it's better to avoid manipulating argument values
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft PR for adding opentelemetry instrumentation of MCP servers in traceloop.
feat(instrumentation): ...orfix(instrumentation): ....Important
Add OpenTelemetry instrumentation for MCP servers with new
McpInstrumentorclass and integrate it into the tracing framework.McpInstrumentorclass ininstrumentation.pyto instrument MCP server and client methodsServer._handle_requestandBaseSession.send_request.McpInstrumentorintotracing.pywithinit_mcp_instrumentor()function..flake8and.python-versionfiles for MCP package.pyproject.tomlandpoetry.tomlfor MCP package dependencies and settings.README.mdfor MCP instrumentation with installation and usage instructions.instruments.pyto includeMCPin theInstrumentsenum.This description was created by
for 2caa60b. It will automatically update as commits are pushed.