Skip to content

[OPIK-7186] [SDK] feat: surface batch-flush data loss via FlushResult - #7513

Merged
alexkuzmik merged 11 commits into
mainfrom
aliaksandrk/OPIK-7186-batch-flush-error-visibility
Jul 22, 2026
Merged

[OPIK-7186] [SDK] feat: surface batch-flush data loss via FlushResult#7513
alexkuzmik merged 11 commits into
mainfrom
aliaksandrk/OPIK-7186-batch-flush-error-visibility

Conversation

@alexkuzmik

@alexkuzmik alexkuzmik commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Details

Opik.end() / flush() silently swallowed batch POST failures (e.g. an HTTP 4xx on an exhausted trace pool — a 402 free-tier span-quota rejection, or a 403): the batch error was only logged, never surfaced via a return value or exception, so caller code saw success while trace/span data was dropped server-side. This PR surfaces terminal data loss in-band — without ever raising into user code or blocking the app, since an observability SDK must not disrupt its host.

What it adds

  • FlushResult (exported from opik) — returned by flush()/end() and cached on Opik.last_flush_result. Fields: flushed, remaining_queue_size, dropped_messages, dropped_items, failures (list of FailedMessageInfo), and a .success property (flushed and dropped_messages == 0).
  • FailedMessageInfo / FailureReason (exported) — per-drop detail: message type, item count, HTTP status, reason (HTTP_CLIENT_ERROR, HTTP_SERVER_ERROR, UNAUTHORIZED, SERIALIZATION, UNKNOWN), and a short detail string.
  • Opik.get_errors_report() -> ErrorsReport (exported) — sender-wide data-loss report, independent of any single flush: surfaces drops that happened before or between flushes, which last_flush_result (scoped to one flush window) does not. Carries exact all-time total_dropped_messages/total_dropped_items, retained per-drop failures (each with its own timestamp), a generated_at timestamp, and first_failure_at/last_failure_at/has_data_loss helpers. Backed by a capped collection (bounded deque, most-recent N), so memory stays bounded regardless of run length.
  • Internal building blocks (single-responsibility, dependency-injected):
    • DataLossTracker — thread-safe, bounded record of terminal drops shared across Opik handles on one connection; keeps exact running message/item totals independent of the bounded detail window, so counts stay correct even after eviction.
    • FlushReporter — assembles a FlushResult from the streamer + tracker; owns the "did this flush lose data" logic so the client doesn't gather it piecemeal.
    • BaseMessage.item_count — how many traces/spans a (possibly batched) message carries.

What it affects (behavior / API changes)

  • flush() -> bool now means "delivered everything": it returns False (never raises) on a timeout or any terminal drop — previously it only reflected whether the queue drained. Callers that check if client.flush(): ... will now correctly see False when data was lost.
  • end() -> Optional[FlushResult] (was None) — returns the flush outcome, derived from the authoritative flush result computed during teardown (not the weaker queue_size() == 0 proxy). Idempotent: a repeated end() no longer overwrites last_flush_result with a spurious not-flushed result.
  • Streamer.close() now returns whether all data was flushed (authoritative on flush=True); threaded through SharedConnectionResourcesBundle.close()/flush() and the lease/manager release path.
  • Terminal drops are recorded in the background message processor: 4xx→HTTP_CLIENT_ERROR, 5xx / exhausted retries→HTTP_SERVER_ERROR, 401→UNAUTHORIZED, serialization→SERIALIZATION, unexpected→UNKNOWN. Transient states are deliberately not counted as loss: 409 dedup, retryable 429, and connection errors parked for replay.
  • Internal constructor wiring only: OpikMessageProcessor and the processors-chain factory take a data_loss_tracker; the bundle builds the tracker + FlushReporter. No change to transient-error handling.

Scope / deferred

Scope is intentionally the reported failure mode: backend rejection of trace/span batches surfaced through the flush contract. Deferred to follow-ups: queue-overflow accounting, shutdown-replay-abandoned accounting, file-upload (attachment) failure counts, and the header-less/unparseable 429 sub-case (a pre-existing path that is neither retried nor recorded — left as-is to avoid changing rate-limit handling in this PR).

Change checklist

  • User facing
  • Documentation update

Issues

  • OPIK-7186

AI-WATERMARK

AI-WATERMARK: yes

  • Tools: Claude Code
  • Model(s): Claude Opus 4.8
  • Scope: implementation, unit tests, and PR-review responses, under author direction
  • Human verification: author reviewed the design, scope decisions, and diff; full SDK unit suite + ruff/ruff-format/mypy run locally and in CI

Testing

  • pytest tests/unit/message_processing/ tests/unit/api_objects/test_opik_client.py tests/unit/api_objects/test_connection_resource_manager.py — all green.
  • pre-commit (ruff, ruff-format, mypy) clean on changed files.
  • New unit coverage: flush()/end() return values and last_flush_result; never-raises on internal failure (and no stale prior success); authoritative flushed (not queue_size() == 0); idempotent repeated end(); per-branch terminal-drop recording (403/500/401/serialization/retry) vs non-recording of transient states (409/429/connection); DataLossTracker exact message/item counts under eviction; get_errors_report() surfaces drops outside the flush window, carries timestamps, and stays bounded to capacity.
  • Verified end-to-end against a live backend: happy path reports success=True; a real backend rejection (invalid key → 401) yields flush() == False with populated last_flush_result and no exception.
  • Not run locally: the live-API integration suites (ADK/Anthropic/etc.) — unaffected by this change.

Documentation

No external documentation changes required. Public docstrings for end(), flush(), last_flush_result, and the new FlushResult / FailedMessageInfo / FailureReason types are added inline.

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>
@alexkuzmik
alexkuzmik requested a review from a team as a code owner July 17, 2026 15:32
@github-actions github-actions Bot added 🔴 size/XL python Pull requests that update Python code tests Including test files, or tests related like configuration. Python SDK and removed 🔴 size/XL labels Jul 17, 2026
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

⏱️ pre-commit per-hook timing

Hook Description Result Duration
🐍 mypy — python sdk Static type check 1.38s
🐍 fix end of files — python sdk Ensure files end in a newline 0.04s
🐍 trim trailing whitespace — python sdk Strip trailing whitespace 0.04s
🐍 ruff-format — python sdk Format Python code (ruff) 0.02s
🐍 ruff — python sdk Lint + autofix Python (ruff) 0.01s
Total (5 ran) 1.49s
⏭️ 36 skipped (no matching files changed)
Hook Description Result
🤖 trim trailing whitespace — optimizer Strip trailing whitespace ⏭️
🤖 fix end of files — optimizer Ensure files end in a newline ⏭️
🤖 check yaml — optimizer Validate YAML syntax ⏭️
🤖 check json — optimizer Validate JSON syntax ⏭️
🤖 check toml — optimizer Validate TOML syntax ⏭️
🤖 check for added large files — optimizer Block large files (>1MB) ⏭️
🔐 detect private key — optimizer Block committed private keys ⏭️
🤖 check for merge conflicts — optimizer Block merge-conflict markers ⏭️
🤖 check for case conflicts — optimizer Block case-only name clashes ⏭️
🤖 pyupgrade — optimizer Modernize Python syntax ⏭️
🤖 ruff — optimizer Lint + autofix Python (ruff) ⏭️
🤖 ruff-format — optimizer Format Python code (ruff) ⏭️
🤖 mypy — optimizer Static type check ⏭️
📓 nbstripout — optimizer notebooks Strip notebook output ⏭️
📝 markdownlint — optimizer Lint Markdown ⏭️
🔤 codespell — optimizer Fix common misspellings ⏭️
📊 radon cc — optimizer Cyclomatic-complexity gate ⏭️
📊 radon raw — optimizer Raw size metrics gate ⏭️
📊 xenon — optimizer Fail on complexity thresholds ⏭️
📊 lizard — optimizer Cyclomatic-complexity gate ⏭️
🧹 vulture — optimizer Find dead code ⏭️
🛡️ trim trailing whitespace — guardrails Strip trailing whitespace ⏭️
🛡️ fix end of files — guardrails Ensure files end in a newline ⏭️
🛡️ ruff — guardrails Lint + autofix Python (ruff) ⏭️
🛡️ ruff-format — guardrails Format Python code (ruff) ⏭️
🛡️ mypy — guardrails Static type check ⏭️
⚓ helm-docs Regenerate Helm chart README ⏭️
block non-public FE plugins Block non-public FE plugins ⏭️
☕ spotless — java backend Format Java code ⏭️
🧪 pre-commit wrapper smoke tests Self-test the wrapper scripts ⏭️
🌐 eslint — frontend Lint + autofix JS/TS ⏭️
🌐 typecheck — frontend Whole-project tsc type check ⏭️
📘 eslint — typescript sdk Lint + autofix JS/TS ⏭️
📘 typecheck — typescript sdk Whole-project tsc type check ⏭️
⚙️ actionlint — github workflows Lint GitHub Actions workflows ⏭️
🐳 hadolint — dockerfiles Lint Dockerfiles ⏭️

Comment on lines 365 to 367
if flush and close_on_zero:
with self._lock:
entry = self._entries.get(key)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrent flushes race on shared bundle

Two racing release() calls can both pass the entry.refcount > 1 check and then call shared_bundle.flush(timeout) concurrently, so the same shared resource gets drained twice and FlushResult can become inconsistent — should we mark the bundle as flush-in-progress under self._lock?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/api_objects/connection_resources.py around lines 365-375 inside
ConnectionResourceManager.release (the shared pre-flush block where `entry.refcount > 1`
leads to `shared_bundle.flush(timeout)` outside the lock), prevent two racing releases
from both flushing the same shared bundle. Refactor the preflush decision so that, while
holding self._lock, you atomically mark the bundle/entry as “flush in progress”
(e.g., add an `entry.preflush_in_progress` flag and optionally store a flush-result
placeholder) and only the first caller may perform `shared_bundle.flush(timeout)`; any
concurrent callers should not call flush again. If you need consistent FlushReporter
outcomes for all callers, have the non-owner callers either wait for a completion Event
set by the owner (without starting a second flush) or return a clearly defined value
(e.g., flushed=None) while the owner drains and records the authoritative result.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping — this shared-bundle pre-flush block predates this PR (the change here only captures its return value). A concurrent double streamer.flush() is benign: the drain is idempotent (the second flush finds an already-drained queue), and each client builds its own FlushResult from its own marker, so there's no cross-client inconsistency. Adding lock-held flush-in-progress state is the extra concurrency machinery this MVP-scoped PR deliberately avoids.

🤖 Reply posted via /address-github-pr-comments

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense — I’ll remember that this pre-existing shared-bundle flush path is intentionally tolerated here and save it to memory once the PR is merged.

bundle = entry.resources

bundle.close(timeout, flush=flush)
return bundle.close(timeout, flush=flush)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Teardown returns false instead of none

Lease.release() returns False on the flush=False last-reference path because ConnectionResourceManager.release() forwards bundle.close(timeout, flush=flush), so the public API no longer matches its None contract for non-draining teardown — should we normalize that branch to None before returning?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/api_objects/connection_resources.py around lines 341-401, in
ConnectionResourceManager.release(), the last-reference branch returns
bundle.close(timeout, flush=flush), which yields False for flush=False and violates
Lease.release()’s “None when no draining occurred” contract. Refactor so the
last-reference path captures the close result and, when flush=False, returns None
instead of False; keep the existing behavior for flush=True (return the drained/flush
outcome). Ensure the method’s Optional[bool] semantics now align with
Lease.release’s docstring for non-flushing teardown.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8ae4a0ConnectionResourceManager.release() now returns None on the flush=False path instead of forwarding close()'s bool, matching the documented "no drain outcome" contract.

🤖 Reply posted via /address-github-pr-comments

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit d8ae4a0 addressed this comment by changing the last-reference release path to return None when flush=False instead of forwarding bundle.close()'s False result. It preserves the flush outcome only for flush=True, so the non-draining teardown now matches the public None contract.

Comment on lines +38 to +46
def build_result(self, marker: int, *, flushed: bool) -> "data_loss.FlushResult":
dropped_messages, failures = self._data_loss_tracker.drops_since(marker)
result = data_loss.FlushResult(
flushed=flushed,
remaining_queue_size=self._streamer.queue_size(),
dropped_messages=dropped_messages,
dropped_items=sum(failure.item_count for failure in failures),
failures=failures,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_result computes dropped_items by summing failure.item_count over the bounded failures window, so once DataLossTracker.drops_since() evicts older entries dropped_items undercounts while dropped_messages stays exact — should we keep a running item-loss accumulator in DataLossTracker (independent of eviction) and use that here instead?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
In sdks/python/src/opik/message_processing/flush_reporter.py around lines 38-46 in
`FlushReporter.build_result`, stop computing `FlushResult.dropped_items` by summing
`failure.item_count` over the truncated `failures` list returned by
`DataLossTracker.drops_since()`. Instead, update `DataLossTracker` to maintain an exact
cumulative item-loss counter alongside its existing recorded marker/count (so eviction
of the bounded deque cannot affect the total), and expose that total via
`drops_since()`'s return value or a dedicated accessor. Then modify `build_result` to
set `dropped_items` from that exact accumulator, ensuring `dropped_messages` and
`dropped_items` don't diverge after high-error bursts trigger eviction. Update or extend
unit tests to cover the eviction scenario where the old approach previously
undercounted.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8ae4a0DataLossTracker now keeps exact running message and item totals independent of the bounded window, and drops_since() returns an exact dropped_items. It no longer undercounts once details are evicted.

🤖 Reply posted via /address-github-pr-comments

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit d8ae4a0 addressed this comment by adding an exact running item-loss total to DataLossTracker that is independent of the bounded deque, so evictions no longer affect dropped_items. FlushReporter.build_result() now uses the tracker’s returned dropped_items directly instead of recomputing it from the truncated failure window.

Comment on lines 1887 to 1889
"""
End the Opik session and submit all pending messages.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end() doc hides drop mode

end() says it submits all pending messages, which is inaccurate for the public flush=False path that deliberately drops queued data — should we qualify this so only flush=True drains and persists the queue?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/api_objects/opik_client.py around lines 1884-1896, in the
`Opik.end()` docstring, the first summary sentence currently says it “submit[s] all
pending messages”, which contradicts the `flush: bool = True` behavior and the
described fire-and-forget teardown when `flush=False`. Update that opening sentence to
explicitly state that pending queued messages (including file uploads) are
drained/submitted only when `flush=True`, and when `flush=False` the method does not
wait and may drop anything still in flight. Keep the rest of the docstring consistent
with the existing Args/Returns sections describing `flush=False` as dropping pending
data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit d8ae4a0 addressed this comment by updating Opik.end()’s docstring to say that pending messages are submitted only when flush=True, and that flush=False drops anything still queued. The surrounding return/behavior docs were also aligned with that distinction.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8ae4a0end()'s docstring now states pending messages are submitted only when flush=True, and dropped when flush=False.

🤖 Reply posted via /address-github-pr-comments

Comment on lines +1931 to +1938
flushed = self._lease.release(timeout, flush=flush, close_on_zero=True)
self._finalizer.detach()
if not flush:
return None
self._last_flush_result = self._flush_reporter.build_result(
marker, flushed=bool(flushed)
)
return self._last_flush_result

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idempotent end overwrites success

end() coerces Lease.release() returning None into bool(flushed) and rebuilds _last_flush_result, so a second end() call overwrites the real flush outcome with FlushResult(flushed=False, ...) even though no new shutdown work ran — should we short-circuit when flushed is None and keep the existing last_flush_result?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/api_objects/opik_client.py around lines 1884-1938 in the `end()`
method, fix idempotency: `self._lease.release(..., flush=flush, ...)` can return `None`
when already released, but the current code still calls
`_flush_reporter.build_result(marker, flushed=bool(flushed))`, overwriting
`_last_flush_result` with a “flushed=False” result on subsequent `end()` calls.
Refactor so that after `flushed = self._lease.release(...)`, if `flush` is True and
`flushed is None`, you short-circuit and return the existing `self._last_flush_result`
(or `None` if it was never set) without rebuilding. Keep the existing behavior for the
first successful release (when `flushed` is not `None`) and still return `None` when
`flush` is False.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit d8ae4a0 addressed this comment by short-circuiting end() when self._lease.release(...) returns None. On repeated end() calls, it now returns the existing self._last_flush_result instead of rebuilding a new FlushResult(flushed=False, ...), preserving the original flush outcome.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8ae4a0end() now short-circuits when release() returns None (already released), keeping the prior last_flush_result instead of overwriting it with a spurious flushed=False result. Added idempotency test coverage.

🤖 Reply posted via /address-github-pr-comments

Comment on lines 1951 to +1953
Returns:
True if all messages have been sent within specified timeout, False otherwise.
True if all messages were delivered within the timeout with no data
loss; False if the timeout was hit or any message was dropped.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlushResult hides upload failures

flush() now promises True if all messages were delivered ... with no data loss, but it only reflects Streamer.flush() while FileUploadManager.flush() still returns True after failed uploads, so Opik.flush() and last_flush_result.success can report success even when attachment uploads were lost — should we scope the public contract to trace/span delivery only, or include upload failures in the flush outcome?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/api_objects/opik_client.py around lines 1940-1975, in the `flush()`
method (and the `last_flush_result` property that it feeds), fix the breaking contract
mismatch: today `flush()` computes `last_flush_result` solely from
`self._streamer.flush(timeout)`, but file/attachment uploads can fail independently
(FileUploadManager.flush() can return success even with per-upload failures). Update
`flush()` so the public `last_flush_result.success` and docstring reflect the true
outcome by either (preferred) aggregating streamer message delivery success plus
file-upload failure/partial-loss into the boolean passed to
`_flush_reporter.build_result`, or (alternative) narrowing the doc/result wording to
“trace/span delivery” only and making `last_flush_result` name/semantics match that
narrower scope. Ensure the same semantics are consistent with `end()` (lines ~1884-1939)
and that the reported success includes (or explicitly excludes) attachment upload
failures.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit d8ae4a0 addressed this comment by narrowing flush()/last_flush_result to trace/span/feedback delivery and explicitly stating that attachment/file uploads are not reflected in the outcome. It also made end() return the same flush outcome path, so the public semantics are now consistent with that narrower contract.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8ae4a0flush()'s docstring now scopes the contract to trace/span/feedback delivery and notes attachment/file uploads aren't reflected in the outcome. (Upload-failure accounting is deferred to a follow-up per this PR's MVP scope.)

🤖 Reply posted via /address-github-pr-comments

Comment thread sdks/python/src/opik/api_objects/opik_client.py
Comment thread sdks/python/src/opik/message_processing/processors/online_message_processor.py Outdated
Comment thread sdks/python/tests/unit/api_objects/test_opik_client.py
…t, 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>
Comment thread sdks/python/src/opik/message_processing/data_loss.py Outdated
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
self._data_loss_tracker.record(
data_loss.FailedMessageInfo(
message_type=type(message).__name__,
reason=reason,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question | Logic

Every terminal branch now records a drop except one: a 429 whose exception.headers is None or whose rate_limit.parse_rate_limit(...) returns None (a few lines up in the elif exception.status_code == 429: block). In that case the handler neither re-raises OpikCloudRequestsRateLimited (so it isn't retried/re-enqueued) nor calls _record_data_loss, so the message is silently abandoned and stays invisible to FlushResult.

Given the PR's goal of surfacing terminal drops in-band, is leaving this 429 sub-case unrecorded intentional? It's pre-existing behavior, but this change makes it the one terminal path that escapes accounting. If it's out of scope, a one-line note in the ## Details "deferred" list would make the boundary explicit.

🤖 Review posted via /review-github-pr

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — it's intentional (the 429 branch is byte-identical to main), but you're right it should be explicit. Added a note to the PR description's deferred list: the header-less / unparseable 429 sub-case is neither retried nor recorded, left as-is to avoid changing rate-limit handling in this PR.

"""
Flush the streamer to ensure all messages are sent.

Covers delivery of trace/span/feedback messages; attachment/file uploads

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 nit | Docs

attachment/file uploads are not reflected in the outcome

This is only true for the data-loss detail (failures / dropped_*). The flushed field — and therefore success and this method's return value — does reflect uploads: streamer.flush() returns upload_flushed and self._all_done(), so a stuck/failed upload makes flushed=False and this returns False.

Consider tightening to something like: "upload failures are not counted in the dropped-message detail, though an incomplete upload still makes the flush report as not fully flushed."

🤖 Review posted via /review-github-pr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 86e3b46 addressed this comment by documenting that flush covers attachment/file uploads while distinguishing the flush outcome from its data-loss detail.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — you're right, flushed does reflect uploads (streamer.flush() returns upload_flushed and _all_done()). Reworded the docstring: attachment/file upload failures aren't counted in the data-loss detail (dropped_*/failures), but an incomplete upload still makes the flush report as not fully flushed, so the returned bool does reflect uploads.

Comment on lines +50 to +51
detail: Optional[str] = None
timestamp: float = dataclasses.field(default_factory=time.time)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 nit | Style

FlushResult is frozen=True but carries a mutable failures: List[FailedMessageInfo], so the value isn't truly immutable — a caller can still result.failures.append(...). It's never mutated internally, so this is cosmetic, but a Tuple[FailedMessageInfo, ...] would make the frozen contract honest (and drops_since already builds a fresh list you can just wrap).

🤖 Review posted via /review-github-pr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit ef23478 addressed this comment by changing both failures fields to tuples and converting the constructed reports with tuple(failures), preventing callers from appending to the returned collection.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — FlushResult.failures (and ErrorsReport.failures) are now Tuple[FailedMessageInfo, ...], built via tuple(...), so the frozen contract is honest.

@JetoPistola

Copy link
Copy Markdown
Contributor

👋 Review summary

What looks good

  • Clean read/write split: DataLossTracker (write side) + FlushReporter (read side), each with one clear job and shared correctly across ref-counted handles.
  • The marker/drops_since design keeps message+item counts exact under eviction while letting only the detail list be best-effort — a genuinely nice touch, and directly tested.
  • end() deriving flushed from the authoritative drain result rather than the queue_size()==0 proxy, plus staying idempotent across repeated calls, closes real races.
  • "Never raise, never block" is honored end-to-end, and the fallback FlushResult on internal failure avoids leaving a stale prior success.
  • Excellent test coverage: per-error-branch recording vs non-recording of transient states, eviction-exactness, idempotency, and the authoritative-flushed path.

Overall
Well-scoped to the reported failure mode, with the deferred items called out explicitly. Solid, mergeable design.

Inline comments: 1 question + 2 nits — nothing blocking.

🤖 Review posted via /review-github-pr

@@ -171,6 +188,14 @@ def process(self, message: messages.BaseMessage) -> None:
extra={"error_tracking_extra": error_tracking_extra},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question | Semantics (platform-side, non-blocking — likely a follow-up)

The ## Details motivating example is "HTTP 403 on an exhausted trace pool." Tracing the managed-cloud path, the actual rejection for an exhausted allocation is the free-tier OPIK_SPAN_COUNT quota, enforced on the @UsageLimited trace/span write endpoints, which throws 402 Payment Required with "You have exceeded the usage limit for this operation." — not 403 (403 is used for genuine authorization failures elsewhere).

This doesn't affect the code here: both 402 and 403 map to HTTP_CLIENT_ERROR → terminal drop, so the accounting is correct regardless. Two small things worth considering, both fine as a follow-up:

  1. PR-description wording — the example is really a 402 free-tier span-quota rejection, not a 403; tightening the wording keeps the user-facing story accurate ("you've hit your span allowance, upgrade/reduce volume" vs. "forbidden").
  2. Should quota-exceeded be a distinct FailureReason? A generic HTTP_CLIENT_ERROR is technically right but loses the actionable meaning. A dedicated reason (e.g. USAGE_LIMIT_EXCEEDED derived from 402) would let callers surface "upgrade your plan" rather than a generic client error. Out of this PR's scope, but a natural extension of the new FailureReason taxonomy.

🤖 Review posted via /review-github-pr

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the managed-cloud path. Tightened the PR description to "an HTTP 4xx ... a 402 free-tier span-quota rejection, or a 403" so the user-facing story is accurate. A dedicated USAGE_LIMIT_EXCEEDED reason derived from 402 is a nice extension of the new FailureReason taxonomy, but out of scope here — both 402 and 403 map to HTTP_CLIENT_ERROR today so accounting is already correct. Noting it as a follow-up.

JetoPistola
JetoPistola previously approved these changes Jul 20, 2026

@JetoPistola JetoPistola left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — solid, well-scoped implementation with strong test coverage. The one open item (402-vs-403 status semantics / a dedicated USAGE_LIMIT_EXCEEDED FailureReason) is platform-side and non-blocking; good candidate for a follow-up. Nice work.

🤖 Review posted via /review-github-pr

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>
alexkuzmik and others added 3 commits July 21, 2026 13:53
…eport()

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>
Callers can check total_dropped_messages directly; the convenience flag is
redundant.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
petrotiurin
petrotiurin previously approved these changes Jul 21, 2026

@petrotiurin petrotiurin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than this looks good, no issues from my side.

Comment thread sdks/python/src/opik/message_processing/data_loss.py Outdated
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>
Comment thread sdks/python/src/opik/message_processing/data_loss.py Outdated
Comment thread sdks/python/src/opik/message_processing/data_loss.py
Comment thread sdks/python/src/opik/message_processing/data_loss.py
Comment thread sdks/python/src/opik/message_processing/data_loss.py
Comment thread sdks/python/src/opik/message_processing/data_loss.py
Comment thread sdks/python/tests/unit/message_processing/test_data_loss.py
alexkuzmik and others added 3 commits July 22, 2026 10:45
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>
…h-error-visibility' into aliaksandrk/OPIK-7186-batch-flush-error-visibility
@alexkuzmik
alexkuzmik merged commit cb2a76a into main Jul 22, 2026
135 of 136 checks passed
@alexkuzmik
alexkuzmik deleted the aliaksandrk/OPIK-7186-batch-flush-error-visibility branch July 22, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Python SDK python Pull requests that update Python code 🔴 size/XL tests Including test files, or tests related like configuration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants