feat(observability): allow selecting Laminar instruments - #4434
feat(observability): allow selecting Laminar instruments#4434Shimada666 wants to merge 2 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
Clean, well-scoped change. The LMNR_INSTRUMENTS env var is parsed into a set of Instruments and forwarded to both the Laminar and generic OTLP Laminar.initialize() branches, with the existing initialize-all behavior preserved when the variable is unset. The motivating latency improvement (5.971s to 0.370s median in deferred-init) is compelling and the implementation matches the existing env-var parsing style in the module.
Tests pass (42 passed, as described) and the parametrized test covers both backend branches.
Risk assessment: LOW
The change is additive and opt-in. When LMNR_INSTRUMENTS is unset, behavior is identical to today. The one material finding below is a robustness gap at an import-time call site rather than a correctness bug in the happy path.
Finding
Unhandled ValueError on malformed/invalid instrument values crashes startup. maybe_init_laminar() runs at module import time (agent.py:99, acp_agent.py:118), so any ValueError from Instruments(value.strip()) propagates through import and breaks agent/server startup entirely.
Concrete triggers:
- Trailing/leading/repeated commas:
LMNR_INSTRUMENTS=litellm,->"litellm,".split(",")yields an empty entry ->Instruments("")->ValueError. - Whitespace-only value:
LMNR_INSTRUMENTS=" "->Instruments("")after strip ->ValueError. - A typo or unsupported instrument name ->
ValueError.
An operator typo in a .env or deployment manifest would therefore turn the deferred-init latency win into a hard startup failure with no actionable log line, rather than a degraded-but-running state. This is inconsistent with the sibling _get_int_env helper in the same module, which logs a warning and returns None on a malformed value rather than raising.
Suggested direction (for the author to decide): filter out empty entries after stripping, and/or guard invalid values with a logger.warning(...) + skip (mirroring _get_int_env). This keeps the fail-open behavior expected of an observability hook. The test suite would also benefit from a case covering an empty/invalid entry so the import-time path is protected against regressions.
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed in 6425b2c: empty entries are ignored, unsupported instrument values now log a warning and are skipped, and the regression test covers both cases across both initialization branches. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Elegant, minimal change that solves a real, measured performance problem.
The parsing logic is clean, backward compatibility is preserved (existing behaviour unchanged when LMNR_INSTRUMENTS is unset), and the PR description includes concrete benchmark evidence (93.8% reduction in POST /api/init latency). Tests cover the positive, invalid-value, and empty-token edge cases across both backend paths. Nothing here to block.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Purely additive, opt-in feature behind a new environment variable. WhenLMNR_INSTRUMENTSis not set the call toLaminar.initialize()is identical to the pre-PR behaviour. No public API surface is changed.
VERDICT:
✅ Worth merging: Implementation is tight, evidence is strong, and tests are present.
KEY INSIGHT:
Optional allow-list filtering at init time eliminates the discovery cost of the full Laminar instrument catalog — the right knob in the right place.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
Reviewed the updated head (6425b2c) which addresses the prior review's finding on commit c6fa8b0.
The earlier concern — that malformed LMNR_INSTRUMENTS entries (trailing commas, whitespace-only, or unsupported names) would raise an uncaught ValueError from Instruments(value) during import-time init and crash agent/server startup — is fully resolved in this revision:
- Empty entries are now skipped after stripping (
if not value: continue,laminar.py:110-111). - Invalid values are wrapped in
try/except ValueErrorwith alogger.warning(...)and skipped (laminar.py:112-115), mirroring the fail-open behavior of the sibling_get_int_envhelper. - The new parametrized test case
("litellm,,unsupported,", {"litellm"}, "unsupported")exercises both the empty-entry skip and the invalid-value warning, covering the import-time path that was previously unprotected.
Verified Instruments is a str-valued enum whose ValueError is the correct exception to catch, and that Laminar.initialize accepts instruments as list | set | tuple | None (it internally calls set(instruments)), so passing a set is type-correct. Passing an empty set when all entries are invalid/empty is explicitly documented Laminar behavior ("disable all instruments"), so that edge case is intentional and consistent.
Tests: uv run pytest tests/sdk/observability/test_laminar.py -q -> 44 passed.
Risk assessment: LOW
The change is additive and opt-in. When LMNR_INSTRUMENTS is unset, instruments stays None and behavior is identical to before. Error handling now fails open with warnings rather than crashing startup. No security, correctness, or maintainability concerns remain.
2 participants
HUMAN:
In our E2B deferred-init setup, limiting Laminar to LiteLLM reduced median POST /api/init latency from 5.971s to 0.370s across three fresh sandboxes.
AGENT:
Why
Laminar.initialize()enables every supported auto-instrumentation by default. Applications that only use a subset still pay the package discovery and initialization cost for the full catalog during startup.In an OpenHands Agent Server deferred-init environment, limiting initialization to LiteLLM reduced median
POST /api/initlatency from 5.971s to 0.370s across three fresh E2B sandboxes per configuration (93.8% reduction).Summary
LMNR_INSTRUMENTSenvironment variable using comma-separated Laminar instrument values.Issue Number
N/A
How to Test
Unit coverage:
Result: 42 passed.
Repository checks:
Result: Ruff format, Ruff lint, pycodestyle, pyright, import rules, and tool registration checks passed.
End-to-end benchmark:
ghcr.io/openhands/agent-server:1.41.0-python-based E2B sandbox in deferred-init mode for each run.POST /api/init.LMNR_INSTRUMENTS=litellm.litellmonlyVideo/Screenshots
Not applicable; this is an environment configuration change.
Type
Notes
Companion documentation PR: OpenHands/docs#706
Review feedback addressed in commit 6425b2c.