serilog-contrib / serilog-sinks-grafana-loki

A Serilog sink sending log events to Grafana Loki
MIT License
197 stars 30 forks source link

Property doesn't render correctly if property, used as a part of it is mapped to the label #138

Open mishamyte opened 2 years ago

mishamyte commented 2 years ago

Discussed in https://github.com/serilog-contrib/serilog-sinks-grafana-loki/discussions/137

Originally posted by **Depechie** October 7, 2022 Was wondering if we are doing something wrong, or misjudging the features? Currently we use Serilog to format our log entries and use the loki sink to push it in json format. If we look at a result in Loki we get this Screenshot 2022-10-07 155644 If we now use the propertiesAsLabel feature we get following result Screenshot 2022-10-07 160254 So the actual properties are indeed transformed, but that seems to also impact the payload structure... the Message line now no longer maps the data but only show the placeholders, like {RequestPath}. Is this expected behaviour?

Which version of Serilog.Sinks.Grafana.Loki are you using?

v8.0.0

mishamyte commented 2 years ago

The problem is TextFormatter gets LogEvent without properties, that will be mapped to labels. This behavior exists because we don't want to create duplicates (properties, which would be both labels and body rendering).

So the way should be found how pass it to formatter and use for message rendering, but not render as a part of the body

mishamyte commented 2 years ago

Seems like it is not possible to do with current abstractions, should be a point of review and breaking changes

EraYaN commented 1 year ago

Honestly for our use case leaving the labels as properties (and thus having a "duplicate") would be an acceptable solution. At least one we could opt in too. Basically leave them in the property list and just extract them towards labels, it's how it used to work for v7 and earlier it seems.

Say an option that opts out of this partitioning: https://github.com/serilog-contrib/serilog-sinks-grafana-loki/blob/master/src/Serilog.Sinks.Grafana.Loki/LokiBatchFormatter.cs#L185

mishamyte commented 1 year ago

Honestly for our use case leaving the labels as properties (and thus having a "duplicate") would be an acceptable solution. At least one we could opt in too. Basically leave them in the property list and just extract them towards labels, it's how it used to work for v7 and earlier it seems.

Say an option that opts out of this partitioning: https://github.com/serilog-contrib/serilog-sinks-grafana-loki/blob/master/src/Serilog.Sinks.Grafana.Loki/LokiBatchFormatter.cs#L185

Yep, this is the problem. To formatter we pass the properties, were not mapped to labels. So while formatter tries to render a Message (for example), rendering input doesn't consist of all needed tokens for rendering.

This is a reason why it is not possible without breaking contracts or scary workarounds fix it in v8.

But this is the point of fix for v9

EraYaN commented 1 year ago

I'll whip up a quick fork for our internal use that could be released in the v8 line of versions if you want. It's a bit of a workaround (the opt-out) but it also doesn't break anything.

mishamyte commented 1 year ago

Yep, see your solution. Seems nice as a workaround for now. Let me think

EraYaN commented 1 year ago

Cool, if you need me to rename the parameter or anything like that let me know.

mishamyte commented 1 year ago

Thanks for your work, @EraYaN!

I think we should add it to the current versions as a workaround for the situation. Will prepare the release

mishamyte commented 1 year ago

Thanks once again!

Released as v8.1.0