Skip to content
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

Add config option to enable json logging #5557

Open
wants to merge 1 commit into
base: release/v0.19
Choose a base branch
from

Conversation

MrKaplan-lw
Copy link

This is backported from 97772b9

#5471
97772b9

This introduces a bit of code duplication due to type differences when json vs non-json logging are used in combination with opentelemetry.
As opentelemetry has been removed in main it's simpler there.

@MrKaplan-lw
Copy link
Author

CI fails due to SQL formatting in migrations, but they weren't touched in this PR

let format_layer = {
let layer = tracing_subscriber::fmt::layer().with_ansi(false).json();
layer.with_filter(targets.clone())
};
Copy link
Member

Choose a reason for hiding this comment

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

Only this part needs to be inside the if, something like this:

let layer = if settings.json_logging {
    tracing_subscriber::fmt::layer().with_ansi(false).json();
} else {
    tracing_subscriber::fmt::layer().with_ansi(false).json();
}
let format_layer = layer.with_filter(targets.clone());
...

Then its not necessary to write all the stuff for opentelemetry etc twice.

Copy link
Author

@MrKaplan-lw MrKaplan-lw Mar 28, 2025

Choose a reason for hiding this comment

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

you included .json() there twice.

i've tried to do it that way first but it won't compile due to different types being returned by the if branches:

   Compiling lemmy_server v0.19.10 (.../lemmy)
error[E0308]: `if` and `else` have incompatible types
   --> src/lib.rs:401:5
    |
398 |     let layer = if settings.json_logging {
    |  _______________-
399 | |     tracing_subscriber::fmt::layer().with_ansi(false).json()
    | |     -------------------------------------------------------- expected because of this
400 | |   } else {
401 | |     tracing_subscriber::fmt::layer().with_ansi(false)
    | |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Layer<_, JsonFields, Format<Json>>`, found `Layer<_>`
402 | |   };
    | |___- `if` and `else` have incompatible types
    |
    = note: expected struct `tracing_subscriber::fmt::Layer<_, JsonFields, tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::Json>>`
               found struct `tracing_subscriber::fmt::Layer<_, DefaultFields, tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::Full>>`

perhaps this could be solved with an explicit more generic type annotation, but i'm not sure which one it would need.

@Nutomic
Copy link
Member

Nutomic commented Mar 28, 2025

CI is fixed with #5558 so you can merge those changes.

@dessalines
Copy link
Member

Make sure to do this PR to main also.

@Nutomic
Copy link
Member

Nutomic commented Mar 28, 2025

This is a backport from main to 0.19

@MrKaplan-lw
Copy link
Author

@Nutomic it seems that you have mixed up branch names while backporting other changes.

the branch release/v0.19, which was previously used for tracking 0.19 updates, only has 2 commits since 0.19.10, but the branch release/0.19 has various other backports you did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants