Skip to content

refactor: move inline telemetry to monkey patching in @blaxel/telemetry#261

Open
devin-ai-integration[bot] wants to merge 5 commits into
mainfrom
devin/1772746138-telemetry-monkey-patching
Open

refactor: move inline telemetry to monkey patching in @blaxel/telemetry#261
devin-ai-integration[bot] wants to merge 5 commits into
mainfrom
devin/1772746138-telemetry-monkey-patching

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Mar 5, 2026

refactor: move inline telemetry to monkey patching in @blaxel/telemetry

Summary

Separates telemetry concerns from business logic in @blaxel/core by moving all inline startSpan calls into @blaxel/telemetry via prototype monkey patching — mirroring the pattern already used in sdk-python's BlaxelCoreInstrumentor.

Core changes (@blaxel/core):

  • Removed startSpan/BlaxelSpan imports and all inline span creation from BlAgent.run(), BlJob.run(), McpTool.listTools(), McpTool.call(), and BlaxelMcpServerTransport message/send handling
  • Exported BlAgent, BlJob classes and re-exported McpTool from the tools barrel so the telemetry package can access prototypes

Telemetry changes (@blaxel/telemetry):

  • New instrumentation/blaxel_core.ts with instrumentBlaxelCore() that monkey-patches all the above methods with span tracking
  • Called from TelemetryManager.instrumentApp() alongside existing HTTP instrumentation

Review & Testing Checklist for Human

  • MCP server span lifecycle regression: The original code tracked per-message spans across receive→send using a Map<string, BlaxelSpan>. The new approach wraps onmessage and send() independently — cross-request span correlation is lost. Verify this is acceptable.
  • McpTool.call() error logging removed: The original call() had a catch block with logger.error(...) for MCP tool call failures. This was removed along with the telemetry code and NOT restored. This is a logging regression separate from telemetry.
  • (this as any).name in McpTool patches: name is a private property on McpTool. The monkey patch bypasses TypeScript's type system to access it. Confirm this is the preferred approach vs. making it public.
  • onmessage property descriptor patching: The MCP server patch uses Object.defineProperty to intercept the setter. Verify this interacts correctly with the MCP SDK's usage pattern (sets onmessage before calling start()).
  • End-to-end telemetry verification: Deploy an agent/job/tool with @blaxel/telemetry enabled and verify spans appear correctly in your observability backend. CI alone cannot verify this.

Notes

  • jobs/start.ts still imports flush() from telemetry — this is intentional as it's cleanup logic, not span tracking
  • The msg as never cast in patchMcpServer is a workaround for JSONRPCMessage being a private interface in mcp/server.ts
  • Build passes cleanly across all packages

Link to Devin session: https://app.devin.ai/sessions/2f5f61caaf1c4c4685b5037c7b10f14e
Requested by: @cploujoux


Open with Devin

Note

Moves all inline startSpan telemetry from @blaxel/core (BlAgent, BlJob, McpTool, BlaxelMcpServerTransport) into @blaxel/telemetry via prototype monkey-patching, mirroring the Python SDK's BlaxelCoreInstrumentor pattern. Classes are exported from @blaxel/core so the telemetry package can access their prototypes.

Written by Mendral for commit 6b15edd.

- Remove startSpan/BlaxelSpan imports and inline span creation from:
  - BlAgent.run() in agents/index.ts
  - BlJob.run() in jobs/jobs.ts
  - McpTool.listTools() and McpTool.call() in tools/mcpTool.ts
  - BlaxelMcpServerTransport message/send handling in mcp/server.ts
- Export BlAgent, BlJob classes and re-export McpTool from barrel
- Create instrumentation/blaxel_core.ts in @blaxel/telemetry with monkey
  patching for all removed telemetry via prototype method wrapping
- Call instrumentBlaxelCore() from TelemetryManager.instrumentApp()

Co-Authored-By: cploujoux <cploujoux@blaxel.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

…oved with telemetry

Co-Authored-By: cploujoux <cploujoux@blaxel.ai>
mendral-app[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 6 additional findings.

Open in Devin Review

The tracedHandler was calling handler(message) synchronously and
immediately ending the span, but the MCP SDK's onmessage handler is
async. Use await Promise.resolve(handler(message)) to properly wait
for the handler to complete before ending the span.

Co-Authored-By: cploujoux <cploujoux@blaxel.ai>
mendral-app[bot]

This comment was marked as outdated.

devin-ai-integration[bot]

This comment was marked as resolved.

mendral-app[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@mendral-app mendral-app Bot left a comment

Choose a reason for hiding this comment

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

🤖 Code Review

Assessment ✅

All three previous comments have been addressed:

  1. Critical bug fixedthrow err restored in the onmessage catch block (commit 6b15edd), so handler errors are no longer silently swallowed.
  2. Async handler fixedawait Promise.resolve(handler(message)) correctly awaits the async handler before ending the span (commit 90b488e).
  3. Double response.text() consumption — implicitly fixed as a side effect of the refactor.

The incremental diff in this update is minimal and correct. No new issues introduced.

Note

Tag @mendral-app with feedback or questions. View session

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.

1 participant