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/attributes] Add support to filter on log body #8996

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Apr 1, 2022

Description:
Add support to filter logs based on their body.

Link to tracking Issue:
None

Testing:
Unit tests

Documentation:
Added to README

@atoulme atoulme requested a review from a team April 1, 2022 01:28
@atoulme atoulme requested a review from pmm-sumo as a code owner April 1, 2022 01:28
@atoulme atoulme force-pushed the support_attributes_processor_with_log_body branch from 1e87874 to 7a8c10c Compare April 1, 2022 01:29
Copy link
Contributor

@pmm-sumo pmm-sumo left a comment

Choose a reason for hiding this comment

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

I think example in README.md how to use the log_bodies would be good to have. Also one in testdata

I believe we should a add a positive test case, perhaps basing on TestLogRecord_Matching_True

This change should be mentioned in changelog

@open-telemetry/log-collection-approvers could you take a look as well?

@sumo-drosiek
Copy link
Member

Could it support regexes?

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Apr 1, 2022

Could it support regexes?

I believe so, since it's using filterset underneath, which can be also configured with regular expression to match

@sumo-drosiek
Copy link
Member

also, kind of related PR: #5680

@atoulme
Copy link
Contributor Author

atoulme commented Apr 1, 2022

@sumo-drosiek your PR looks much better than mine, why didn't it make it? My PR is just about one thing, getting log bodies to be used for the processor. Right now, the processor is useless for logs since it only allows to filter on the name attribute (I actually think looking at the logs the name filters is not working) and that attribute is deprecated.

So if that small scope of change is ok and gets us there, please don't mind me and I'll keep plugging away.

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Apr 1, 2022

@sumo-drosiek your PR looks much better than mine, why didn't it make it? My PR is just about one thing, getting log bodies to be used for the processor. Right now, the processor is useless for logs since it only allows to filter on the name attribute (I actually think looking at the logs the name filters is not working) and that attribute is deprecated.

So if that small scope of change is ok and gets us there, please don't mind me and I'll keep plugging away.

I believe there was some research on using common expression language for OpenTelemetry Collector components in progress back then, which I believe ended up with conclusion that we want to follow with what is now transformprocessor.

The latter is still a work in progress, so I think we should be OK with making some small changes/additions to filter/attributes/resource processors as needed (like adding another fields which can be filtered), though I believe that adding another expression language here is too much and we should rather wait until transformprocessor is ready and supports more signals. Please correct me if I'm wrong here @bogdandrutu @anuraaga :)

@anuraaga
Copy link
Contributor

anuraaga commented Apr 1, 2022

That sounds about right. This is SlowTel so I'd recommend not expecting quick progress regardless of the component ;)

@atoulme
Copy link
Contributor Author

atoulme commented Apr 1, 2022

@pmm-sumo feedback addressed, but no example in README, sorry. I didn't see a good spot for it.

@atoulme atoulme force-pushed the support_attributes_processor_with_log_body branch from ce9fc5d to 088dd03 Compare April 1, 2022 17:50
@@ -218,6 +218,10 @@ attributes:
# This is an optional field.
log_names: [<item1>, ..., <itemN>]

# The log body must match at least one of the items.
# This is an optional field.
log_bodies: [<item1>, ..., <itemN>]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth mentioning that only String bodies are matched against here

CHANGELOG.md Show resolved Hide resolved
Co-authored-by: Dmitrii Anoshin <[email protected]>
@sumo-drosiek
Copy link
Member

sumo-drosiek commented Apr 4, 2022

@atoulme I just linked it because it is related, but as @pmm-sumo mentioned the conclusion was to skip adding expr language :)

@jpkrohling jpkrohling changed the title Add support to filter on log body [processor/attributes] Add support to filter on log body Apr 6, 2022
@jpkrohling jpkrohling merged commit 80e6ea2 into open-telemetry:main Apr 6, 2022
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.

7 participants