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

Remove default labels #103

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

IppX
Copy link

@IppX IppX commented Feb 15, 2022

Motivation

Labels in Loki should be added carefully and kept to a minimum, since it directly impacts how logs are stored and performance see here. They shouldn't be dynamically added based on log content by default.
Also, in the current implementation labels and level are not added to the log line, which can be an issue when using a custom format.

Description

  • Remove the default level, label and labels label and change the default format to a json representation of the info object. This enables the user in Loki to then use the | json parser to extract those labels if needed. This also solves the issue with custom formats.
  • Fix the tests

Limit

The current drawback is that there is no way to add a label dynamically anymore. If that's a must have feature then I can add another option, to specify a list of keys from the log entry that should be extracted into labels (similarly to the official fluentbit output for example).

Let me know what you think and thanks for putting this adapter together :)

@JaniAnttonen
Copy link
Owner

JaniAnttonen commented Feb 15, 2022

Seems like a good idea to make the adapter be more idiomatic towards Loki! This would become a new major release with some, if not all, changes presented here, together with a fix to #90 (comment) and possibly some other issues :)

@IppX
Copy link
Author

IppX commented Feb 16, 2022

Seems like a good idea to make the adapter be more idiomatic towards Loki! This would become a new major release with some, if not all, changes presented here, together with a fix to #90 (comment) and possibly some other issues :)

Let me know how I can support you here ;)

index.js Show resolved Hide resolved
@Alexsey
Copy link
Contributor

Alexsey commented Jun 21, 2022

According to current official recommendations, dynamic labels are ok as long as the scope of potential values is limited, but level is particularly recommended not to put in labels. Having an option to specify an array or a Set (any iterator) of keys from the log entry that should be extracted into labels seems really nice!

UPD: Removing level from labels may not be a good idea for now for esthetic purposes: with the label, we can easily filter error messages by the level. Without the label, we need to filter by word "ERROR", and for now it looks ugly, and there is nothing we can do with it

index.js Show resolved Hide resolved
@JWilson45
Copy link

I agree with removing the log level, but leaving it in as a default for backward compatibility. This label should not be forced on users as they should want to keep their label cardinality to a minimum.

@JWilson45
Copy link

@IppX

Limit

The current drawback is that there is no way to add a label dynamically anymore. If that's a must have feature then I can add another option, to specify a list of keys from the log entry that should be extracted into labels (similarly to the official fluentbit output for example).

There is a way to add labels dynamically, each log line can have labels added to them individually. Not sure why you would want this unless it was a very small loki deployment in charge of a small amount of apps.

@IppX
Copy link
Author

IppX commented Oct 6, 2022

There is a way to add labels dynamically, each log line can have labels added to them individually.

This PR breaks that feature that's why I added it under a limit section.

Not sure why you would want this unless it was a very small loki deployment in charge of a small amount of apps.

I agree, that's why I don't think it's a deal breaker to remove this feature. Plus Loki can do this on the query side.

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.

None yet

4 participants