Repository navigation
feat: add automation OTEL tracing - #544
Conversation
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Reviewed exact head 6826d93d7724b834b3a89bb9a8f1133db7995765 (feat: add automation OTEL tracing).
State: PR is open, non-draft, label type: feat; no prior reviews; the only issue comment is the bot coverage comment. All check runs on this head are green (unit-tests, backend, Build and Push Automation Image, pr-title, check-pr-artifacts, Validate PR description; the two cleanup checks are skipped). uv run ruff check on the changed files passes, and tests/test_observability.py passes locally (3 passed). The broader tests/test_ingest.py / tests/test_event_router.py suites require the PostgreSQL testcontainers fixture and could not run here (no Docker daemon), which matches the PR's own note.
Material finding (blocks the PR's headline "propagate OTEL trace context into dispatched runs" goal):
The injected trace context is never consumed downstream. inject_trace_context() calls the W3C propagator's inject(carrier), which only writes traceparent/tracestate into the dispatch env. The sandbox side runs the pinned openhands-sdk==1.50.1, whose observability layer reads upstream context solely from the LMNR_SPAN_CONTEXT env var (lmnr _initialize_context_from_env, reached through maybe_init_laminar()); nothing in the SDK, lmnr, or the agent server reads traceparent from the environment. I confirmed this empirically: exporting traceparent before maybe_init_laminar() does not parent a downstream span, while LMNR_SPAN_CONTEXT produced by Laminar.serialize_span_context() does. As written, the dispatch span's trace id is not propagated, so sandbox conversation/agent spans start disconnected traces. The fix is to emit LMNR_SPAN_CONTEXT (e.g. carrier["LMNR_SPAN_CONTEXT"] = Laminar.serialize_span_context()) from inside the active dispatch span rather than relying on propagate.inject.
See the inline comment for the precise location. Everything else in the diff (span/event wiring in ingest, event_router, scheduler, dispatcher, router, and the preset observability metadata) looks consistent with the repository guidance and is a no-op when observability is unconfigured.
🔄 CHANGES REQUESTED
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Re-reviewed exact head 20cb114709ac9da188cb00b197e6b4e802709efe (the PR moved from 6826d93; the author pushed fix: propagate Laminar span context). Scope check: the change belongs in this repository — it instruments the automation service's own webhook/trigger/routing/scheduler/dispatch/callback paths and passes correlation metadata into the SDK conversations it dispatches, consistent with AGENTS.md's ownership of scheduling, dispatch, and run lifecycle.
Previous blocking finding — resolved. My earlier review flagged that inject_trace_context() only wrote the W3C traceparent/tracestate header into the dispatch env, which the sandbox-side SDK (openhands-sdk==1.50.1) never reads; it consumes upstream context solely from LMNR_SPAN_CONTEXT. Commit 20cb114 adds carrier["LMNR_SPAN_CONTEXT"] = Laminar.serialize_span_context() before the W3C injection, and _execute_run_safe now wraps _execute_run in the automation.run.dispatch span, so serialization runs with that span active. I verified the fix end-to-end: injecting inside an active dispatch span yields both LMNR_SPAN_CONTEXT and traceparent, and a fresh process started with those env vars produces a downstream SDK span whose trace id matches the dispatch span — i.e. the propagated context now has a real consumer.
Verification on this head:
tests/test_observability.pypasses locally (4 passed), including the newtest_inject_trace_context_sets_laminar_span_context.uv run ruff checkanduv run pyrighton the changed modules are clean.unit-testsandbackendchecks are green.
One non-blocking CI note: the Build and Push Automation Image (Docker) check on this head failed. It is unrelated to this diff — the failure is an apt-get solver error (Unable to correct problems, you have held broken packages) while installing libpq-dev git in base-image setup step [3/10], before any application code is copied. This PR does not touch containers/Dockerfile, pyproject.toml, or uv.lock, and the same workflow succeeds on other branches. It looks like a transient snapshot/apt issue rather than a regression from this change; a re-run should confirm.
No other material issues found. The added spans/events are no-ops when observability is unconfigured, and the preset observability metadata flows through supported SDK Conversation kwargs.
✅ APPROVED
|
Laminar verification after restarting with the pushed refs shows the webhook/event path is now stitched into the same trace as the run lifecycle. Observed singular trace: Spans seen in that same trace: Key checks:
Older traces from before the restart are still split, which is expected because those runs predate the persisted parent-context changes. This PR comment was created by an AI agent (OpenHands) on behalf of the user. |
|
Additional Laminar verification for a custom automation that manually dispatches conversations: The custom script used Observed trace: The trace contains the webhook and automation lifecycle spans: It also contains two separate manually-created agent conversations in the same trace. They appear as sibling conversation branches under Conversation 1: Conversation 2: For both conversation roots, Laminar association metadata includes the expected automation fields: Laminar path for the spawned conversations shows they are nested under the automation trace: One remaining custom-script integration note: the callback for this custom script was separate because the script did not yet include the same observability headers on its completion callback request. The callback can use the same pattern and include This PR comment was created by an AI agent (OpenHands) on behalf of the user. |
43fe3d5 to
91573cc
Compare
Laminar QA resultsValidated the association tracing path against a local webhook-triggered automation after this branch was deployed locally. Test automation:
Configured associations were successfully emitted on the trace: {
"external.customer_tier": "enterprise",
"external.delivery_id": "local-assoc-test-001",
"external.priority": "high",
"github.issue.key": "OpenHands/automation#543",
"github.issue.number": 543,
"github.repository.full_name": "OpenHands/automation",
"github.sender.login": "local-test-user"
}I was able to discover the trace from the issue-number association alone with a Laminar query matching Observed single-trace waterfall: The association values appeared on the run-created anchor span and the conversation root span. Lower-level SDK spans are still correlated by trace parentage rather than repeating every high-cardinality association. For lookup safety, This comment was created by an AI agent (OpenHands) on behalf of the user. |
91573cc to
e65faaa
Compare
|
Created the SDK follow-up issue with the ready-for-dev label: OpenHands/software-agent-sdk#5464 This comment was created by an AI agent (OpenHands) on behalf of the user. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Re-reviewed the current exact head e65faaa8cd174e8f5a493ee72578ef39d28b6f80 (the branch advanced well past the 20cb114 I last approved; it now adds persisted parent-span propagation, high-cardinality associations, and a cross-repo SDK pin). Scope check: this still belongs in this repository — it instruments the automation service's own webhook/match/route/scheduler/dispatch/callback lifecycle and threads correlation through the SDK conversations it dispatches, matching AGENTS.md ownership of scheduling, dispatch, and run lifecycle.
What I verified on this head:
- The prior
traceparentvsLMNR_SPAN_CONTEXTissue stays fixed:inject_trace_context()writesLMNR_SPAN_CONTEXT(viacurrent_span_context()/Laminar.serialize_span_context()), and that value is copied toOPENHANDS_OBSERVABILITY_PARENT_SPAN_CONTEXTin the dispatch env. I confirmed the env var names match the SDK helper exactly (OPENHANDS_OBSERVABILITY_METADATA/_TAGS/_SPAN_NAME/_PARENT_SPAN_CONTEXT) and thatopenhands.sdk.automation.automation_conversation_kwargsexists at the pinned SDK ref834e23fafcd30b81888cb6e4cafcd9cd1f2a9617. - Persisted parent context is wired consistently: event ingest captures
current_span_context()insideautomation.webhook.receive, stores it on the run (observability_parent_span_context, migration031), and the dispatcher re-attaches withspan(..., parent_span_context=...).complete_runreads theX-OpenHands-Observability-Parent-Span-Contextheader the SDK sends — matching the header name emitted byautomation_observability_headers(). - Focused tests pass locally:
tests/test_observability.py,tests/test_observability_associations.py,tests/test_run_utils.py,tests/test_schemas.py,test_health.py::TestSdkVersionEndpoint,test_test_tarball_setup.py, andtest_migration_history.py(49 passed; the new observability tests 13 passed).ruff checkon the changed modules is clean. - CI for this head is green:
unit-tests,backend,Build and Push Automation Image, description/artifact/title checks all success. - Association config is validated (key charset/length, ≤20 entries, JMESPath compile, scalar/≤1000-char value clamping) and evaluated defensively, so a malformed row cannot break run creation. JMESPath evaluation is not code execution, and outputs are JSON-encoded into env vars rather than shell-interpolated.
Non-blocking risk to flag (not a correctness bug on this head): get_sdk_package_specs() temporarily pins every sandbox's openhands-sdk/openhands-tools/openhands-workspace to git ref 834e23fafcd30b81888cb6e4cafcd9cd1f2a9617, which is the tip of the still-open, currently non-mergeable OpenHands/software-agent-sdk#5462. Because setup.sh resolves this spec on every run, any force-push/GC of that branch would break setup for all automations, and the automation branch must not ship past this pin before the SDK PR merges. The PR documents this as temporary with a tracked follow-up issue; worth confirming the SDK PR lands (or the pin is replaced with a released version) before this merges to main.
No other material issues found.
✅ APPROVED
There was a problem hiding this comment.
I'm an AI agent (Claude, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.
Request changes: the git SDK pin in /sdk-version should not ship as it stands. all-hands-bot's last approval called the pin a non-blocking risk, to be resolved before merge. That has not happened, and since that review main has moved in a way that makes the pin harmful.
-
Sandboxes would install an older SDK than the service.
get_sdk_package_specs()(utils/version.py:14) sends every new prompt, plugin and test-tarball setup tosoftware-agent-sdk@834e23f.- That commit is from before OpenHands/software-agent-sdk#5462 merged, and its
openhands-sdkversion is 1.50.1. #5462 merged on 2026-10-03 as1b2b5c8. That was after v1.51.0, the latest release, so no release contains it yet. The branch has been deleted. mainnow pinsopenhands-sdk==1.51.0(#549).834e23fand v1.51.0 have diverged: the pinned commit lacks #5358, #5406 and the release commit.
After merge,
/sdk-versionwould reportversion: 1.51.0while sandboxes install 1.50.1-based code from git. The pin also replaces PyPI wheels with a clone of the SDK repo and three source builds on every fresh sandbox, whether or not tracing is configured. -
The presets now require an unreleased SDK module.
presets/prompt/sdk_main.py:195andpresets/plugin/sdk_main.py:192importopenhands.sdk.automationat module level, and v1.51.0 does not have it. So droppingpackagesand falling back to==versionwould break new preset runs at import. To fix this, wait for an SDK release that contains #5462, bumpopenhands-sdkandopenhands-workspaceto it, and remove the git pin.
Separately, please update the PR description. The last commit adds a public contract change that the description does not mention: the observability_associations field on create, update, preset and draft requests, on responses and in git-sync YAML; the trigger_event_id column; and migration 031. The description also still lists #5462 as a draft.
What I verified beyond earlier reviews:
- Migration:
031revises030, main's head, and leaves one Alembic head. It uses generic types, and the PostgreSQLCOMMENTs are guarded. - Dependencies: there are no new direct dependencies.
lmnrand OTEL come in throughopenhands-sdk, andjmespathwas already used. - Off by default: tracing stays off unless
LMNR_PROJECT_API_KEYor one of theOTEL_*endpoint variables is set. On main's SDK 1.51.0, the disabled check costs fouros.getenvcalls. On this head's 1.50.1 lock, the SDK'sget_envalso parses.envon every call, so the cheap path depends on mergingmain. - No secrets in traces: span and event attributes carry IDs, names, event source and key, subject key and filter expression. They carry no API keys, prompts or payloads, apart from association values that an automation owner configures (at most 20, each at most 1000 characters).
- Sandbox env: the new env vars reach the sandbox through
_shell_quote. - Merging: the branch merges cleanly with
main, #302, #304 and #305. #517 has since merged with its own031_add_webhook_event_id_header, so this branch's031now needs to become032, revising031.
| SDK_WORKSPACE_PACKAGE_NAME: Final[str] = "openhands-workspace" | ||
| SDK_REPOSITORY_URL: Final[str] = "https://github.com/OpenHands/software-agent-sdk.git" | ||
| # Temporary cross-repo pin for OpenHands/software-agent-sdk#5462 until released. | ||
| SDK_OBSERVABILITY_REF: Final[str] = "834e23fafcd30b81888cb6e4cafcd9cd1f2a9617" |
There was a problem hiding this comment.
This sends every new sandbox to a pre-merge commit of software-agent-sdk#5462, whose openhands-sdk version is 1.50.1. #5462 has since merged as 1b2b5c8, after v1.51.0, so no release contains it yet. With main on 1.51.0 (#549), sandboxes would go back to 1.50.1-based code while /sdk-version reports 1.51.0. Please replace this with an SDK release that includes #5462, and bump pyproject.toml to match, before merging.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
3d9ef05 to
5af1f40
Compare
Use published OpenHands SDK packages instead of the temporary git commit pin.\n\nCo-authored-by: openhands <openhands@all-hands.dev>
5af1f40 to
f6de25f
Compare
|
🚀 Released in 1.19.0. |
4 participants
Summary
OPENHANDS_OBSERVABILITY_METADATAcarries searchable high-cardinality attributes such asautomation.id,automation.run_id, trigger source, conversation ID, sandbox ID, and bash command ID.OPENHANDS_OBSERVABILITY_TAGScarries low-cardinality trace tags such asautomation,automation.trigger:<type>, andautomation.run_trigger:<source>.OPENHANDS_OBSERVABILITY_PARENT_SPAN_CONTEXTis derived from the Laminar parent span context so SDK/agent-server conversation spans can attach to the automation run trace.openhands.sdk.automation.automation_conversation_kwargs(...), so custom automations can use the same helper instead of duplicating env parsing.automation.callback.receivedto the propagated Laminar parent span context from SDK completion callbacks, so callbacks can share the dispatch/conversation trace./sdk-versionto return SDK install package specs and temporarily pin sandbox-installed SDK packages to feat: propagate automation observability context software-agent-sdk#5462 at834e23fafcd30b81888cb6e4cafcd9cd1f2a9617, so automation sandboxes get the SDK/agent-server generic context propagation from that PR.Related SDK PR
Requires draft SDK PR: OpenHands/software-agent-sdk#5462
Current SDK ref used by this automation branch:
Testing
Focused tests that passed:
Result:
Preset template tests:
Result:
Lint/type/syntax checks:
Result:
Result:
Result: exited 0.
Preset helper integration validation:
Result:
Callback trace-parent validation:
Result:
Note:
tests/test_router.py::TestCompleteRun::test_complete_run_uses_propagated_parent_span_contextwas also added, but the local targeted run could not start because Docker/testcontainers could not connect to/var/run/docker.sockin this environment.Attempted dispatcher test that requires the repository PostgreSQL testcontainers fixture:
Result: errored before test execution because this environment has no Docker daemon for testcontainers:
Earlier broader targeted tests had the same Docker limitation after non-container tests passed.
This PR was created by an AI agent (OpenHands) on behalf of the user.