Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openhands-agent-server/openhands/agent_server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ repository.

#### What is sent

Events: `conversation_started`, `conversation_finished`, `conversation_failed`,
Events: `conversation_created`, `conversation_finished`, `conversation_failed`,
Comment thread
malhotra5 marked this conversation as resolved.
`conversation_error`, `request_failed`, `server_started`, `server_stopped` β€” all
prefixed `agent_server.`.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2127,10 +2127,10 @@ async def _maybe_subscribe_telemetry(

The subscriber is attached on *every* path, including rehydration, so
errors and terminal outcomes are always captured. But
``conversation_started`` is emitted only for a genuinely new
``conversation_created`` is emitted only for a genuinely new
conversation: ``_start_event_service`` also runs when an idle
conversation is lazily reloaded and when RUNNING conversations are
recovered after a restart, and counting those as starts would inflate
recovered after a restart, and counting those as creations would inflate
the metric on every server bounce.

Deliberately total: telemetry must never be able to fail conversation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,13 @@ def build(
*,
user_id: str | None = None,
occurred_at: datetime | None = None,
insert_id: str | None = None,
) -> DiagnosticEvent:
return DiagnosticEvent(
event_name=event_name,
schema_version=TELEMETRY_SCHEMA_VERSION,
occurred_at=occurred_at or utc_now(),
insert_id=insert_id,
distinct_id=self.distinct_id(user_id),
runtime=self._runtime,
properties=properties,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class EventName(StrEnum):
SERVER_STARTED = "agent_server.server_started"
SERVER_STOPPED = "agent_server.server_stopped"
CONVERSATION_STARTED = "agent_server.conversation_started"
CONVERSATION_CREATED = "conversation_created"
Comment thread
malhotra5 marked this conversation as resolved.
Outdated
CONVERSATION_FINISHED = "agent_server.conversation_finished"
CONVERSATION_FAILED = "agent_server.conversation_failed"
CONVERSATION_ERROR = "agent_server.conversation_error"
Expand Down Expand Up @@ -245,6 +246,7 @@ class DiagnosticEvent(BaseModel):
event_name: EventName
schema_version: int = TELEMETRY_SCHEMA_VERSION
occurred_at: datetime
insert_id: SafeToken | None = None

distinct_id: Annotated[str, StringConstraints(min_length=1, max_length=256)]
"""Correlation identity, passed through verbatim.
Expand All @@ -271,6 +273,8 @@ def to_payload(self) -> dict[str, object]:
**self.runtime.model_dump(mode="json"),
**self.properties.model_dump(mode="json", exclude={"kind"}),
}
if self.insert_id is not None:
payload["$insert_id"] = self.insert_id
return payload


Expand All @@ -287,6 +291,7 @@ def to_payload(self) -> dict[str, object]:
"platform",
"deferred_init",
"source",
"$insert_id",
"conversation_ref",
"llm_model_family",
"agent_kind",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ def _handle_state_update(self, event: ConversationStateUpdateEvent) -> None:
self._emit_terminal(status)

def emit_started(self) -> None:
"""Emit ``conversation_started``. Called once, at registration."""
"""Emit canonical ``conversation_created`` once at registration."""
try:
properties = m.ConversationStartedProperties(
conversation_ref=self.context.conversation_ref,
Expand All @@ -149,13 +149,14 @@ def emit_started(self) -> None:
)
self.sink.emit(
self.factory.build(
m.EventName.CONVERSATION_STARTED,
m.EventName.CONVERSATION_CREATED,
properties,
user_id=self.context.user_id,
insert_id=(f"conversation_created:{self.context.conversation_ref}"),
)
)
except Exception:
logger.debug("Telemetry failed to emit conversation_started", exc_info=True)
logger.debug("Telemetry failed to emit conversation_created", exc_info=True)

def _emit_terminal(self, status: str) -> None:
if self._terminal_emitted:
Expand Down Expand Up @@ -287,7 +288,7 @@ async def close(self) -> None:
conversation. Emitting unconditionally here produced a
``conversation_finished`` β€” carrying a non-terminal
``terminal_status`` like ``paused`` β€” for a conversation that did
nothing this session, with no matching ``conversation_started``, and
nothing this session, with no matching ``conversation_created``, and
again on every view-then-restart cycle for the same
``conversation_ref``.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ async def test_conversation_service_reads_the_live_sink_not_a_captured_one(
captured at construction, every conversation would see the pre-init NoOp and
emit nothing regardless of consent. Build the service while the sink is still a
NoOp, enable telemetry afterwards, and assert a new conversation still attaches
the subscriber and emits ``conversation_started``.
the subscriber and emits ``conversation_created``.
"""
from uuid import uuid4

Expand Down Expand Up @@ -178,7 +178,7 @@ async def subscribe_to_events(self, subscriber):
)

assert len(event_service.subscribers) == 1
assert m.EventName.CONVERSATION_STARTED in sink.events
assert m.EventName.CONVERSATION_CREATED in sink.events


def test_app_exposes_a_sink_on_state_after_startup(temp_persistence_dir):
Expand Down
7 changes: 5 additions & 2 deletions tests/agent_server/telemetry/test_telemetry_end_to_end.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ async def test_opted_in_session_emits_sanitized_lifecycle_and_error_events():
await asyncio.wait_for(sink.aclose(), timeout=10)

names = [p["event"] for p in exporter.payloads]
assert m.EventName.CONVERSATION_STARTED in names
assert m.EventName.CONVERSATION_CREATED in names
assert m.EventName.CONVERSATION_ERROR in names
assert m.EventName.CONVERSATION_FINISHED in names

Expand Down Expand Up @@ -173,7 +173,10 @@ async def test_opted_in_session_reports_useful_diagnostics():

by_name = {p["event"]: p["properties"] for p in exporter.payloads}

started = by_name[m.EventName.CONVERSATION_STARTED]
started = by_name[m.EventName.CONVERSATION_CREATED]
assert (
started["$insert_id"] == f"conversation_created:{started['conversation_ref']}"
)
assert started["llm_model_family"] == "anthropic"
assert started["tool_count"] == 4

Expand Down
1 change: 1 addition & 0 deletions tests/agent_server/telemetry/test_telemetry_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ def test_property_names_match_the_declared_allowlist():
actual: set[str] = {"schema_version"}
for model in PROPERTY_MODELS:
actual.update(n for n in model.model_fields if n != "kind")
actual.add("$insert_id")

assert actual == set(m.EXPECTED_PROPERTY_NAMES), (
"Diagnostic property set changed. Update EXPECTED_PROPERTY_NAMES "
Expand Down
12 changes: 6 additions & 6 deletions tests/agent_server/telemetry/test_telemetry_subscriber.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,20 +79,20 @@ def make_subscriber(sink, factory, user_id: str | None = "user-1"):
# ── lifecycle ─────────────────────────────────────────────────────────────


async def test_emits_exactly_one_started_event(factory):
async def test_emits_exactly_one_created_event(factory):
sink = CollectingSink()
sub = make_subscriber(sink, factory)

sub.emit_started()
assert sink.names == [m.EventName.CONVERSATION_STARTED]
assert sink.names == [m.EventName.CONVERSATION_CREATED]


def test_started_is_only_emitted_for_genuinely_new_conversations():
"""Regression: ``_start_event_service`` also runs on rehydration.

It is called when an idle conversation is lazily reloaded and when RUNNING
conversations are recovered after a restart. Emitting
``conversation_started`` from all of those would inflate the metric on
``conversation_created`` from all of those would inflate the metric on
every server bounce, so the flag must default to *not* emitting.
"""
import inspect
Expand All @@ -103,7 +103,7 @@ def test_started_is_only_emitted_for_genuinely_new_conversations():
param = sig.parameters["is_new_conversation"]

assert param.default is False, (
"_start_event_service must default to NOT emitting conversation_started; "
"_start_event_service must default to NOT emitting conversation_created; "
"the hydration path relies on that default"
)
assert param.kind is inspect.Parameter.KEYWORD_ONLY
Expand Down Expand Up @@ -156,7 +156,7 @@ async def test_close_is_silent_when_no_run_was_observed(factory):

The subscriber attaches on every _start_event_service path, including the
lazy attach when a user merely views an old conversation. Emitting on close
produced a conversation_finished with no matching conversation_started,
produced a conversation_finished with no matching conversation_created,
repeated on every view-then-restart cycle for the same conversation_ref.
"""
sink = CollectingSink()
Expand Down Expand Up @@ -397,7 +397,7 @@ async def test_disabled_sink_short_circuits_before_building_events(factory):

sub.emit_started()
# emit() itself is a no-op on a disabled sink; nothing is recorded.
assert sink.events == [] or sink.names == [m.EventName.CONVERSATION_STARTED]
assert sink.events == [] or sink.names == [m.EventName.CONVERSATION_CREATED]


# ── identity ──────────────────────────────────────────────────────────────
Expand Down
Loading