-
Notifications
You must be signed in to change notification settings - Fork 423
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
Output logs in JSON with QW_LOG_FORMAT=json
#5672
Conversation
quickwit/quickwit-cli/src/logger.rs
Outdated
|
||
enum EventFormat<'a> { | ||
Full(Format<Full, UtcTime<Vec<BorrowedFormatItem<'a>>>>), | ||
Json(tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::Json>), |
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.
Json(tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::Json>), | |
Json(Format<tracing_subscriber::fmt::format::Json>), |
quickwit/quickwit-cli/src/logger.rs
Outdated
enum EventFormat<'a> { | ||
Full(Format<Full, UtcTime<Vec<BorrowedFormatItem<'a>>>>), | ||
Json(tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::Json>), | ||
} |
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.
Is there any strong reason for using a custom event format rather than a boxed layer? Maybe from a performance point of view enum is a tiny bit better, but boxing would be substantially simpler.
quickwit/quickwit/quickwit-lambda/src/logger.rs
Lines 92 to 102 in f946626
fn fmt_layer<S>(level: Level) -> Box<dyn Layer<S> + Send + Sync + 'static> | |
where | |
S: for<'a> LookupSpan<'a>, | |
S: tracing::Subscriber, | |
{ | |
if *ENABLE_VERBOSE_JSON_LOGS { | |
json_fmt_layer(level).boxed() | |
} else { | |
compact_fmt_layer(level).boxed() | |
} | |
} |
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 assumed we didn't go for a boxed implementation in the first place for a good reason, so I also added unboxed code. I'm open to a refactor that uses the same code in cli
and lambda
and potentially uses boxed layers.
registry | ||
.with(telemetry_layer) | ||
.with( | ||
tracing_subscriber::fmt::layer() | ||
.event_format(event_format) | ||
.fmt_fields(fmt_fields) | ||
.with_ansi(ansi_colors), |
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.
what does it mean to set ansi
to true if the format is json?
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.
No op.
f946626
to
9d19d56
Compare
Description
Per title.
How was this PR tested?
Ran locally and inspected logs manually