fix: flush telemetry for short-lived runs#93
Conversation
📝 WalkthroughWalkthroughAdds a ChangesForce Flush Telemetry
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime as OpenCode Runtime
participant Plugin as Plugin (index.ts)
participant Otel as otel.ts
participant Providers as OTEL Providers
Runtime->>Plugin: session.idle / session.error
Plugin->>Plugin: flushTelemetry(reason)
Plugin->>Otel: forceFlushOtel(providers)
Otel->>Providers: forceFlush() (meter, logger, tracer)
Providers-->>Otel: settled results
Runtime->>Plugin: message (assistant, completed)
Plugin->>Plugin: flushTelemetry("message.completed")
Plugin->>Otel: forceFlushOtel(providers)
Otel->>Providers: forceFlush() (meter, logger, tracer)
Runtime->>Plugin: shutdown()
Plugin->>Plugin: shuttingDown = true
Plugin->>Otel: forceFlushOtel(providers)
Plugin->>Providers: shutdown meter/logger/tracer
Related issues: Suggested labels: bug, telemetry Suggested reviewers: DEVtheOPS 🐰 A rabbit taps the OTEL drum, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/opencode give this a quick look |
|
Clean change — addresses #92 with minimal surface area.
No issues from me. LGTM. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/otel.test.ts (1)
126-140: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing coverage for partial-failure resilience.
forceFlushOtelusesPromise.allSettledspecifically so one provider's rejectedforceFlush()doesn't stop the others or throw. This test only covers the happy path; add a case where one provider'sforceFlushrejects to confirm the others still flush and the function resolves without throwing.✅ Suggested additional test
await forceFlushOtel(fakeProviders) expect(calls.sort()).toEqual(["logs", "metrics", "traces"]) }) + + test("continues flushing other providers when one rejects", async () => { + const calls: string[] = [] + const fakeProviders = { + meterProvider: { forceFlush: async () => { throw new Error("boom") } }, + loggerProvider: { forceFlush: async () => { calls.push("logs") } }, + tracerProvider: { forceFlush: async () => { calls.push("traces") } }, + } as unknown as OtelProviders + + await expect(forceFlushOtel(fakeProviders)).resolves.not.toThrow() + expect(calls.sort()).toEqual(["logs", "traces"]) + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/otel.test.ts` around lines 126 - 140, The current forceFlushOtel test only covers the all-success path; add a new case in otel.test.ts under forceFlushOtel that makes one OtelProviders forceFlush reject while the others resolve, then verify the remaining providers still run and that forceFlushOtel completes without throwing. Use the existing forceFlushOtel helper and the fakeProviders pattern to assert Promise.allSettled resilience rather than fail-fast behavior.src/index.ts (1)
338-340: 🚀 Performance & Scalability | 🔵 TrivialConsider the export volume from flushing on every completed assistant message.
flushTelemetry("message.completed")forces a full export of all three signals on every assistant turn. In long-lived interactive (TUI) sessions this fires repeatedly, negating the batching provided byBatchSpanProcessor/BatchLogRecordProcessor/PeriodicExportingMetricReaderand increasing OTLP request volume. It is also partly redundant with thesession.idleflush that fires shortly after (Line 287). Since the PR goal is reliably capturing telemetry for short-livedopencode run,session.idlealone may already cover that path; consider whether the per-message flush is needed or should be debounced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 338 - 340, The per-assistant-turn flush in the message handling flow causes excessive full telemetry exports and duplicates the later session-idle flush. In the `src/index.ts` message completion path (`info.role === "assistant" && info.time?.completed`), remove or debounce `flushTelemetry("message.completed")` so telemetry is not exported on every completed assistant message, and rely on the existing `session.idle` flush path for the short-lived `opencode run` case unless a specific need for per-message flushing remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/index.ts`:
- Around line 338-340: The per-assistant-turn flush in the message handling flow
causes excessive full telemetry exports and duplicates the later session-idle
flush. In the `src/index.ts` message completion path (`info.role === "assistant"
&& info.time?.completed`), remove or debounce
`flushTelemetry("message.completed")` so telemetry is not exported on every
completed assistant message, and rely on the existing `session.idle` flush path
for the short-lived `opencode run` case unless a specific need for per-message
flushing remains.
In `@tests/otel.test.ts`:
- Around line 126-140: The current forceFlushOtel test only covers the
all-success path; add a new case in otel.test.ts under forceFlushOtel that makes
one OtelProviders forceFlush reject while the others resolve, then verify the
remaining providers still run and that forceFlushOtel completes without
throwing. Use the existing forceFlushOtel helper and the fakeProviders pattern
to assert Promise.allSettled resilience rather than fail-fast behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98fa8af1-ce25-4d24-910d-920a3ed628a4
📒 Files selected for processing (3)
src/index.tssrc/otel.tstests/otel.test.ts
1 participant
Summary
Fixes #92
Validation
Summary by CodeRabbit
New Features
Bug Fixes