Skip to content

Add Modal Sandbox instrumentation#281

Open
laminar-coding-agent[bot] wants to merge 14 commits into
mainfrom
agent/lam-1441/modal-instrumentation-new/e083af
Open

Add Modal Sandbox instrumentation#281
laminar-coding-agent[bot] wants to merge 14 commits into
mainfrom
agent/lam-1441/modal-instrumentation-new/e083af

Conversation

@laminar-coding-agent

@laminar-coding-agent laminar-coding-agent Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add OpenTelemetry instrumentation for Modal Sandbox create and exec methods
  • Capture spans with command, sandbox ID, timeout, and workdir attributes
  • Emit stdout/stderr as OTel log records via tee-wrapped iterators (logs are emitted as the user reads output without consuming the data)
  • Handle Modal's synchronicity pattern by instrumenting both public Sandbox and internal _Sandbox classes

Implementation

  • New instrumentation module at opentelemetry/instrumentation/modal/ following the Daytona instrumentation pattern
  • ModalSandboxInstrumentor registered in Instruments enum and INSTRUMENTATION_INITIALIZERS
  • _TeeStreamIterator wraps ContainerProcess.stdout/stderr to emit logs transparently

Test plan

  • ModalSandboxInstrumentor class instantiates and reports correct scope/dependencies
  • All 4 wrapped functions (Sandbox.create, Sandbox.exec, _Sandbox.create, _Sandbox.exec) instrumented successfully via wrapt
  • End-to-end test: created Modal sandbox, executed command, read stdout/stderr — all data passed through to user correctly
  • Verified modal.sandbox.create and modal.sandbox.exec spans ingested into ClickHouse with correct input/output attributes
  • Verified modal.log.stdout and modal.log.stderr logs ingested into ClickHouse with correct body and attributes
  • Existing SDK tests (test_initialize.py) pass without regression

🤖 Generated with Claude Code


Note

Medium Risk
Adds a new auto-instrumentor and modifies shared tracing/log-emission utilities, including monkeypatching Modal ContainerProcess stream access, which could affect runtime behavior across different Modal versions. Changes also refactor Daytona log emission to a shared helper, so regressions could impact existing sandbox log capture.

Overview
Adds Modal Sandbox auto-instrumentation: new ModalSandboxInstrumentor wraps Sandbox.create/exec (sync + async) to emit spans with input/output attributes, and tees ContainerProcess.stdout/stderr so reading output produces modal.log.stdout/modal.log.stderr OTel log records.

Refactors sandbox log emission by introducing shared shared/log_emission.py (common LogStream + emit_log) and updating Daytona to use it. Extends shared/utils.py with safe_* helpers (no-op spans, safe end/is_recording/context) to make wrappers resilient to Laminar/OTel failures, and registers the new instrumentor via Instruments.MODAL and a corresponding initializer.

Reviewed by Cursor Bugbot for commit 7b3ead5. Bugbot is set up for automated code reviews on this repo. Configure here.

Instrument Modal Sandbox.create and Sandbox.exec methods to capture
traces and logs. The instrumentation handles Modal's synchronicity
pattern by wrapping both the public Sandbox class and internal _Sandbox
class separately.

Key features:
- Sandbox.create spans with sandbox ID, timeout, and config attributes
- Sandbox.exec spans with command and workdir attributes
- stdout/stderr log emission via tee-wrapped iterators that emit OTel
  logs as the user reads output, without consuming the data
- Both sync and async code paths instrumented

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread src/lmnr/opentelemetry_lib/tracing/instruments.py Outdated
Rainhunter13 and others added 2 commits April 10, 2026 15:56
Move MODAL after MISTRAL in both the Instruments enum and
INSTRUMENTATION_INITIALIZERS dict to maintain alphabetical order.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The async iterator protocol requires __aiter__ to be a regular
(synchronous) method that returns self. Using async def causes it to
return a coroutine object, breaking async for iteration in Python 3.10+.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ility

_TeeStreamIterator only implemented iterator protocols but not other
methods like read() that Modal's StreamReader exposes. This caused
AttributeError when users called process.stdout.read(). Adding
__getattr__ transparently proxies all non-iterator attribute access
to the original stream.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/modal/wrappers.py Outdated
Compute the command string once in _wrap_exec/_awrap_exec and pass it
to _set_exec_request_attributes instead of computing it independently
in both places.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Rainhunter13

Copy link
Copy Markdown
Member

@laminar-coding-agent

explain what is internal _Sandbox class and why instrumenting it brings any value

…ented

Modal's `synchronicity` library generates separate sync/async class pairs
where `_Sandbox` (internal async) and `Sandbox` (public sync) have entirely
distinct method objects. Patching one does not affect the other, so both
must be instrumented to capture all traces.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@laminar-coding-agent

Copy link
Copy Markdown
Contributor Author

What is _Sandbox?

Modal uses the synchronicity library to auto-generate sync/async class pairs from a single implementation. _Sandbox is the internal async implementation class, while Sandbox is the public sync wrapper exposed to users.

Why instrument both?

synchronicity generates entirely separate method objects for each class:

  • Sandbox.exec is a functools.partial
  • Sandbox.create is a FunctionWithAio
  • Neither shares a reference with _Sandbox.exec / _Sandbox.create

This means patching _Sandbox alone does NOT intercept calls made through the public Sandbox API (and vice versa). If we only instrumented Sandbox, async users calling _Sandbox methods directly (or Modal internals routing through _Sandbox) would produce no traces. If we only instrumented _Sandbox, sync callers using Sandbox would be missed.

We must instrument both classes to capture traces regardless of whether user code calls the sync or async interface.

I've updated the module docstring and inline comments in 4e13c23 to explain this clearly.

@laminar-coding-agent

Copy link
Copy Markdown
Contributor Author

The CLAUDE.md already contains a well-written section on "Synchronicity-based SDKs (e.g. Modal)" that covers the key insight about _Sandbox vs Sandbox. The task was adding code comments to explain the same concept — no new non-obvious knowledge was discovered beyond what's already documented. Skipping the CLAUDE.md update.

@Rainhunter13

Copy link
Copy Markdown
Member

@laminar-coding-agent

async users calling _Sandbox methods directly

It looks like Sandbox is a wrapper around Sandbox to handle both sync and async contexts. Users are using only public Sandbox class. Let's drop the _Sandbox instrumentation.

Sandbox is a wrapper around _Sandbox that users interact with directly.
Instrumenting _Sandbox separately is unnecessary since users only use
the public Sandbox API.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@laminar-coding-agent

Copy link
Copy Markdown
Contributor Author

Done. Changes pushed:

  • __init__.py: Removed the two _Sandbox WrappedFunctionSpec entries (async create/exec), updated the import to drop _awrap_create/_awrap_exec, and simplified the module docstring.
  • wrappers.py: Removed _awrap_create and _awrap_exec async wrapper functions.
  • CLAUDE.md: Removed the "Synchronicity-based SDKs" section about instrumenting _Sandbox separately.

Only the public Sandbox.create and Sandbox.exec (sync) are now instrumented.

Rainhunter13 and others added 3 commits April 10, 2026 17:10
Modal's StreamReader.read() is a primary usage pattern but was delegated
via __getattr__ without emitting OTel logs. This adds an explicit read()
override that emits log records, matching the behavior of iteration.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Without @dont_throw, an exception raised inside _record_exception (e.g.,
from a broken __str__ on the user exception) would bubble up past the
call site's span.end(), leaking the active span context and masking the
original user-facing exception with an instrumentation error.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f02d1eb. Configure here.

cursor Bot and others added 3 commits April 23, 2026 13:25
Modal and Daytona wrappers each carried nearly identical copies of the
LogStream enum and a provider-scoped _emit_log function that only
differed by the attribute/event prefix. Both now share
shared/log_emission.py (emit_log parameterized by provider name), and
the two wrapper modules bind their provider with functools.partial so
call-sites are unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
_TeeStreamIterator.__next__ previously called next(self._original), which
only works when the original stream is itself an iterator. If Modal's
StreamReader.__iter__ ever returns a fresh iterator (the general iterable
protocol), next() would raise TypeError. Now we cache the iterator
produced by iter()/__aiter__() and advance that, which works for both
self-iterator and fresh-iterator conventions.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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