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

Missing filtrationMode bloats Loki labels #81

Closed
1 task done
AntonPetrov83 opened this issue Feb 1, 2022 · 4 comments · Fixed by #101
Closed
1 task done

Missing filtrationMode bloats Loki labels #81

AntonPetrov83 opened this issue Feb 1, 2022 · 4 comments · Fixed by #101
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@AntonPetrov83
Copy link

AntonPetrov83 commented Feb 1, 2022

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

7.1.0

Which version of .NET are you using?

net5.0

Describe the bug

I left label filtration mode uninitialized during configuration and received labels I was not expected. This is because of current implementation:

private bool IsAllowedByFilter(string label) =>
            _filtrationMode switch
            {
                LokiLabelFiltrationMode.Include => IsInFilterList(label),
                LokiLabelFiltrationMode.Exclude => !IsInFilterList(label),
                null => true,
                _ => true
            };

I think this is a very bad design decision because overusing labels downgrades Loki performance. I suggest that the default behaviour of IsAllowedByFilter should be to return false.

Currently it looks like it is not possible to ignore label filtration logics at all.

To Reproduce

Expected behavior

Log/SelfLog output or exception with stacktrace

No response

Application or code sample, which could be used to reproduce a bug

No response

Additional context

No response

I have read the documentation

@AntonPetrov83 AntonPetrov83 added the bug Something isn't working label Feb 1, 2022
@mishamyte
Copy link
Member

The current behavior is works as expected, as for me.

  1. The labels are important for Loki, this is described in wiki
  2. If there are no filtration mode it is oblivious that all data would be returned (it is the common behavior for filters anywhere. If you won't specify Where in EF - it won't do a filtration)
  3. Changing behavior to your version will increase a count of issues kind of "Sink is not working!1"

So I don't think this is the way we're going.

Facing the performance issues, related to not using labels is a good point to go the Loki + this sink doc and read it

@AntonPetrov83
Copy link
Author

  1. Agree. And you say it in your docs: "Big count of streams can kill Loki and called high cardinality." - in my opinion a default configuration of a sink must not "kill" anything ))) that is why I created this issue. We should avoid creating dangerous defaults.

  2. Can you imagine any case when filtrationMode = null will be a desired behaviour? I doubt it and right now this is a default value. Because filtration implies some performance impact I would make this entire feature to be optional. Would be great to say "I do not need automatic labeling at all."

  3. Understood. I guess this is because some labels are required by Loki. Then LokiSink should throw an exception if no labels were configured.

Thank you for your time answering this 👍

@mishamyte
Copy link
Member

About the 2nd point, yep, you are right, but in my practice we always left some values that should be mapped to labels and config was like Include + important values.

I will think about the possibility to turn off labeling and how it could live with the other planned features

@mishamyte mishamyte added enhancement New feature or request documentation Improvements or additions to documentation investigation needed The investigation is needed and removed bug Something isn't working labels Feb 1, 2022
@mishamyte mishamyte added this to the Backlog milestone Feb 1, 2022
This was referenced Mar 31, 2022
@mishamyte
Copy link
Member

@AntonPetrov83 the change of behavior was implemented in v8.0.0-beta.0

Now you explicitly specify what properties will be mapped to the labels. Rest will be appended in the body (by default formatter). If you will define no list - no labels would be created.

More details in #99

@mishamyte mishamyte removed documentation Improvements or additions to documentation investigation needed The investigation is needed labels Apr 1, 2022
@mishamyte mishamyte modified the milestones: Backlog, v8.0.0 Apr 1, 2022
@mishamyte mishamyte linked a pull request Apr 1, 2022 that will close this issue
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

2 participants