Add absolute time-window filtering to miren logs#868
Merged
phinze merged 2 commits intoJun 29, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (35)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR adds an optional upper time bound to streamed logs across the RPC API, CLI, observability query path, and server handling. It also updates generated RPC metadata to include explicit parameter names, and adds client/server introspection support for checking method parameters. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/commands/logs.go (1)
113-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider rejecting an inverted window (
--sinceafter--until).
resolveLogWindowvalidates flag conflicts but never checks thatfrom <= to. Inputs like--since 1h --until 2h(or--since "14:30" --until "14:00") resolve to a window where the start is after the end, which silently yields confusing/empty results rather than an actionable error. Since both bounds are already materialized here, a cheap guard improves UX.Note: if the server already rejects inverted ranges, defer to it to keep validation centralized.
🛡️ Optional guard
if until != "" { t, perr := parseTimeFlag(until, now) if perr != nil { return nil, nil, fmt.Errorf("invalid --until value %q: %w", until, perr) } to = standard.ToTimestamp(t) } + if from != nil && to != nil && standard.FromTimestamp(to).Before(standard.FromTimestamp(from)) { + return nil, nil, fmt.Errorf("--until is before --since; the window is empty") + } + return from, to, nilBased on learnings: prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client.
🤖 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 `@cli/commands/logs.go` around lines 113 - 141, resolveLogWindow currently checks flag conflicts but does not reject an inverted time window where the computed from timestamp is after to. Update resolveLogWindow to compare the materialized bounds after parsing --since/--last and --until, and return a clear error when from > to unless this is already enforced by the server. Keep the change localized to resolveLogWindow and use the existing parseTimeFlag and standard.ToTimestamp flow so the validation stays consistent.Source: Learnings
🤖 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.
Inline comments:
In `@api/app/rpc.yml`:
- Around line 563-565: The streamLogChunks parameter order has changed in a way
that renumbers CBOR keys and can break mixed-version compatibility. Update the
streamLogChunks definition in rpc.yml so the optional `to` field is appended
after `chunks` rather than inserted before `follow`, `filter`, and `chunks`, or
explicitly pin field indices to preserve the wire contract. Use the
`streamLogChunks` params block and the `to` field as the anchor points when
making the change.
---
Nitpick comments:
In `@cli/commands/logs.go`:
- Around line 113-141: resolveLogWindow currently checks flag conflicts but does
not reject an inverted time window where the computed from timestamp is after
to. Update resolveLogWindow to compare the materialized bounds after parsing
--since/--last and --until, and return a clear error when from > to unless this
is already enforced by the server. Keep the change localized to resolveLogWindow
and use the existing parseTimeFlag and standard.ToTimestamp flow so the
validation stays consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b29680d-1555-4c3e-a4b7-10f26361a650
📒 Files selected for processing (13)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlcli/commands/logs.gocli/commands/logs_doc.gocli/commands/logs_test.godocs/docs/command/logs-app.mddocs/docs/command/logs-build.mddocs/docs/command/logs-sandbox.mddocs/docs/command/logs-system.mddocs/docs/command/logs.mdobservability/logs.goservers/logs/logs.goservers/logs/logs_test.go
phinze
force-pushed
the
phinze/mir-1226-miren-logs-add-absolute-time-window-filtering-since-until
branch
from
671198c to
ae89884
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/commands/logs.go (1)
329-347: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t gate
--untilsupport on method presence alone.Line 330 only checks for
streamLogChunks, but that method predates the newTofield. A server that hasstreamLogChunksbut was generated before this RPC change will take Line 340, so--untilmay be ignored or fail generically instead of returning the clean newer-server error on Line 347. Please gate this on a schema/protocol capability that specifically confirmsTosupport, or introduce a versioned method/capability for bounded chunk streaming.🤖 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 `@cli/commands/logs.go` around lines 329 - 347, Gate the --until path in logs.go using a capability that specifically indicates bounded chunk streaming support, not just HasMethod(ctx, "streamLogChunks"), because older servers may expose streamLogChunks without the To field. Update the logic around streamLogChunks and the --until check so a server without To support returns the explicit newer-server error instead of attempting the call and failing generically; if needed, add or use a versioned capability/method name that clearly identifies To-enabled streaming.
🤖 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.
Outside diff comments:
In `@cli/commands/logs.go`:
- Around line 329-347: Gate the --until path in logs.go using a capability that
specifically indicates bounded chunk streaming support, not just HasMethod(ctx,
"streamLogChunks"), because older servers may expose streamLogChunks without the
To field. Update the logic around streamLogChunks and the --until check so a
server without To support returns the explicit newer-server error instead of
attempting the call and failing generically; if needed, add or use a versioned
capability/method name that clearly identifies To-enabled streaming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea178b8c-9974-4895-abfb-95904b0eee06
📒 Files selected for processing (13)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlcli/commands/logs.gocli/commands/logs_doc.gocli/commands/logs_test.godocs/docs/command/logs-app.mddocs/docs/command/logs-build.mddocs/docs/command/logs-sandbox.mddocs/docs/command/logs-system.mddocs/docs/command/logs.mdobservability/logs.goservers/logs/logs.goservers/logs/logs_test.go
✅ Files skipped from review due to trivial changes (4)
- docs/docs/command/logs-sandbox.md
- docs/docs/command/logs-build.md
- docs/docs/command/logs-app.md
- docs/docs/command/logs-system.md
🚧 Files skipped from review as they are similar to previous changes (8)
- cli/commands/logs_doc.go
- docs/docs/command/logs.md
- api/app/app_v1alpha/rpc.gen.go
- cli/commands/logs_test.go
- servers/logs/logs.go
- observability/logs.go
- api/app/rpc.yml
- servers/logs/logs_test.go
phinze
force-pushed
the
phinze/mir-1226-miren-logs-add-absolute-time-window-filtering-since-until
branch
from
ae89884 to
3603c8d
Compare
Method discovery (the /_rpc/methods endpoint behind HasMethod) only ever reported method names, so a client could tell whether a server had a method but not whether it understood a particular parameter. That makes it impossible to gate a newly added parameter cleanly: a server with the method but an older signature silently ignores the new field rather than refusing it. This teaches discovery to report each method's parameter names alongside the method list, and adds HasMethodParam so a client can ask whether a server accepts a specific parameter. The generated method registry now carries the schema parameter names, the server returns them from introspection, and the in-process LocalClient answers the same way so both transports agree. The response field is additive, so older servers simply omit it and clients read its absence as "can't report parameters," i.e. unsupported.
`miren logs` could only ever look backward from now: --last set the start and the end was pinned to the present. Chasing an incident with a known timeframe meant scrolling through everything since. This adds --since/--until to all four log subcommands (app, sandbox, build, system), following the docker/journalctl idiom so it matches muscle memory. Each accepts an RFC3339 timestamp, a friendlier naive form like "2006-01-02 15:04" (parsed in local time), or a Go duration read as "ago". --last still works and is now just shorthand for --since with a duration. The end bound is the genuinely new wiring: streamLogChunks gained a `to` parameter, logReadOpts grew an Until field, and executeStreamQuery now uses it instead of hardcoding time.Now(). --since rides the existing `from` and works on every protocol. --until needs the modern streamLogChunks path, and it is gated on the server actually accepting that method's `to` parameter (via the RPC parameter-level capability check), so a server too old to know `to` returns a clean "newer server required" error instead of silently dropping the bound. This also closes the same latent gap for --service/--build, which depend on the `filter` parameter. Two notes on the wire format. The RPC layer assigns CBOR field keys by parameter position, so `to` is appended after the existing streamLogChunks parameters; inserting it earlier would renumber follow/filter/chunks and break streaming against mixed-version peers mid-rollout. Separately, the CLI rejects an inverted window (--until before --since) up front.
phinze
force-pushed
the
phinze/mir-1226-miren-logs-add-absolute-time-window-filtering-since-until
branch
from
3603c8d to
4bc9b0e
Compare
Contributor
Author
|
Addressed in commit |
|
✅ Action performedComments resolved. Approval is disabled; enable |
phinze
deleted the
phinze/mir-1226-miren-logs-add-absolute-time-window-filtering-since-until
branch
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
2 participants
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.
miren logscould only ever look backward from now. The one time control was--last <duration>, which set the start tonow - durationand always pinned the end to the present. That's fine for "what just happened," but useless when you're chasing an incident with a known window and don't want to scroll through everything since 2pm to see what happened between 2:00 and 2:30.So this adds a
--since/--untilpair to all four log subcommands (app,sandbox,build,system). Rather than invent our own vocabulary, we leaned into what people already have in their fingers from docker, journalctl, and git:--since/--until, each overloaded to take either a timestamp or a duration. You can write a full RFC3339 timestamp for a precise boundary, a friendlier naive form like"2026-06-25 14:00"(interpreted in your local timezone), or just a duration like2hthat's read as "that long ago."--since 2h --until 30mgives you the window from two hours ago to thirty minutes ago. The old--lastflag still works, it's just shorthand for--sincewith a duration now, so nobody has to relearn anything.Most of the backend was already in shape for this. The query layer always called into VictoriaLogs with both a start and an end, but the end was hardcoded to
time.Now()and there was no way to pass anything else through. The real work was threading an upper bound the rest of the way:streamLogChunksgained atoparameter,logReadOptsgrew anUntilfield, andexecuteStreamQuerynow honors it.--sincewas almost free since it just produces afromtimestamp that every protocol version already accepts. A couple of guardrails fall out naturally too:--untilwith--followis rejected (a live tail has no end),--sincealongside--lastis rejected (they'd fight over the start), and an inverted window (--untilbefore--since) is caught up front.While wiring this up, the obvious gate had a hole:
--untilwould be allowed whenever the server hadstreamLogChunks, but that method predates thetoparameter, so a server in that in-between range would silently ignore the bound. Since the whole fleet released to date is in exactly that range, that's not a rare edge. So the RPC discovery layer now reports parameter names alongside method names, and--untilchecks for realtosupport before sending it. Older servers get a clean "newer server required" error instead of a silently-dropped bound, and the same fix covers the latent--service/--buildcase for free. That capability work is the first commit; the logs feature that consumes it is the second.Closes MIR-1226