-
-
Notifications
You must be signed in to change notification settings - Fork 234
feat(logs): support log streaming #2666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
93b8792
to
90ba3c8
Compare
src/commands/logs/list.rs
Outdated
} | ||
|
||
// Maintain buffer size limit by removing oldest entries | ||
while self.buffer.len() > self.max_size { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done to prevent OOM in case this keeps running for a long time
90ba3c8
to
44a987b
Compare
Signed-off-by: Vjeran Grozdanic <[email protected]>
44a987b
to
d18413f
Compare
src/utils/formatting.rs
Outdated
format.separator( | ||
prettytable::format::LinePosition::Title, | ||
prettytable::format::LineSeparator::new('-', '+', '+', '+'), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to do custom formatting, because every other formatting adds empty lines after printing out the table. We want it to seem like it's infinite table, without empty lines in between printing multiple rows, so we need to use FORMAT_NO_BORDER
, and define our own line separator for header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revisit whether it makes sense to output streamed logs in the table format? I really think it would be better, certainly in terms of making the implementation easier, but possibly also from a usability standpoint, if we just outputted them in an unstructured format. Or, if we also want to support a structured format for AI agents, I would suggest a separate mode that outputs in JSON or CSV, which could be enabled with a --structured
flag.
My concern with table format is that the implementation is complex, it's not human readable unless you widen your terminal window, and also it probably is not a very standard format for LLMs compared with something like JSON or CSV. I think we can do better by having the default log format look like the logs that Sentry CLI outputs (human-readable), with a structured logging option.
What are your thoughts?
Deferring review until after HackWeek – please ping me if you need a review more urgently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes, as the deduplication logic seems strange/possibly wrong, and we can improve it. Please see inline comment for details 👍
src/utils/formatting.rs
Outdated
format.separator( | ||
prettytable::format::LinePosition::Title, | ||
prettytable::format::LineSeparator::new('-', '+', '+', '+'), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revisit whether it makes sense to output streamed logs in the table format? I really think it would be better, certainly in terms of making the implementation easier, but possibly also from a usability standpoint, if we just outputted them in an unstructured format. Or, if we also want to support a structured format for AI agents, I would suggest a separate mode that outputs in JSON or CSV, which could be enabled with a --structured
flag.
My concern with table format is that the implementation is complex, it's not human readable unless you widen your terminal window, and also it probably is not a very standard format for LLMs compared with something like JSON or CSV. I think we can do better by having the default log format look like the logs that Sentry CLI outputs (human-readable), with a structured logging option.
What are your thoughts?
src/commands/logs/list.rs
Outdated
fields: &[&str], | ||
args: &ListLogsArgs, | ||
) -> Result<()> { | ||
let mut deduplicator = LogDeduplicator::new(MAX_DEDUP_BUFFER_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have the max size here be the set to the number of logs we are fetching per request? We probably would need to add that as a parameter to the signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my thinking process: If we are always fetching the most recent
- Suppose fetch
$n$ includes logs$\ell_1, \ell_2, ..., \ell_m$ , where$m$ is maximum number of logs. - Suppose fetch
$n+1$ then includes logs$\ell_{m+1}, \ell_{m+2}, ..., \ell_{2m}$ . Importantly, no logs are repeated. - If the timestamps cannot be changed and no logs can be changed, then it is not possible for fetch
$n+2$ to contain any of$\ell_1, \ell_2, ..., \ell_m$ , as they would have also been included in$n+1$ . - That means, we only need to cache at most
$m$ entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense, i'll update the code
Co-authored-by: Daniel Szoke <[email protected]>
src/commands/logs/list.rs
Outdated
args: &ListLogsArgs, | ||
) -> Result<()> { | ||
let mut deduplicator = | ||
LogDeduplicator::new(NonZero::new(args.max_rows).expect("max-rows should be non-zero")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal to update the max_rows
field in the struct to also have a NonZeroUsize
type. That way, we don't need the expect
, because the nonzero value is enforced through the type system. But, we can do that in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have updated it as a part of this PR
Adds support for live streaming of logs by polling the API every 2 seconds (by default).
Polling interval is configurable via command line argument.