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

chore(level_detection): Make log level detection configurable #14784

Merged
merged 11 commits into from
Nov 6, 2024

Conversation

shantanualsi
Copy link
Contributor

@shantanualsi shantanualsi commented Nov 6, 2024

What this PR does / why we need it:
Currently log level detection works only for a fixed set of default labels. The list in reality is unbounded, customers use any label or field such as log_level, logging_level, etc. There have been quite a few escalations and questions to Loki team concerning that it doesn't work properly when they something else apart from the default labels.

This PR adds an optional config to Loki that allows a tenant to specify custom labels for level detection.

The long term solution to this problem could be to leverage adaptive logs and patterns to make it robust. This is a temporary approach till we finalize on a better one.

Which issue(s) this PR fixes:
Fixes #1194

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Nov 6, 2024
# If discover_log_levels is enabled, this list of fields will be used to determine the level (case-sensetive). Loki checks for these labels first within labels or structured metadata.
# If not, level is determined from OTLP severity number in case of OTLP logs, and finally attempted to extract from the log line itself.
# CLI flag: -validation.log-level-fields
[log_level_fields: <list of strings> | default = [level LEVEL Level Severity severity SEVERITY lvl LVL Lvl]]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open to suggestions if there's a better name

@shantanualsi shantanualsi marked this pull request as ready for review November 6, 2024 09:00
@shantanualsi shantanualsi requested a review from a team as a code owner November 6, 2024 09:00
Comment on lines 105 to 109
if isJSON(log) {
v = l.getValueUsingJSONParser(logSlice)
} else {
v = l.getValueUsingLogfmtParser(logSlice)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the log line is neither JSON nor logfmt formatted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use the same pattern that you use for detected json format rather than encapsulating the check in the lgetValueUsingLogfmtParser(logSlice) function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. In the current case, it defaults to scan the log line as the value returned is nil. But this is implicit as you mentioned already.

I've made the changes to make it clearer.

}

switch {
case bytes.EqualFold(v, []byte("trace")), bytes.EqualFold(v, []byte("trc")):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make the byte slices a package variable to allocate them only once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed 👍

Comment on lines +29 to +33
allowedFieldsMap := make(map[string]struct{}, len(allowedFields))
for _, field := range allowedFields {
allowedFieldsMap[field] = struct{}{}
}
return allowedFieldsMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowedLabelsForLevel() can be called quite a few times. I think we should allocate the map only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines +183 to +186
equalIndex := bytes.Index(line, []byte("="))
if len(line) == 0 || equalIndex == -1 {
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code can now be removed from the getValueUsingLogfmtParser() function.

@shantanualsi shantanualsi merged commit 867ce3d into main Nov 6, 2024
60 checks passed
@shantanualsi shantanualsi deleted the configurable-log-level-label branch November 6, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants