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

feat: detect json and logfmt log types #44

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

adamhackl
Copy link
Contributor

I added sections to detect type and extract level for both json and logfmt to complement the existing klog and zerolog detection. I used the logic that was used in grafana-agent-modules, which seems to work well. The only deviation I made was to add a start of line ^ to the regex for logfmt, because without it I got a lot of false alerts (basically anything with an = in it).

These don't check for the format pod annotation like the grafana-agent-modules did. That would take some significant rework and personally I like that it auto-detects the type and level.

@bentonam
Copy link
Collaborator

@adamhackl thanks for the PR, appreciate the contribution!

I would like to avoid having a module that can continuously grow in size, with more and more conditions. Essentially, stage.match is an "if" condition. Each condition has a cost, and that is CPU time, and we should be cognizant of that.

The bottom of that module has a very large "catch all" regex that will catch the vast majority of patterns if they aren't already defined. There are only two checks above that, one for klog which is a lot different, and another for zero log which was also different.

There is value in detecting that actual format though, to allow for other downstream conditions, reporting, etc. Ideally, this would be the users choice to implement though.

What I would purpose or like to see happen is:

  1. Within the log-levels.alloy module, remove the klog condition from default_levels and introduce a new component in that same file klog_format, and the same for zerolog a new component called zerolog_format
  2. Add a new component json_format
  3. Add a new component called logfmt_format

Having the component exist in the module does not mean it is used, it still most be invoked/instantiated. This way if a user wants to explicitly detect json formatted logs, they can use that component. The user has the choice of implementing what they want instead of this module applying what would be an ever growing list of conditions within a single module.

@erikbaranowski @mattdurham any additional thoughts to add to this?

@adamhackl
Copy link
Contributor Author

@bentonam thanks for the feedback! I like your idea.
I first dove into this module because its so drastically different from how the grafana-agent-modules worked. As I was migrating, I noticed that virtually all log files were labeled with log_type as unknown ... even ones I had explicitly set or that had previously been identified correctly. After figuring out what this module is actually doing, I thought it would be good to also have json and logfmt modules since those are the two that Loki natively parses and are pretty common. The old modules tried to identify a whole lot of log types, but we did see some crazy CPU usage from those modules also, which could be correlated.
A couple things I need to work through:

  • We'll still need the default module, but all its going to do is set everything type and level to unknown. This module is going to have to be run prior to any of the other modules...? Because all the modules require the level = unknown.
  • Is there any reason to do anything with annotations like grafana-agent-modules did?
  • It would be nice if we could respect if the log_type label is set already, but thats probably more complexity than it is worth.

The log_type definitely doesn't have a whole lot of use cases aside from being informational. I might actually just drop it instead of exposing it to the users if a lot of the lines are going to be labeled unknown.

@adamhackl
Copy link
Contributor Author

@bentonam I incorporated your suggestions and was able to test ... all seems to work as expected. Hopefully captured your suggestion correctly. This would be a breaking change for anyone already using this component.

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.

2 participants