hrxi / tracing-loki

A tracing layer for shipping logs to Grafana Loki
Apache License 2.0
50 stars 20 forks source link

Use original level strings #3

Closed j4nk3e closed 2 years ago

j4nk3e commented 2 years ago

The level_str breaks the color coding of log levels e.g. in Grafana. Using the original as_str from tracing_core::Level fixes this issue.

hrxi commented 2 years ago

"Original level strings" seems to mean "level strings from tracing_core which have no relation to Loki".

Could the strings be taken from https://grafana.com/docs/grafana/latest/packages_api/data/loglevel/ instead?

Member Value
alert "critical"
crit "critical"
critical "critical"
dbug "debug"
debug "debug"
emerg "critical"
eror "error"
err "error" 
error "error"
fatal "critical"
info "info"
information "info"
informational "info"
notice "info"
trace "trace"
unknown "unknown"
warn "warning"
warning "warning"

So perhaps "trace", "debug", "info", "warning", "error"?

j4nk3e commented 2 years ago

You're right about the tracing_core strings having no relation to Loki, but since there is no best practice for Loki (at least I couldn't find one) and it uses the same string representations as the log package (disregarding the case, see https://docs.rs/log/latest/log/enum.Level.html#variants), I consider it being a standard at least in Rust.

The issue I had with Grafana actually comes from the logs integration, which seems to have a different mapping than what you posted:

Supported expressions Log level Color
emerg critical purple
fatal critical purple
alert critical purple
crit critical purple
critical critical purple
err error red
eror error red
error error red
warn warning yellow
warning warning yellow
info info green
information info green
notice info green
dbug debug blue
debug debug blue
trace trace light blue
* unknown grey

Source: https://grafana.com/docs/grafana/latest/explore/logs-integration/

The string informational is missing here, which also was the root cause of my issue. The coloring works fine with my version, so I assume the case is ignored.

So perhaps "trace", "debug", "info", "warning", "error"?

That would work with the logs integration of Grafana. What would be the benefit of using this mapping (which differs besides the case only in warn vs. warning) in contrast to Level.as_str? Going with Level.as_str is in my opinion the easier implementation, because we don't have to maintain another string mapping (that is not obvious to the user in the first place - I only figured it out after reading the source code of the BackgroundTask implementation) and also matches the default level output of e.g. tracing_subscriber::fmt. Is there maybe some Grafana/Loki/Rust or general best practice I'm not aware of or compatibility issue with different software? Please enlighten me!

hrxi commented 2 years ago

Going with Level.as_str is in my opinion the easier implementation

"easier" is very far back in my list of implementation concerns. First come correctness and sensibility.

I can see the argument that matching the default level output of tracing_subscriber::fmt is good.

I would somehow assume that uppercase is uncommon in structured logs, but that was just a gut feeling not backed by any data. The screenshot in your documentation link does show lowercase log levels. I'll try looking for some other Loki client libraries.

hrxi commented 2 years ago

I only found these:

https://github.com/grafana/loki/blob/31ca108a3cf5bcb87ffeb54d06d6fa4c53e4830d/clients/pkg/promtail/targets/gelf/gelftarget.go#L24-L34

https://github.com/grafana/loki/blob/31ca108a3cf5bcb87ffeb54d06d6fa4c53e4830d/clients/pkg/promtail/targets/journal/journaltarget.go#L363-L383

So maybe go with the lowercased variants of the tracing levels? "error", "warn", "info", "debug", "trace"?

j4nk3e commented 2 years ago

sounds good, I updated the PR

hrxi commented 2 years ago

sounds good, I updated the PR

Sorry, one last thing: Could you update it to use a function again? Currently it lowercases the log level (and even allocates for it) for each log message, which is a bit wasteful.

hrxi commented 2 years ago

Thanks!

hrxi commented 2 years ago

Released a new version tracing-loki 0.2.0 on crates.io.

j4nk3e commented 2 years ago

Thank you!