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
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. |
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.