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

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

Open
mishamyte opened this issue Oct 7, 2022 Discussed in #137 · 9 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@mishamyte
Copy link
Member

Discussed in #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 mishamyte added the bug Something isn't working label Oct 7, 2022
@mishamyte mishamyte self-assigned this Oct 7, 2022
@mishamyte
Copy link
Member Author

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
Copy link
Member Author

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

@mishamyte mishamyte added this to the v9.0.0 milestone Oct 9, 2022
@EraYaN
Copy link
Contributor

EraYaN commented Nov 18, 2022

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
Copy link
Member Author

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
Copy link
Contributor

EraYaN commented Nov 18, 2022

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
Copy link
Member Author

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

@EraYaN
Copy link
Contributor

EraYaN commented Nov 18, 2022

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

@mishamyte
Copy link
Member Author

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 pushed a commit that referenced this issue Nov 25, 2022
…abels (#154)

* Add opt out to filtering the properties list when using propertiesAsLabels

Workaround for #138

* Added missing XML comment
@mishamyte
Copy link
Member Author

Thanks once again!

Released as v8.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants