[OPIK-7279] [SDK] fix: convert trace lifecycle to upsert-only (no trace.update()/end()) - #25
Merged
Merged
Conversation
QA of opik-hermes 0.1.1 surfaced the Opik SDK warning on every Hermes message: "Calling Trace.update() shortly after creation with batching enabled may cause data loss." That warning is the symptom of violating the codebase-wide upsert-only rule for integrations: when an entity finishes, re-send the same id with the finished payload rather than mutating it via update()/end(). - lifecycle.py finish_trace: finalize via a single client.trace(id=..., output=..., end_time=...) re-send instead of trace.update()+trace.end(). - state.py evict_stale_locked: finalize evicted traces via the same upsert (id + end_time) instead of trace.end(); fail-open if no client. - Spans were already upsert-compliant and are left untouched. Tests: FakeOpik.trace() now models real Opik's upsert coalescing; FakeTrace update()/end() raise so a regression fails loudly. E2E asserts the batching warning is absent and the finalized trace carries output + end_time. Implements OPIK-7279. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The real-Opik E2E caught an NA-trace regression: the finalize upsert re-sent only id+output+end_time. But client.trace(id=...) builds a full CreateTraceMessage — the omitted name/thread_id/input went as null and the backend's last-write-wins merge clobbered the create, landing an NA trace (name=None/thread_id=None). start_time also drifted to a fresh now(). The SDK's own Trace.update() docstring documents the fix: re-send the FULL payload with the same id. So capture the create kwargs on TraceState (pinning start_time) and replay them + output/end_time on both the finish and eviction re-sends. Also made the mock-Opik journal assertion faithful to real Opik (last-write-wins including nulls) so the cheaper per-PR E2E now reproduces and catches this NA regression, and added unit guards asserting the finalize re-send carries name/thread_id/input and the create's start_time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
QA of opik-hermes 0.1.1 surfaced the Opik SDK warning on every Hermes message:
That warning is the symptom of violating the codebase-wide upsert-only rule for Opik integrations: when an entity starts, send the partial (started) entity; when it finishes, re-send the same entity (same id) with the full/finished payload and let the SDK's batching layer coalesce the two sends into one row. An
update()/end()shortly after create is the anti-pattern. So this is not "silence a warning" — it's adopting the upsert pattern for the trace lifecycle, which removes the warning as a side effect.Spans were already upsert-compliant (created fully-formed in a single
trace.span()call) and are left untouched — only the trace lifecycle needed the change.Changes
observability/opik/lifecycle.py—finish_tracenow finalizes via a singleclient.trace(id=state.trace.id, ...)re-send instead oftrace.update(output=...)+trace.end(). The re-send replays the full create payload (name / thread_id / input / start_time / metadata / tags) plusoutput+end_time— not just output+end_time.client.trace(id=...)builds a wholeCreateTraceMessage, so any omitted field is sent as null and the backend's last-write-wins merge would clobber the create, landing an NA trace (name/thread_id → None).start_timeis pinned at create time so the re-send carries the same value, not a freshnow().observability/opik/state.py— carries the create kwargs onTraceState.create_kwargs;evict_stale_lockedfinalizes evicted traces via the same full-payload upsert re-send (+end_time) instead oftrace.end(); importsget_clientand fails open if no client is available.observability/opik/hooks.py— comment refresh only, no behavior change.The create is still flushed as its own batch before the finalize re-send, so the NA-trace race (name/thread/input lost to batch coalescing on a fast turn) remains guarded.
Change checklist
Issues
Testing
FakeOpik.trace()now models real Opik's upsert coalescing — a call carrying a knownidmerges into the existing trace as a finalize re-send rather than minting a new row.FakeTrace.update()/.end()now raise, so any regression back to the forbidden pattern fails loudly. Addedtest_finalize_is_upsert_not_update_or_endandtest_finalize_resend_replays_full_create_payload(guards the NA-trace regression: the finalize upsert must replay the full create payload, not just output+end_time).e2e/run_e2e.sh,e2e/run_e2e_real_opik.sh): capture full Hermes output and assert themay cause data losswarning is absent.assert_journal.pymerges trace rows last-write-wins including nulls (mirroring real Opik) and asserts the finalized trace still carries name + thread_id + output + end_time with zerotrace.update()PATCHes — so a partial re-send that nulls name/thread is caught. Verified to pass the fix and fail the old pattern.Trace.update()+Trace.end()); this branch emits zero. The finalized trace queried back from the backend carried a real name/thread_id/input/output/end_time with no NA trace and clean llm+tool spans (no NA spans). This confirms the warning-absence check genuinely surfaces the warning (it fired on old code) rather than silently missing it.ruff check/ruff format --check/pytestall green (matches CI).Documentation
N/A — no user-facing docs. Behavior change is internal to the plugin's trace lifecycle; the upsert-only rule it adopts is the existing codebase-wide integration convention.