Skip to content

Commit cb2a76a

Browse files
alexkuzmikclaude
andauthored
[OPIK-7186] [SDK] feat: surface batch-flush data loss via FlushResult (#7513)
* [OPIK-7186] [SDK] feat: surface batch-flush data loss via FlushResult The background sender silently dropped trace/span batches on terminal failures (e.g. 403), while flush()/end() reported success. Add in-band, non-blocking, non-throwing visibility: - FlushResult returned by flush()/end() (flushed, drops, pending_replay, per-drop details); flush() bool now reflects data loss. - Opik.last_flush_result + Opik.get_upload_errors() (sender-wide history). - DataLossTracker records terminal drops; FlushReporter assembles results. - Drops recorded across processor (HTTP/serialization/unknown, 401), queue overflow (generic on_evict hook), and shutdown-replay abandonment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sdk): address PR review — idempotent end(), flush() failure result, exact dropped_items - end(): short-circuit when release() returns None so a repeated end() no longer overwrites last_flush_result with a spurious not-flushed result - flush(): on failure, record a failed FlushResult instead of leaving a stale prior success in last_flush_result - manager.release(): return None (not close()'s bool) on the flush=False path, matching the "no drain outcome" contract - DataLossTracker: keep exact running message/item totals so dropped_items no longer undercounts once details are evicted from the bounded window - revert recording unparseable-429s as terminal data loss (a 429 is transient, not lost) — restores prior behavior - docs: qualify end()/flush() for flush=False and attachment-upload scope - test: build the flush-result client via __new__ to avoid real background resources; add idempotent-end and stale-result coverage Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(sdk): fix subject-verb agreement in drops_since docstring Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sdk): add Opik.get_data_loss() sender-wide data-loss history last_flush_result is scoped to a single flush window, so drops that happened before or between flushes were recorded but not retrievable in-band. Expose the DataLossTracker's retained history via Opik.get_data_loss() (tracker -> FlushReporter -> client). Storage stays a capped deque, so memory is bounded regardless of run length. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sdk): return ErrorsReport with timestamps from Opik.get_errors_report() Replace the bare-list get_data_loss() with get_errors_report() returning an ErrorsReport instance: exact all-time dropped message/item totals, retained per-drop details, a generated_at timestamp, and first/last_failure_at helpers (each FailedMessageInfo already carries its own timestamp). Storage stays a capped deque, so memory is bounded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(sdk): drop ErrorsReport.has_data_loss Callers can check total_dropped_messages directly; the convenience flag is redundant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(sdk): document that ErrorsReport failures are capped Make the bounded (drop-oldest, default 1000) nature of the report's per-drop details explicit on ErrorsReport and Opik.get_errors_report(): totals stay exact, but failures keeps only the most recent entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(sdk): drop the lock in DataLossTracker Per review: the deque is thread-safe and an exact-to-the-message data-loss tally isn't worth lock contention on the hot sending path. Counters are plain ints now (best-effort under concurrent drops); reads snapshot the deque once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(sdk): make FlushResult/ErrorsReport failures an immutable tuple Address PR review: frozen dataclasses now expose failures as Tuple[FailedMessageInfo, ...] so the value is truly immutable; clarify that flush()'s bool does reflect upload completion (only the data-loss detail excludes uploads); strengthen the eviction test to assert retained identities and order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7df8e83 commit cb2a76a

16 files changed

Lines changed: 1006 additions & 33 deletions

sdks/python/src/opik/__init__.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@
2424
from .api_objects.trace import Trace
2525
from .configurator.configure import configure
2626
from .decorator.tracker import flush_tracker, track
27+
from .message_processing.data_loss import (
28+
ErrorsReport,
29+
FailedMessageInfo,
30+
FailureReason,
31+
FlushResult,
32+
)
2733
from .evaluation import (
2834
evaluate,
2935
evaluate_experiment,
@@ -68,6 +74,10 @@
6874
"ExperimentItemReferences",
6975
"track",
7076
"flush_tracker",
77+
"FlushResult",
78+
"FailedMessageInfo",
79+
"FailureReason",
80+
"ErrorsReport",
7181
"Opik",
7282
"get_global_client",
7383
"set_global_client",

sdks/python/src/opik/api_objects/connection_resources.py

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
from ..file_upload import upload_manager
3131
from ..healthcheck import connection_monitor, connection_probe
3232
from ..message_processing import (
33+
data_loss,
34+
flush_reporter,
3335
message_queue,
3436
permissions,
3537
streamer,
@@ -61,6 +63,7 @@ def __init__(
6163
file_upload_manager: upload_manager.FileUploadManager,
6264
replay_manager: replay_manager.ReplayManager,
6365
streamer: streamer.Streamer,
66+
data_loss_tracker: data_loss.DataLossTracker,
6467
flush_timeout: Optional[int],
6568
) -> None:
6669
self.httpx_client = httpx_client
@@ -69,14 +72,18 @@ def __init__(
6972
self.file_upload_manager = file_upload_manager
7073
self.replay_manager = replay_manager
7174
self.streamer = streamer
75+
self.flush_reporter = flush_reporter.FlushReporter(
76+
streamer=streamer,
77+
data_loss_tracker=data_loss_tracker,
78+
)
7279
self.flush_timeout = flush_timeout
7380

74-
def close(self, timeout: Optional[int], *, flush: bool) -> None:
81+
def close(self, timeout: Optional[int], *, flush: bool) -> bool:
7582
# Drain/stop the streamer (consumer threads, replay, batch preprocessor);
7683
# on flush=True it also flushes pending file uploads.
7784
# Closing the streamer also stops and joins the replay manager (its own
7885
# daemon thread), so there is no separate replay teardown to do here.
79-
self.streamer.close(timeout, flush=flush)
86+
flushed = self.streamer.close(timeout, flush=flush)
8087
# Stop the upload worker pool too, so eviction doesn't leave its threads
8188
# running. wait=flush mirrors the streamer: block for in-flight uploads
8289
# on a durable close, return immediately on fire-and-forget teardown.
@@ -90,19 +97,23 @@ def close(self, timeout: Optional[int], *, flush: bool) -> None:
9097
# daemon threads to finish in-flight requests, so closing the pool
9198
# here would race them — leave it for GC / process-exit close_all.
9299
self.httpx_client.close()
100+
return flushed
93101

94-
def flush(self, timeout: Optional[int]) -> None:
102+
def flush(self, timeout: Optional[int]) -> bool:
95103
"""Drain the shared message queue without tearing the bundle down.
96104
97105
Used when a handle releases with ``flush=True`` while other handles still
98106
share the bundle: the queued data is persisted now, but the transport
99107
stays alive for the remaining handles.
108+
109+
Returns whether the queue drained fully within ``timeout``.
100110
"""
101-
self.streamer.flush(timeout)
111+
return self.streamer.flush(timeout)
102112

103113

104114
def _create_replay_manager(
105-
config: opik_config.OpikConfig, httpx_client: httpx.Client
115+
config: opik_config.OpikConfig,
116+
httpx_client: httpx.Client,
106117
) -> replay_manager.ReplayManager:
107118
probe = connection_probe.ConnectionProbe(
108119
base_url=config.url_override,
@@ -156,6 +167,8 @@ def create_connection_resources(
156167
worker_count=config.file_upload_background_workers,
157168
)
158169

170+
data_loss_tracker = data_loss.DataLossTracker()
171+
159172
fallback_replay = _create_replay_manager(config, httpx_client_)
160173

161174
message_processor = message_processors_chain.create_message_processors_chain(
@@ -166,6 +179,7 @@ def create_connection_resources(
166179
retry_interval_seconds=config.unauthorized_message_type_retry_interval,
167180
max_retry_count=config.unauthorized_message_type_max_retry_count,
168181
),
182+
data_loss_tracker=data_loss_tracker,
169183
)
170184
streamer_ = streamer_constructors.construct_online_streamer(
171185
file_uploader=file_uploader,
@@ -186,6 +200,7 @@ def create_connection_resources(
186200
file_upload_manager=file_uploader,
187201
replay_manager=fallback_replay,
188202
streamer=streamer_,
203+
data_loss_tracker=data_loss_tracker,
189204
flush_timeout=config.default_flush_timeout,
190205
)
191206

@@ -236,12 +251,16 @@ def __init__(
236251

237252
def release(
238253
self, timeout: Optional[int], *, flush: bool = True, close_on_zero: bool
239-
) -> None:
254+
) -> Optional[bool]:
255+
"""Release this handle's reference. Returns the authoritative flush
256+
outcome when this release performed the drain (an explicit
257+
``flush=True`` release), ``None`` otherwise (already released, or a GC
258+
finalizer that does no network I/O)."""
240259
with self._once_lock:
241260
if self._released:
242-
return
261+
return None
243262
self._released = True
244-
self._manager.release(
263+
return self._manager.release(
245264
self._key, timeout, flush=flush, close_on_zero=close_on_zero
246265
)
247266

@@ -326,7 +345,7 @@ def release(
326345
*,
327346
flush: bool = True,
328347
close_on_zero: bool,
329-
) -> None:
348+
) -> Optional[bool]:
330349
# Durability under sharing: an explicit ``end(flush=True)`` on a handle
331350
# that still shares its bundle must drain the shared queue *before* this
332351
# handle gives up its reference. Flushing while our reference is still
@@ -337,6 +356,12 @@ def release(
337356
# bundle; a sole holder's ``close(flush=True)`` below already drains
338357
# durably. A GC finalizer (``close_on_zero=False``) never does network
339358
# I/O, so it never pre-flushes.
359+
# Authoritative flush outcome for the caller: set by whichever branch
360+
# below actually drains — the shared pre-flush or the last-reference
361+
# close. Stays None when this release did no draining (GC finalizer, or
362+
# a bundle already released elsewhere), so the caller can tell "not
363+
# confirmed" apart from "confirmed not flushed".
364+
flushed: Optional[bool] = None
340365
if flush and close_on_zero:
341366
with self._lock:
342367
entry = self._entries.get(key)
@@ -346,7 +371,7 @@ def release(
346371
else None
347372
)
348373
if shared_bundle is not None:
349-
shared_bundle.flush(timeout)
374+
flushed = shared_bundle.flush(timeout)
350375

351376
# Now drop our reference. Because we only decrement here — after any
352377
# pre-flush above has completed — a close can never run while another
@@ -355,10 +380,10 @@ def release(
355380
with self._lock:
356381
entry = self._entries.get(key)
357382
if entry is None:
358-
return
383+
return flushed
359384
entry.refcount -= 1
360385
if entry.refcount > 0:
361-
return
386+
return flushed
362387
if not close_on_zero:
363388
# The last reference was dropped by a GC finalizer (see
364389
# ``Opik._acquire_shared_resources``). Only the refcount
@@ -367,13 +392,16 @@ def release(
367392
# never happen inside garbage collection. Leave the bundle
368393
# cached so a later same-identity ``acquire`` reuses it, or
369394
# ``close_all`` disposes it at process exit.
370-
return
395+
return flushed
371396
# Evict before close, under the lock, so a concurrent acquire never
372397
# receives a bundle that is being torn down.
373398
del self._entries[key]
374399
bundle = entry.resources
375400

376-
bundle.close(timeout, flush=flush)
401+
closed_flushed = bundle.close(timeout, flush=flush)
402+
# A non-draining teardown has no flush outcome to report — return None
403+
# (not close()'s bool) so the result stays "no drain happened here".
404+
return closed_flushed if flush else None
377405

378406
def close_all(self, *, flush: bool = True) -> None:
379407
"""Close and evict every cached bundle. Registered as the process

sdks/python/src/opik/api_objects/opik_client.py

Lines changed: 88 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
url_helpers,
7272
)
7373
from ..message_processing import (
74+
data_loss,
7475
messages,
7576
)
7677
from ..message_processing.batching import sequence_splitter
@@ -217,6 +218,8 @@ def _bind_resources(self) -> None:
217218
self._rest_client = self._resources.rest_client
218219
self.__internal_api__message_processor__ = self._resources.message_processor
219220
self._streamer = self._resources.streamer
221+
self._flush_reporter = self._resources.flush_reporter
222+
self._last_flush_result: Optional[data_loss.FlushResult] = None
220223

221224
def _display_trace_url(self, trace_id: str, project_name: str) -> None:
222225
project_url = url_helpers.get_project_url_by_trace_id(
@@ -1927,9 +1930,13 @@ def get_experiment_by_id(self, id: str) -> experiment.Experiment:
19271930
project_name=experiment_public.project_name,
19281931
)
19291932

1930-
def end(self, timeout: Optional[int] = None, *, flush: bool = True) -> None:
1933+
def end(
1934+
self, timeout: Optional[int] = None, *, flush: bool = True
1935+
) -> Optional[data_loss.FlushResult]:
19311936
"""
1932-
End the Opik session and submit all pending messages.
1937+
End the Opik session, releasing this client's connection reference. When
1938+
``flush`` is True (the default), all pending messages are submitted
1939+
first; when ``flush`` is False, anything still queued is dropped.
19331940
19341941
Connection resources are shared and ref-counted across clients with a
19351942
matching configuration: this releases the current client's reference.
@@ -1957,28 +1964,102 @@ def end(self, timeout: Optional[int] = None, *, flush: bool = True) -> None:
19571964
is shared — it may still succeed by riding another live client's
19581965
resources. Do not rely on either outcome; create a new client instead.
19591966
1967+
The outcome is also available afterwards via :attr:`last_flush_result`.
1968+
19601969
Returns:
1961-
None
1970+
The flush outcome (including any data-loss detail) when ``flush`` is
1971+
True; ``None`` when ``flush`` is False (nothing was flushed).
19621972
"""
19631973
timeout = timeout if timeout is not None else self._flush_timeout
1974+
marker = self._flush_reporter.marker()
19641975
# Explicit teardown on a user thread, so close on the last reference
19651976
# (close_on_zero=True). Releasing is idempotent, so the detached GC
1966-
# finalizer cannot double-decrement.
1967-
self._lease.release(timeout, flush=flush, close_on_zero=True)
1977+
# finalizer cannot double-decrement. release() returns the authoritative
1978+
# flush outcome computed inside the drain (streamer.flush) — the same
1979+
# source flush() uses — rather than the weaker queue_size()==0 proxy,
1980+
# which can read empty on the pop-vs-processed race and while file
1981+
# uploads are still in flight.
1982+
flushed = self._lease.release(timeout, flush=flush, close_on_zero=True)
19681983
self._finalizer.detach()
1984+
if not flush:
1985+
return None
1986+
if flushed is None:
1987+
# No drain ran on this call — e.g. a repeated end() after the client
1988+
# was already released. Keep the outcome from the release that did
1989+
# the work rather than overwriting it with a spurious not-flushed
1990+
# result, so end() is idempotent.
1991+
return self._last_flush_result
1992+
self._last_flush_result = self._flush_reporter.build_result(
1993+
marker, flushed=flushed
1994+
)
1995+
return self._last_flush_result
19691996

19701997
def flush(self, timeout: Optional[int] = None) -> bool:
19711998
"""
19721999
Flush the streamer to ensure all messages are sent.
19732000
2001+
Attachment/file upload *failures* are not counted in the data-loss
2002+
detail (``dropped_*`` / ``failures``), but an incomplete upload still
2003+
makes the flush report as not fully flushed — so ``flushed`` (and hence
2004+
the returned bool) does reflect uploads. Never raises and never blocks
2005+
beyond ``timeout``: an observability SDK must not disrupt the app it
2006+
instruments. Detailed outcome — including any data that was dropped — is
2007+
available via :attr:`last_flush_result`.
2008+
19742009
Args:
19752010
timeout (Optional[int]): The timeout for flushing the streamer. Once the timeout is reached, the flush method will return regardless of whether all messages have been sent.
19762011
19772012
Returns:
1978-
True if all messages have been sent within specified timeout, False otherwise.
2013+
True if all messages were delivered within the timeout with no data
2014+
loss; False if the timeout was hit or any message was dropped.
19792015
"""
19802016
timeout = timeout if timeout is not None else self._flush_timeout
1981-
return self._streamer.flush(timeout)
2017+
try:
2018+
marker = self._flush_reporter.marker()
2019+
flushed = self._streamer.flush(timeout)
2020+
self._last_flush_result = self._flush_reporter.build_result(
2021+
marker, flushed=flushed
2022+
)
2023+
return self._last_flush_result.success
2024+
except Exception:
2025+
# An observability SDK must not disrupt the app it instruments: a
2026+
# failure inside flush is reported as "not flushed", never raised.
2027+
# Record a failed outcome so last_flush_result reflects this attempt
2028+
# rather than keeping a stale prior success. Built directly (not via
2029+
# build_result, which may itself be what raised) so it cannot re-raise.
2030+
LOGGER.error("Opik flush failed unexpectedly", exc_info=True)
2031+
self._last_flush_result = data_loss.FlushResult(
2032+
flushed=False,
2033+
remaining_queue_size=0,
2034+
dropped_messages=0,
2035+
dropped_items=0,
2036+
failures=(),
2037+
)
2038+
return False
2039+
2040+
@property
2041+
def last_flush_result(self) -> Optional[data_loss.FlushResult]:
2042+
"""Outcome of the most recent ``flush()``/``end()`` on this client.
2043+
2044+
``None`` until the first flush.
2045+
"""
2046+
return self._last_flush_result
2047+
2048+
def get_errors_report(self) -> data_loss.ErrorsReport:
2049+
"""Report of messages the background sender terminally dropped.
2050+
2051+
Unlike :attr:`last_flush_result`, which is scoped to a single flush, this
2052+
reports the sender's retained data-loss history — including drops that
2053+
happened before or between flushes.
2054+
2055+
The report is **capped**: the total counts are exact, but the per-drop
2056+
``failures`` list keeps only the most recent entries (bounded, drop-oldest)
2057+
so it never grows without bound — see :class:`~opik.ErrorsReport`.
2058+
2059+
The sender is shared across clients with a matching configuration, so
2060+
the report may include drops from sibling clients on the same connection.
2061+
"""
2062+
return self._flush_reporter.build_errors_report()
19822063

19832064
def __internal_api__drain_to_processors__(
19842065
self, timeout: Optional[float] = None

0 commit comments

Comments
 (0)