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

[attributesprocessor] Support filter by severity #9132

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Apr 10, 2022

Description:
Adds a severity text matcher for the attributesprocessor.

Link to tracking Issue:
No issue.

Testing:
Unit tests

Documentation:
README and configuration

@atoulme atoulme requested a review from a team April 10, 2022 06:04
@atoulme atoulme requested a review from pmm-sumo as a code owner April 10, 2022 06:04
@atoulme atoulme changed the title Support filter by severity [attributesprocessor] Support filter by severity Apr 10, 2022
@atoulme atoulme force-pushed the filter_by_severity branch from 09276d0 to b18780d Compare April 10, 2022 06:05
@pmm-sumo
Copy link
Contributor

I think the change looks good overall. I think before merging it would be good taking the item raised by @djaglowski, i.e. if we want to have separate filter for log severity number and if that's the case then how the configuration keys should be named for them.

As a digression, I think that while having ability to filter records when attributes processor is concerned is helpful, it would be even more useful to have similiar capabilities exposed in filterprocessor as well

@atoulme
Copy link
Contributor Author

atoulme commented Apr 11, 2022

@pmm-sumo would you be awesome and file an issue for the digression? That sounds like a good move.

Agree on your comments folks, will make the filter's name more clear.

The problem with severity number fwiw is that you'd be well tempted to use a >, <, >= or <= to filter by, and that seemed like something just a bit more complex.

@djaglowski
Copy link
Member

As a digression, I think that while having ability to filter records when attributes processor is concerned is helpful, it would be even more useful to have similiar capabilities exposed in filterprocessor as well

+1, This processor is is filtering on several non-attributes at this point.

@atoulme atoulme force-pushed the filter_by_severity branch from b7ab705 to 742853c Compare April 11, 2022 22:50
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

The change looks good to me. @djaglowski @pmm-sumo please review and approve if your comments have been addressed.

@pmm-sumo
Copy link
Contributor

@pmm-sumo would you be awesome and file an issue for the digression? That sounds like a good move.

Sure thing: #9235

The problem with severity number fwiw is that you'd be well tempted to use a >, <, >= or <= to filter by, and that seemed like something just a bit more complex.

I think that filtering by severity text is already pretty useful, don't want to block this PR due to lack of filtering by number but I wanted to hear from @djaglowski on his take on that :)

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.

Sorry for the delay. LGTM

@codeboten codeboten merged commit ad0e612 into open-telemetry:main Apr 21, 2022
@TylerHelmuth
Copy link
Member

Tangentially related, when should new capabilities be added to the attribute processor vs the transform processor?

@pmm-sumo
Copy link
Contributor

Tangentially related, when should new capabilities be added to the attribute processor vs the transform processor?

That's a good question. In my opinion it's OK to add small features here that users want and then port those to transform processor after it's considered ready to be used

There is a PR that adds some log processing capabilities. This makes it already quite useful IMO

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.

6 participants