[SDK] Consolidate deployment log reading into fetch_logs - #152
Merged
Merged
Conversation
Stream a revision's logs oldest-first with bounded held state: pages are fetched as the iterator is consumed, per-pod anchors are trimmed to the server's re-delivery window, and pod=None merges every pod via a (timestamp, id) watermark. follow=True keeps tailing, re-listing the revision's pods so replacement pods join the merge; a pod silent past LOG_MERGE_HOLD_POLLS poll intervals stops gating the watermark. Signed-off-by: Honglin Cao <hocao@nvidia.com>
Signed-off-by: Honglin Cao <hocao@nvidia.com>
Signed-off-by: Honglin Cao <hocao@nvidia.com>
Review fixes for iter_deployment_logs: - Per-pod buffers with backpressure (LOG_MERGE_BUFFER_PAGES): a pod far ahead of the merge watermark parks at two pages instead of buffering its whole history against a terminated peer. - Watermark: strict cross-pod order while any pod is catching up; once all are at the tip, hold lines one poll_interval so concurrent pods interleave; single-pod streams release immediately. The dedup window and the merge delay are now separate concerns (LOG_MERGE_HOLD_POLLS is gone), and follow=False drains buffers fully before returning. - Caught-up pods are re-polled at most once per poll_interval (next_poll_at), and a caught-up pod gone from the pod list stops being polled, keeping its dedup window in case it is listed again. - Ordering docs state the contract plainly: late-arriving lines are appended when they arrive (the CloudWatch path dropped them). Signed-off-by: Honglin Cao <hocao@nvidia.com>
A millisecond holding more than the log store's per-query ceiling cannot be delivered whole; the page carries the 5000 nearest its paging direction. Signed-off-by: Honglin Cao <hocao@nvidia.com>
The ordering bound is the release point (one poll_interval merging pods, immediate for a single pod), not the server's re-delivery window, which bounds delivery instead. Signed-off-by: Honglin Cao <hocao@nvidia.com>
anandj91
reviewed
Sep 15, 2026
Replace the iter_deployment_logs generator with fetch_logs, one reader for every deployment-log shape: an inclusive [start_time, end_time] window (either bound optional and pure), a newest_first flag selecting only the order chunks arrive, lazy chunks of 1..chunk_size events, and pod=None merging every pod of the revision into one stream. fetch_logs validates eagerly and returns a private generator, so a bad call raises at the call site rather than at the first next(). Forward reads keep the dedup-anchored page walk (the server re-delivers a ~15s look-behind span); backward reads page with bare exclusive int boundaries — pages never split a millisecond — and need no dedup state. The cross-pod merge is direction-aware: forward releases lines at or below the minimum newest-buffered frontier, backward mirrors it with the maximum oldest-buffered frontier, and LOG_MERGE_BUFFER_PAGES backpressure bounds memory in both directions. Lines inside every chunk stay in ascending (timestamp, id) order regardless of direction. get_deployment_logs, get_deployment_logs_range and deployment_log_session delegate to the shared _fetch_log_page primitive and warn as deprecated; no SDK-internal path trips its own warning. Signed-off-by: Honglin Cao <hocao@nvidia.com>
Rewrite the deployment-logs README section and the SDK example around the consolidated fetch_logs: unbounded-by-default windows, newest_first as chunk arrival order only (lines inside every chunk stay ascending), the backward-read completeness caveat, and tailing as repeated forward fetch_logs calls whose consecutive windows overlap by the server's ~15s late-arrival span, deduplicated by event.id with the id set trimmed to the overlap window so the loop's memory never grows with the stream. Extend the migration tables: 0.5.x start_from_head maps to newest_first, and the deprecated 0.6.0 readers each map to a fetch_logs form. Signed-off-by: Honglin Cao <hocao@nvidia.com>
anandj91
reviewed
Sep 16, 2026
Signed-off-by: Honglin Cao <hocao@nvidia.com>
Signed-off-by: Honglin Cao <hocao@nvidia.com>
anandj91
requested changes
Sep 16, 2026
anandj91
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the thorough work, but this is much more than what I asked for. What I want is one simple function: fetch a pod's log lines within a time window and yield them in caller-sized chunks, so the same call serves a one-off read of the past and tailing a running deployment. Roughly two thirds of the new code (direction flag, multi-pod merge with backpressure, the migration guides) implements things nobody requested, and three of my earlier comments were not followed (start_time default, per-pod scope, deprecation decorator). Please cut it down per the inline comments. The forward single-pod walker you already have is essentially the whole feature.
Per review on #152: pod is required, start_time defaults to the call time, and without end_time the generator never terminates — once caught up it yields an empty chunk instead of returning, keeping the dedup anchor alive so tailing needs no caller-side bookkeeping. newest_first, the backward walker, the multi-pod merge and _iter_log_chunks are removed; validation still raises at the call via a nested generator. Tests are cut to the reduced scope, the pytest.ini deprecation filter is replaced with pytest.warns at the call sites, and an open-ended empty-chunk test is added. Signed-off-by: Honglin Cao <hocao@nvidia.com>
The tail recipe with its seen dict, overlap paragraphs and the 0.5.x and 0.6.0 migration tables are gone; the example shows a window read and a tail loop over a single generator. Signed-off-by: Honglin Cao <hocao@nvidia.com>
anandj91
approved these changes
Sep 16, 2026
Request the caller's chunk_size as max_lines instead of overriding it with MAX_LOG_PAGE_LINES, and yield each server page as one chunk rather than buffering pages to re-slice them into exact chunk_size pieces. A chunk can now be smaller (the first page's look-behind span below start_time and re-delivered lines are filtered out) or larger (the server never splits a millisecond, so a burst millisecond arrives whole). chunk_size goes on the wire, so it inherits the server's ceiling and is validated eagerly at the call. The within-chunk insort stays: the server sorts a page by nanosecond timestamp only, never by the id's hash suffix, so lines sharing one nanosecond can arrive in either id order (verified against local Loki). Signed-off-by: Honglin Cao <hocao@nvidia.com>
A page request that fails takes the generator with it, but every chunk already yielded is complete and the next one has not been started. Say so, and give the resume recipe: start_time is inclusive, so re-anchoring on the last delivered event's timestamp re-delivers only that millisecond. Signed-off-by: Honglin Cao <hocao@nvidia.com>
State three things the docs got wrong: a bounded read also ends when the store runs out of lines, so a future end_time does not keep it polling; the dedup window is LOG_DEDUP_RETENTION_MS, not the server's span; and a line landing further behind the boundary than that span is never returned at all, because this reader only pages forward. Hold ids and timestamps in the anchor instead of whole events, so a long tail stops retaining message text it has already handed out. Break out of the page once one line passes end_time, and skip the retention trim on the empty pages of an idle tail. Restore the example's empty-pod-list guard, dropped in the rewrite: a fresh deployment has no pods and the example raised IndexError. Revert the DeploymentLogEvent docstring and the get_deployment_logs page-ceiling paragraph to main; neither method changed behaviour here. Drop the draft narration from the livelock test comment and name the span the way the SDK does everywhere else. Cover the failed-page contract, start_time=0 clamping, and both ends of the chunk_size range. Signed-off-by: Honglin Cao <hocao@nvidia.com>
A page whose lines are all below start_time or all already delivered produces no chunk at all, not the empty chunk that means the stream is caught up. The docstring and README claimed every page becomes a chunk; say what actually happens and pin it with a test. Signed-off-by: Honglin Cao <hocao@nvidia.com>
warnings.catch_warnings swaps the module-global filter list, not a thread-local one, so the window silently dropped every other thread's DeprecationWarning for the duration of the construction. Keeping it cost more than the duplicate warning it hid, and the two warnings are not redundant: one names the factory the caller used, the other the class it keeps using afterwards. Without it the method body is main's again, and warnings is no longer imported. Signed-off-by: Honglin Cao <hocao@nvidia.com>
The read path is rate limited upstream on a bucket shared by every caller, and fetch_logs is the reader that reaches it: chunk_size is the request size, so the default walks a large window in thousands of round trips. A 503 used to take the generator with it, losing the anchor and the position that make the caller's bookkeeping unnecessary in the first place. There is no server-issued cursor, so a page request is a pure function of its anchor and re-issuing it can neither duplicate nor skip lines. Retry inside the loop, where the anchor is still in scope, with exponential backoff and jitter against the shared bucket, honouring Retry-After when the server sends one. Only 503 retries: a rejected request does not get better by repetition. Only fetch_logs. The single-page readers lose nothing when a page fails and their callers can decide for themselves whether to wait. Signed-off-by: Honglin Cao <hocao@nvidia.com>
Nothing on the read path sends Retry-After: the API answers with a bare FastAPI HTTPException and the ingress rate limiter adds no headers either, so that branch was shaping a delay no server asks for. The ceiling was unreachable too — four doublings from half a second top out at four, jitter included. What is left is the backoff itself. Name the jitter fraction rather than spelling the band inline, and cover the growth the constants describe. The give-up test now exhausts the budget on a later page, so it also pins that an already delivered chunk stands. Signed-off-by: Honglin Cao <hocao@nvidia.com>
The default was 10 back when a buffer sat between the wire page and the chunk, and nothing re-derived it once chunk_size became the page size itself. The value still suits the common call — a tail, where the store has a handful of new lines to give whatever the page size — so keep it, but name it and say why it sits below the server's own page default. Attributing each line to its pod only made sense while one call could merge several pods. It cannot any more: pod is required and the example already names it in the header. The example's formatter is main's again. Carry the fully-filtered-page correction into the example comment, the third copy of a sentence the last commit fixed in two. Bind the page call with partial rather than a lambda default argument: same protection against the loop variable, and the callable types cleanly. Assert that each backoff outgrows the last instead of restating the expression that produces it. Signed-off-by: Honglin Cao <hocao@nvidia.com>
Strictly increasing waits let a regression to a fixed interval through whenever four jittered samples happen to land in ascending order — about one run in twenty-four. Comparing the last wait against the first closes that: doubling clears the margin every time, a fixed interval never does. Signed-off-by: Honglin Cao <hocao@nvidia.com>
Reading pods[0] without printing the roster hides from a multi-replica reader that there were other pods to choose from. Running the two demonstrated shapes back to back is also the one composition that loses lines: the tail begins at its own "now", so whatever was logged between the window's end_time and that moment belongs to neither read. Measured against the local environment: 16 lines written while the window read paged, 15 of them never delivered. One generator with an earlier start_time and no end_time covers both and has no such window — same test, nothing missing. Say so where someone is about to copy the wrong pair. Signed-off-by: Honglin Cao <hocao@nvidia.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.
Problem
Deployment log reading was spread over three stateful readers (
get_deployment_logs,get_deployment_logs_range,deployment_log_session), and none of them could stream one pod's logs lazily with bounded memory. Tailing a running deployment meant re-creating a reader over overlapping windows and keeping aseendict in user code to drop the server's late-arrival re-deliveries — bookkeeping the SDK should absorb.Change
fetch_logs(deployment_id, revision_number, pod, start_time=None, end_time=None, chunk_size=10)is the whole surface.podis required (discover names withget_deployment_pods()).start_timedefaults to the current time and is resolved whenfetch_logsis called rather than at the firstnext(), so lines logged while the generator sits unstarted are not skipped.chunk_sizeis the number of lines requested per round trip (max_lineson the wire), so it inherits the server's 1..5000 ceiling and is validated eagerly at the call. Each server page is yielded as one chunk: usually up tochunk_sizelines, smaller when lines belowstart_timeor already-delivered re-deliveries are filtered out of the page, larger when one millisecond holds more thanchunk_sizelines — the server never splits a millisecond across pages. A bulk read of history wants a largechunk_size; the log read path is rate-limited upstream, and a smallchunk_sizeover a large window multiplies requests.With
end_timeset the iterator terminates once the window is delivered or the store has no more lines to give, whichever comes first — anend_timein the future does not keep it polling until then. Withoutend_timethe iterator never terminates: once caught up it yields an empty chunk each time nothing new is stored yet, and the caller decides when to sleep or break. A page filtered away entirely yields nothing at all rather than that empty chunk, which means only that the stream is caught up.The dedup anchor lives inside the generator for its whole lifetime and is trimmed by time (
LOG_DEDUP_RETENTION_MS), which is what bounds its memory. Every stored line is delivered at most once and callers need no cross-call dedup.A page the store answers as busy (HTTP 503 — the read path is rate limited upstream on a bucket shared by every caller) is retried inside the loop, where the anchor is still in scope, with exponential backoff and jitter. There is no server-issued cursor, so a page request is a pure function of its anchor and re-issuing it can neither duplicate nor skip lines. Only 503 retries, and only here: the single-page readers lose nothing when a page fails and their callers can decide for themselves whether to wait. Nothing on this path sends
Retry-Aftertoday — neither the API'sHTTPExceptionnor the ingress limiter — so the SDK does not read one.If a page request fails for any other reason, or the retries run out, the iterator raises and, like any generator, cannot be resumed — but every chunk already yielded is complete and none is left half-built. The docstring gives the resume recipe: re-anchor a new
fetch_logson the last delivered event's timestamp, which re-delivers only the lines sharing that millisecond.Lines inside a chunk are always ascending by
(timestamp, id). The server orders a page by nanosecond timestamp only, never by the id's hash suffix, so lines sharing one nanosecond can arrive in either id order; the SDK re-inserts them.get_deployment_logs,get_deployment_logs_range,deployment_log_sessionand theDeploymentLogSessionclass carrytyping_extensions.deprecated(PEP 702, the 3.10-compatible backport ofwarnings.deprecated) and keep working unchanged.typing-extensionsis now a declared dependency rather than a transitive one. SDK-internal paging goes through the private_fetch_log_page, so it does not trip the warning. Thepytest.inideprecation filter is gone; the deprecated readers' tests assert the warning withpytest.warns(DeprecationWarning).get_deployment_pods()is unchanged.README is one short paragraph plus one example each for a window read and a tail.
examples/sdk/get_deployment_logs.pyshows the same two shapes over a single generator, with noseendict and no overlap window.Test plan
End-to-end against the local v2 environment at
d4bd070: the real API behind the k3dingress, real Loki behind it, synthetic lines pushed under the labels deployment 1729
revision 9 already uses.
get_deployment_pods()returned 10 pods, terminated ones includedDeploymentLogEvent(id='<nanoseconds>-<hash>', timestamp=<epoch ms>, message=..., pod=...); the wire carries no pod, the SDK attaches itchunk_size1 / 10 / 100 / 5000 — 314 / 34 / 5 / 2 requests, 0 duplicates, iterator terminates on its ownchunk_size=7every request sentmax_lines=7; 46 requests over the same windowchunk_size=10, chunks of 9 with a final 7: the server withholds each full page's trailing millisecond and the next page re-covers itchunk_size=5(chunk lengths 4, 12, 4)start_timeandend_timeboth inclusive; an inner window trims both sidesnext()start_timepinned at the callchunk_size0 / 5001, negative bounds andstart_time > end_timeeach raisedValueErrorat the call, before any requestfetch_logs(pod='no-such-pod')yielded nothingNotFoundException404,{"detail":"Revision number 99999 not found for deployment 1729"}chunk_size=1read of 300 lines: 365 requests, 64 backoffs, 300/300 delivered, 0 duplicates, 42 s. The same shape died withServiceException(503)at request ~32 before the retryget_deployment_logs()emits oneDeprecationWarning,deployment_log_session()emits two — one for the factory, one for the session class it hands back