instrumentation for @modelcontextprotocol/sdk

[narengogi]

Credit where it's due, The mcp server in the sample app is from the following repository:
https://github.com/modelcontextprotocol/servers

• I've not been able to get auto-instumentation to work for the Classes I'm instrumenting, I've verified the path to the exports, but the otel package is not picking them up for patching, this would require users to manually instrument thus

  
  const mcpInstrumentation = new McpInstrumentation();
  mcpInstrumentation.manuallyInstrument(sse);
  
  const consoleExporter = new ConsoleSpanExporter();
  const provider = new NodeTracerProvider({
    spanProcessors: [new SimpleSpanProcessor(consoleExporter)]
  });
  provider.register();

  registerInstrumentations({
    instrumentations: [mcpInstrumentation],
  });

• Have to patch the send method on both stdio and sse clients, currently I'm only instrumenting the onmessage method, which receives messages from the server
• Not keen on how useful instrumenting the server module would be. I've seen some discussion from @fali007 mentioning how attaching meta to the messages can help with distributed tracing, this is something I have to look into Adding Opentelemetry to MCP SDK modelcontextprotocol/python-sdk#421
• Have to write tests
• Add sample apps

---

Important

Adds OpenTelemetry instrumentation for MCP with manual instrumentation for SSE and stdio clients, and integrates it into the Traceloop SDK.

• Instrumentation:
  • Adds @traceloop/instrumentation-mcp package for OpenTelemetry instrumentation of MCP.
  • Implements McpInstrumentation class in instrumentation.ts for manual instrumentation of sse and stdio clients.
  • Adds manuallyInstrument() method to handle onmessage method wrapping for both clients.
• Semantic Attributes:
  • Adds MCP-related attributes to SpanAttributes in SemanticAttributes.ts.
• SDK Integration:
  • Updates traceloop-sdk to include @traceloop/instrumentation-mcp in package.json.
  • Modifies initialize-options.interface.ts to support MCP modules.
  • Updates index.ts in tracing to initialize and configure McpInstrumentation.
• Configuration:
  • Adds rollup.config.js for building the new package.
  • Updates package.json scripts and dependencies for the new package.

^{This description was created by for e408685. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to e408685 in 2 minutes and 13 seconds. Click for details.

• Reviewed 388 lines of code in 9 files
• Skipped 3 files when reviewing.
• Skipped posting 7 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/ai-semantic-conventions/src/SemanticAttributes.ts:60

• Draft comment:
  Consider adding inline documentation (e.g. JSDoc comments) for the new MCP semantic attributes for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. packages/instrumentation-mcp/src/instrumentation.ts:25

• Draft comment:
  The 'manuallyInstrument' method expects two parameters (sseModule, stdioModule) but is later called with a single argument in the SDK. Consider updating its signature or the SDK usage to pass both modules.
• 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 refers to SDK usage that we can't see in this diff. The method implementation clearly expects and uses both parameters correctly. Without seeing the SDK code where this is called, we can't verify if there's actually a problem. The comment is speculative about code we can't see. The comment could be correct - there might be a real mismatch in the SDK. We're only seeing one side of the potential issue. However, per the rules, we should not keep speculative comments or comments that require more context from other files to verify. The implementation itself is correct. Delete the comment since it requires examining code outside this file to verify, and the implementation shown here is internally consistent.

3. packages/instrumentation-mcp/src/instrumentation.ts:56

• Draft comment:
  There is a commented-out patch for the 'send' method. If capturing outgoing messages is needed for tracing, consider instrumenting 'send' as well.
• 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 is focused on tracing MCP client responses (as seen in _startSpan method name and attributes). The commented line appears to be intentionally disabled. Without knowing the full requirements or design decisions, suggesting to enable outgoing message tracing could be presumptuous. The author likely commented it out deliberately. I could be wrong about the intentionality - maybe it was just temporarily commented for testing. The tracing could be incomplete without capturing both directions of communication. Even if it was temporary, we should trust the author's current implementation. If bidirectional tracing becomes a requirement, it can be added then. The comment makes assumptions about tracing requirements and questions an intentional code choice without strong evidence that it's needed.

4. packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts:115

• Draft comment:
  The MCP configuration currently only includes 'sseModule'. If stdio instrumentation is also intended, consider adding a 'stdioModule' property.
• 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 speculative - it suggests adding something that might be needed without any clear evidence that it is actually needed. The code only shows SSE-related changes. We don't have context about whether stdio instrumentation is even a planned feature. This seems like the kind of speculative "what if" comment we should avoid. Perhaps there are plans for stdio instrumentation that aren't visible in this file. The comment could be raising a valid architectural consideration. Even if stdio support is planned, this comment is still speculative and asks the author to consider something without clear evidence it's needed. We should wait for actual stdio implementation before suggesting configuration changes. This comment should be deleted as it's speculative and suggests adding configuration without clear evidence that it's needed.

5. packages/traceloop-sdk/src/lib/tracing/index.ts:219

• Draft comment:
  The call to 'mcpInstrumentation.manuallyInstrument' is passing a single argument, but the method signature requires two parameters (sseModule and stdioModule). Update the call to provide both modules or adjust the method signature.
• 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 presence of @ts-ignore suggests this is intentional. All other instrumentations follow the same single-argument pattern. Without seeing the McpInstrumentation class definition, I can't verify if this is actually an issue or if the type definitions are just not up to date. The consistent pattern across all other instrumentations suggests this is likely working as intended. I don't have access to the McpInstrumentation class definition, so I can't verify if this is actually a problem. The comment could be correct about the method signature. While I can't see the class definition, the consistent pattern across all instrumentations and the explicit @ts-ignore suggest this is working as intended. If it were broken, it would likely have been caught in testing. Without strong evidence that this is actually a problem, and given the consistent pattern across other instrumentations, this comment should be removed.

6. packages/instrumentation-mcp/package.json:8

• Draft comment:
  Typo: The repository field "traceloop/openllmetry-js" seems to have a typo ("openllmetry-js"). Did you mean "opentelemetry-js"?
• 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% Looking more carefully, I notice that the package name is "@traceloop/instrumentation-mcp" and the homepage URL also uses "openllmetry-js". This suggests that "openllmetry" might actually be intentional - it could be a play on words combining "LLM" and "telemetry". The consistency between repository and homepage URLs suggests this is not a typo but an intentional naming choice. I might be overthinking this - maybe it really is just a simple typo that should be fixed. The package clearly relates to OpenTelemetry based on its dependencies and description. The consistent use of "openllmetry" in both the repository and homepage fields, combined with this being a new file where such naming would have been carefully chosen, strongly suggests this is intentional branding rather than a mistake. The comment should be deleted as it's pointing out something that appears to be an intentional naming choice rather than a typo.

7. packages/instrumentation-mcp/package.json:57

• Draft comment:
  Typo: The homepage URL contains "openllmetry-js" which likely should be "opentelemetry-js".
• 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% While it might look like a typo at first glance, the fact that both the repository and homepage fields use "openllmetry-js" suggests this is intentional. The package is from "traceloop" organization, not the main OpenTelemetry org, so this could be their intentional repository name. Without access to verify if this GitHub repository actually exists, we shouldn't assume it's a typo. The URL pattern does look unusual and "openllmetry" could be a typo of "opentelemetry". The comment might be helping catch a real issue. However, the consistency between repository and homepage fields, plus the fact this is a new file in a different organization suggests this is their intended repository name. We don't have enough context to be certain this is a mistake. We should delete this comment as we don't have strong evidence that this is actually a typo rather than an intentional repository name.

Workflow ID: wflow_HBpI07CIDMDsOfK2

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

[ellipsis-dev]

Avoid using the generic Function type for the onmessage handler; use a more specific function signature to improve type safety.

Suggested change

	set(this: any, newHandler: Function) {
	set(this: any, newHandler: (event: MessageEvent) => void) {

[nirga]

Hey @narengogi - look at the langchain instrumentation - there are some tricks to instrument sub modules

[narengogi]

Hey @nirga !

1. Is there a problem with any of the keys of this span?
   I'm unable to see these spans in the Traceloop dashboard, however I've confirmed that the spans are exported by using an instance of ConsoleSpanExporter

{
  resource: {
    attributes: {
      'service.name': undefined,
      'telemetry.sdk.language': 'nodejs',
      'telemetry.sdk.name': 'opentelemetry',
      'telemetry.sdk.version': '1.29.0',
      'process.pid': 1349940,
      'process.executable.name': 'node',
      'process.executable.path': '/home/naren/.nvm/versions/node/v20.17.0/bin/node',
      'process.command_args': [
        '/home/naren/.nvm/versions/node/v20.17.0/bin/node',
        '/home/naren/Code/traceloop/mcp-server-test/src/sse-client.js'
      ],
      'process.runtime.version': '20.17.0',
      'process.runtime.name': 'nodejs',
      'process.runtime.description': 'Node.js',
      'process.command': '/home/naren/Code/traceloop/mcp-server-test/src/sse-client.js',
      'process.owner': 'naren',
      'host.name': 'arch-linux',
      'host.arch': 'amd64',
      'host.id': 'd8ee4a281760asdasd'
    }
  },
  instrumentationScope: undefined,
  traceId: '800e70e3d9419c8a42097436d3a03091',
  parentSpanContext: undefined,
  traceState: undefined,
  name: 'mcp.client.response',
  id: 'eda193391c63afaf',
  kind: 2,
  timestamp: 1749072849426000,
  duration: 5335.81,
  attributes: { 'mcp.request.method': 'notifications/initialized' },
  status: { code: 0 },
  events: [],
  links: []
}

2. W.R.T auto-instrumentation,
   I've tried a few combinations including providing the full qualified path to the modules in the final node_modules build, none of which seem to work
   I've tried

• import * as sse from '@modelcontextprotocol/sdk/client/sse.js';
• import * as sse from '@modelcontextprotocol/sdk/client/sse';
• import * as sse from '@modelcontextprotocol/sdk/client/sse.js';
  I'll take a look at this again

[narengogi]

Added tests and a sample app!

[nirga]

@narengogi can you try running the python sample app and compare the span attributes? they should be the same.
In traceloop dashboard, try clicking on the word "Traces" (the title) and then choosing "Spans" from the menu that opens.
I re-run CI - looks like it's failing