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

[processor/filter] Add functionality to allow filtering on SeverityNumber #13002

Merged

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented Aug 8, 2022

Description:

  • Add the ability to specify a minimum severity for filtering logs
    • Also added a flag that allows "undefined" severity logs to optionally be matched when min_severity is defined.

Link to tracking Issue: #12959

Testing:

  • Added unit tests to test new functionality
  • Manually tested with some mocked logs to check filtering

Documentation:

  • Added documentation for new min_severity and match_undefined_severity fields
  • Added example using new fields.

@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review August 8, 2022 16:19
@BinaryFissionGames BinaryFissionGames requested a review from a team as a code owner August 8, 2022 16:19
processor/filterprocessor/config.go Show resolved Hide resolved
e.g. if this is "WARN", all log records with "WARN" severity and above (WARN[2-4], ERROR[2-4], FATAL[2-4]) are matched.
The list of valid severities that may be used for this option can be found [here](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#displaying-severity)
By default, logs with undefined severity are not matched.
- `match_undefined_severity`: MatchUndefinedSeverity defines whether to match logs with undefined severity or not when using the `min_severity` matching option. If `min_severity` is not specified, this option does nothing. If `match_undefined_severity` is set to true, log records with no severity will be matched. If set to false, log records with no severity will not be matched.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need this option. The data model has an opinion on where "undefined" severity ranks vs other severities levels, so I think we should just support it according to that ordering.

Copy link
Contributor Author

@BinaryFissionGames BinaryFissionGames Aug 8, 2022

Choose a reason for hiding this comment

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

I was really on the fence about this one, but IMO it's really useful from a user perspective, because "undefined" severity can come up in exceptional circumstances, e.g. I set up a filelog recevier, with a severity parsing operator. But maybe I mess up and typo one of the severities or something.

Now I'm dropping logs, because they are now undefined severity and I'm filtering everything below "info" level. But maybe those logs are actually really important, just the severity can't be parsed.

I think it's really nice to have this option when you need to be more fault-tolerant.

Copy link
Member

Choose a reason for hiding this comment

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

I can appreciate the use case, but I don't think we should have a separate setting. What do you think about reworking the config a bit?

log_severity:
  min: INFO
  match_undefined: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworked the config as suggested; I used severity_number instead of log_severity to avoid stuttering and potential confusion with severity_texts.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM

@djaglowski
Copy link
Member

@boostchicken, do you want to take a look as code owner?

@djaglowski djaglowski force-pushed the filterprocessor-sev-num-filtering branch from 71b787f to fd5a185 Compare August 10, 2022 12:19
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

3 participants