Skip to content

Commit 57db6e8

Browse files
authored
fix(agent): mark successful runs ok and mirror fatal crashes into telemetry
Addresses three review findings: - The sandbox never emits task_complete for successful runs (the terminal "completed" status is decided by the workflow outside), so the root span was ending with unset status on success. The trace builder now marks the root span OK when a turn ends cleanly with end_turn; a run error still always wins and a turn completion arriving after an error cannot flip the status back. - reportFatalError (uncaught exception / unhandled rejection) marked the run failed via the API but never touched telemetry, leaving hard crashes invisible in the new pipeline. It now mirrors an error record (error_source=agent_server_crash) and shuts telemetry down, ending the root span as errored and flushing before the process dies. - The README "Writing logs" section still described the removed otel-log-writer as the preferred full-transcript path to the nonexistent /i/v1/agent-logs endpoint; rewritten to match the sink-based metadata export. Generated-By: PostHog Code Task-Id: 0c511836-2180-455a-9b58-45df2a0661ec
1 parent 8e45ab4 commit 57db6e8

4 files changed

Lines changed: 49 additions & 6 deletions

File tree

packages/agent/README.md

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,12 +217,9 @@ Logs serve two purposes: real-time observability and session resume. Every ACP m
217217

218218
### Writing logs
219219

220-
`SessionLogWriter` (`src/session-log-writer.ts`) is a per-session multiplexer that buffers raw ndJson lines. On flush (auto-scheduled 500ms after writes, or explicit), it dispatches to whichever backend is configured:
220+
`SessionLogWriter` (`src/session-log-writer.ts`) is a per-session multiplexer that buffers raw ndJson lines. On flush (auto-scheduled 500ms after writes, or explicit), it POSTs batched `StoredNotification` entries to the Django API via `PostHogAPIClient.appendTaskRunLog()`; the API stores them in S3 as the task run's `log_url`. This S3 log is the source of truth for the full transcript and for session resume.
221221

222-
- **OTEL** (`src/otel-log-writer.ts`) — preferred path. Creates an OpenTelemetry `LoggerProvider` per session with resource attributes (`task_id`, `run_id`, `device_type`) set once and indexed via `resource_fingerprint`. Each ndJson line is emitted as an OTEL log record with an `event_type` attribute (the ACP method name) and exported via OTLP HTTP to PostHog's `/i/v1/agent-logs` endpoint. Batch flush interval defaults to 500ms.
223-
- **Legacy S3** — falls back to `PostHogAPIClient.appendTaskRunLog()`, which POSTs batched `StoredNotification` entries to the Django API. The API stores them as the task run's `log_url`.
224-
225-
Both backends can be active simultaneously — OTEL for fast indexed queries, S3 for full log download.
222+
Independently of that flush cycle, the writer tees every parsed non-chunk entry to optional `SessionLogSink`s at append time. In cloud, `OtelRunTelemetry` (`src/otel-telemetry.ts`) is wired as a sink and exports an allowlisted metadata subset (run/turn/tool lifecycle, usage, errors — never message content or tool arguments) to PostHog Logs, plus an APM trace per run when configured. See "Optional run telemetry" above for the env vars and behavior. Sink failures can never break S3 log persistence.
226223

227224
### Resuming from logs
228225

packages/agent/src/otel-telemetry.test.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,9 @@ describe("OtelRunTelemetry", () => {
483483
RUN_ID,
484484
makeEntry("_posthog/turn_complete", { stopReason: "end_turn" }),
485485
);
486-
telemetry.append(RUN_ID, makeEntry("_posthog/task_complete", {}));
486+
// Deliberately no task_complete: production never emits it (the
487+
// terminal "completed" status is decided outside the sandbox), so the
488+
// root span's OK must come from the clean end_turn above.
487489
}
488490

489491
it("builds a run trace: root span, turn span, tool span", async () => {
@@ -578,6 +580,12 @@ describe("OtelRunTelemetry", () => {
578580
error: "gateway exploded",
579581
}),
580582
);
583+
// A turn completion arriving after the error must not flip the run
584+
// back to OK.
585+
telemetry.append(
586+
RUN_ID,
587+
makeEntry("_posthog/turn_complete", { stopReason: "end_turn" }),
588+
);
581589

582590
await telemetry.shutdown();
583591

packages/agent/src/otel-trace-builder.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ export interface RunTraceBuilderConfig {
4141
* the corresponding log record should be emitted under, so logs and spans
4242
* cross-link in the UI via trace_id/span_id.
4343
*
44+
* Root span status: OK once a turn ends cleanly (`end_turn`) or on
45+
* task_complete, ERROR on a run error (which always wins), unset when the run
46+
* ends without either (cancelled / timed out).
47+
*
4448
* Spans carry the same allowlist stance as the log export: lifecycle, status,
4549
* usage, and identifiers only — never prompts, tool arguments, or output.
4650
*/
@@ -162,6 +166,13 @@ export class RunTraceBuilder {
162166
typeof params.stopReason === "string" ? params.stopReason : undefined;
163167
this.closeOpenTools(time);
164168
this.closeTurn({ stopReason, errored: stopReason === "error" }, time);
169+
// The sandbox never emits task_complete for successful runs (the terminal
170+
// "completed" status is decided by the workflow outside), so a cleanly
171+
// finished turn is the success signal for the run. A later error still
172+
// wins via rootErrored/handleError.
173+
if (stopReason === "end_turn" && !this.rootErrored) {
174+
this.rootSpan.setStatus({ code: SpanStatusCode.OK });
175+
}
165176
return context;
166177
}
167178

packages/agent/src/server/agent-server.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -979,6 +979,33 @@ export class AgentServer {
979979
stopError,
980980
);
981981
}
982+
983+
// Mirror the crash into run telemetry and shut it down (ends the root
984+
// span as errored and flushes) - the process is about to die, so nothing
985+
// else will get this record out.
986+
try {
987+
const session = this.session;
988+
if (session?.telemetry) {
989+
session.telemetry.append(session.payload.run_id, {
990+
type: "notification",
991+
timestamp: new Date().toISOString(),
992+
notification: {
993+
jsonrpc: "2.0",
994+
method: POSTHOG_NOTIFICATIONS.ERROR,
995+
params: {
996+
source: "agent_server_crash",
997+
error: `Agent server crashed: ${errorMessage}`,
998+
},
999+
},
1000+
});
1001+
await session.telemetry.shutdown();
1002+
}
1003+
} catch (telemetryError) {
1004+
this.logger.error(
1005+
"Failed to flush telemetry after fatal error",
1006+
telemetryError,
1007+
);
1008+
}
9821009
}
9831010

9841011
private authenticateRequest(

0 commit comments

Comments
 (0)