feat: emit canonical conversation telemetry from agent server - #4459
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
Co-authored-by: openhands <openhands@all-hands.dev>
4782a20 to
aadf917
Compare
malhotra5
changed the title
|
🤖 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
This PR renames the conversation-creation telemetry event from conversation_started to conversation_created and adds a deterministic $insert_id for PostHog retry deduplication. The renaming is well-motivated and the test updates are thorough. However, there is one material issue with the new event name that should be addressed before merge.
Risk Assessment: Medium
The missing agent_server. prefix on the new event name will cause the conversation-creation KPI to land in a different PostHog event namespace than every sibling event, breaking any existing dashboards or queries that filter on agent_server.*. This appears to be an unintentional oversight rather than a deliberate design choice, since the PR's own README update claims "all prefixed agent_server.."
Findings
1. Missing agent_server. prefix on CONVERSATION_CREATED (blocking)
CONVERSATION_CREATED = "conversation_created" (models.py:70) drops the agent_server. prefix that every other event in the EventName enum uses:
CONVERSATION_FINISHED = "agent_server.conversation_finished"CONVERSATION_FAILED = "agent_server.conversation_failed"CONVERSATION_ERROR = "agent_server.conversation_error"SERVER_STARTED = "agent_server.server_started"SERVER_STOPPED = "agent_server.server_stopped"REQUEST_FAILED = "agent_server.request_failed"
The posthog exporter passes event.event_name directly as the PostHog event name (posthog_exporter.py:82), so this event will appear as conversation_created in PostHog rather than agent_server.conversation_created. Any existing PostHog dashboards, insights, or funnels filtering on agent_server.* will silently miss this event. The README update (line 197-199) explicitly states these events are "all prefixed agent_server." -- which is now self-contradicting for conversation_created.
If the unprefixed name is intentional and the prefix is being dropped as part of this canonicalization, the README claim should be updated and the migration should be called out in the PR description. Otherwise, this should be "agent_server.conversation_created".
2. Orphaned CONVERSATION_STARTED enum member (non-blocking)
CONVERSATION_STARTED = "agent_server.conversation_started" (models.py:69) is no longer emitted by any production code. It is only referenced in test_telemetry_sink.py:23 as a convenience for constructing arbitrary test events. Consider removing the dead enum member and updating the test helper to use a live event name (e.g., CONVERSATION_CREATED). Leaving it creates confusion about whether it is still an active event.
3. $insert_id scoped to the KPI event only (observation)
The deterministic $insert_id is set only on conversation_created, which aligns with the stated goal of deduplicating the KPI count. Other events (conversation_finished, conversation_failed, conversation_error) can also be duplicated on PostHog retry but do not receive a deterministic insert_id. This appears to be a deliberate scope decision; flagging it here so it is a conscious choice rather than an omission.
4. Insert-id determinism is per-process (observation, not a defect)
The conversation_ref used in the insert_id is derived via pseudonymize() with a per-process salt (uuid.uuid4().hex in DiagnosticEventFactory.__init__). This means the insert_id is deterministic within a single server process (sufficient for PostHog batch-retry dedup) but not across restarts. This is correct for the stated purpose -- a restarted process would not re-emit conversation_created for an existing conversation since is_new_conversation defaults to False.
Co-authored-by: openhands <openhands@all-hands.dev>
|
🚦 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. |
4 participants
HUMAN:
Canonicalize conversation creation telemetry around the agent-server source of truth.
AGENT:
Why
$insert_idto avoid duplicate KPI counts.How to Test
uv run pytest tests/agent_server/telemetry/test_telemetry_subscriber.py tests/agent_server/telemetry/test_telemetry_end_to_end.py tests/agent_server/telemetry/test_telemetry_schema.py -quv run pytest tests/agent_server/telemetry/test_telemetry_exporter_posthog.py -qSummary
agent_server.conversation_createdtelemetry from agent-server for genuinely new conversations.$insert_idvalues based on the sanitized conversation reference.This PR description was updated by an AI agent (OpenHands) on behalf of the user.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• 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:c96c16a-pythonRun
All tags pushed for this build
About Multi-Architecture Support
c96c16a-python) is a multi-arch manifest supporting both amd64 and arm64c96c16a-python-amd64) are also available if needed