Add conversation attribution to Laminar traces#3580
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED Behavioral default changes detectedThese public
|
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
Functional QA verified the Laminar attribution changes work for SDK local and remote conversation creation; CI is not fully green yet.
Does this PR achieve its stated goal?
Yes. The PR set out to add conversation attribution to Laminar traces by forwarding user_id for remote conversations and attaching conversation tags as root-span attributes. Exercising the SDK before/after showed local conversation tags now reach the Laminar span attributes, and remote conversation creation now sends user_id alongside tags in the /api/conversations payload.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully with uv 0.11.19 and Python 3.13.13 |
| CI Status | PR Description Check), 1 skipped, 7 pending at review time |
| Functional Verification | ✅ Local Laminar span attributes and remote create payload verified before/after |
Functional Verification
Test 1: Local SDK conversations attach automation tags to the Laminar root span
Step 1 — Establish baseline on origin/main (without the fix):
Ran git switch --detach origin/main && uv run python - <<'PY' ... PY, creating a real Conversation(...) with tags={"automationid": "auto-1", "automationrunid": "run-1"} while intercepting Laminar.start_span with an in-process fake span to observe SDK behavior without an external Laminar account.
Relevant output:
local_span_attributes= {}
This shows the old SDK accepted conversation tags but did not attach them to the Laminar root span.
Step 2 — Apply the PR's changes:
Checked out neubig/automation-laminar-attribution at 6c2e02b017aba72dbbfdcdce79595392eb55c994.
Step 3 — Re-run with the fix in place:
Ran the same SDK conversation creation path on the PR branch.
Relevant output:
local_span_attributes= {'conversation.tags.automationid': 'auto-1', 'conversation.tags.automationrunid': 'run-1'}
This confirms local conversation tags are now converted to conversation.tags.<key> span attributes, matching the PR goal.
Test 2: Remote SDK conversations forward user_id and tags in the create payload
Step 1 — Establish baseline on origin/main (without the fix):
Ran git switch --detach origin/main && uv run python - <<'PY' ... PY, creating a real RemoteConversation through the public Conversation(...) API against a local HTTP server that captured the actual POST /api/conversations request.
Relevant output:
remote_conversation_created= RemoteConversation
payload_user_id= None
payload_tags= {'automationid': 'auto-1'}
This shows the old SDK forwarded tags but dropped the caller-provided user_id, preventing the server from attributing the Laminar trace to that user.
Step 2 — Apply the PR's changes:
Checked out neubig/automation-laminar-attribution at 6c2e02b017aba72dbbfdcdce79595392eb55c994.
Step 3 — Re-run with the fix in place:
Ran the same remote SDK conversation creation path on the PR branch.
Relevant output:
remote_conversation_created= RemoteConversation
payload_user_id= user-42
payload_tags= {'automationid': 'auto-1'}
This confirms the remote create request now includes both user_id and tags, so agent-server has the attribution fields needed for Laminar trace user/session filtering.
Issues Found
None from functional QA. CI is not fully green yet: PR Description Check/Validate PR description is failing and 7 checks were still pending when reviewed.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Add conversation attribution to Laminar traces
🟢 Good taste - Elegant, minimal change that solves a real observability problem.
Summary
This PR adds two pieces of attribution plumbing to connect automation conversations with their owning users in Laminar traces:
-
Tags as span attributes: Conversation tags (e.g.,
automationid,automationrunid) are now attached asconversation.tags.<key>attributes on the root span, enabling filtering in Laminar. -
User ID forwarding: Remote conversations now forward
user_idin the create payload so the agent server can callLaminar.set_trace_user_id().
Analysis
[IMPROVEMENT OPPORTUNITIES] (minor - worth considering)
- [openhands-sdk/openhands/sdk/conversation/base.py, Line 162-173] Simplification: The
if attributes:/else:branching creates duplicatestart_root_span()calls. Sincestart_root_span()already acceptsattributes=None, this can be simplified to always passattributes=_conversation_tag_attributes(tags). The conditional adds no value.
# Current (duplicate calls):
if attributes:
self._observability_root_span = start_root_span(..., attributes=attributes)
else:
self._observability_root_span = start_root_span(...)
# Simplified:
self._observability_root_span = start_root_span(
"conversation",
session_id=session_id,
user_id=user_id,
attributes=_conversation_tag_attributes(tags),
)This is a stylistic suggestion only - the code is correct and functional as-is.
What's Good
- Clean separation of concerns:
_conversation_tag_attributes()helper keeps the prefix logic in one place - Backward-compatible: all new parameters have
| Nonedefaults - Appropriate test coverage with mocked unit tests + functional QA verification
- No breaking changes to existing APIs
- Correct use of
contextlib.suppress(Exception)for Laminar calls (fail silently, don't crash application)
Risk Assessment
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The change is purely additive observability plumbing. It adds optional parameters with safe defaults, makes no breaking changes, and has appropriate test coverage. The external Laminar library calls are wrapped with exception suppression to prevent crashes.
VERDICT: ✅ Worth merging
Core logic is sound. The conditional simplification is a minor style suggestion that doesn't block approval.
KEY INSIGHT:
The _conversation_tag_attributes() helper provides a clean abstraction for the attribute prefix convention, making it easy to change the naming scheme in one place if needed.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| attributes = _conversation_tag_attributes(tags) | ||
| if attributes: | ||
| self._observability_root_span = start_root_span( | ||
| "conversation", | ||
| session_id=session_id, | ||
| user_id=user_id, | ||
| attributes=attributes, | ||
| ) | ||
| else: | ||
| self._observability_root_span = start_root_span( | ||
| "conversation", session_id=session_id, user_id=user_id | ||
| ) | ||
|
|
There was a problem hiding this comment.
| attributes = _conversation_tag_attributes(tags) | |
| if attributes: | |
| self._observability_root_span = start_root_span( | |
| "conversation", | |
| session_id=session_id, | |
| user_id=user_id, | |
| attributes=attributes, | |
| ) | |
| else: | |
| self._observability_root_span = start_root_span( | |
| "conversation", session_id=session_id, user_id=user_id | |
| ) | |
| self._observability_root_span = start_root_span( | |
| "conversation", | |
| session_id=session_id, | |
| user_id=user_id, | |
| attributes=_conversation_tag_attributes(tags), | |
| ) |
nit. Moreover, I think there is a test to update
| # OTel context; we'll restore it on every entry point via ``use_span``. | ||
| self.span = Laminar.start_span(name) | ||
| if attributes: | ||
| with contextlib.suppress(Exception): |
There was a problem hiding this comment.
I suggest to move the contextlib.suppress inside the loop.
3 participants
HUMAN:
Adds attribution plumbing so automation conversations can be tied back to their owning user in Laminar and filtered by conversation tags.
AGENT:
Why
Automation conversations currently carry automation context in product metadata, but their Laminar traces do not reliably receive the owning user ID or automation tags. This makes abuse triage require joins outside Laminar.
Summary
user_idinRemoteConversationcreate requests so the agent server can callLaminar.set_trace_user_id().conversation.tags.<key>attributes.Issue
Closes #3587
This PR description update was created by an AI agent (OpenHands) on behalf of Graham Neubig.
How to Test
git diff --checksdk-tests,agent-server-tests,cross-tests,pre-commit, and API compatibility checks are passing on this PR.Video/Screenshots
N/A: observability plumbing change with regression tests.
Type
Notes
This pairs with OpenHands/automation#169, which passes the automation owner user ID into SDK conversation creation.
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:6c2e02b-pythonRun
All tags pushed for this build
About Multi-Architecture Support
6c2e02b-python) is a multi-arch manifest supporting both amd64 and arm646c2e02b-python-amd64) are also available if needed