-
Notifications
You must be signed in to change notification settings - Fork 55
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
base: development
Are you sure you want to change the base?
Conversation
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 ;) |
According to current official recommendations, dynamic labels are ok as long as the scope of potential values is limited, but UPD: Removing |
a42ded1
to
32d43f2
Compare
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. |
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. |
This PR breaks that feature that's why I added it under a limit section.
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. |
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
level
,label
andlabels
label and change the default format to a json representation of theinfo
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.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 :)