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

filterprocessor: fix filtering non-string body by bodies property #22738

Merged

Conversation

sumo-drosiek
Copy link
Member

Description:

Convert body to string before filtering

Link to tracking Issue: #22736

Testing: Unit tests

Documentation: N/A

Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Thank you for bringing this to our attention.

With #18642 in the works I really really don't want to make changes to filterspan, filtermetric, or filterlog anymore. The ability to index body via OTTL has been introduced via #22102 and will be available in the next release of the collector to solve the problem this PR solves.

filter:
  logs:
    log_record:
    - 'IsMatch(body["some key"], "some value")'

I am willing to hear arguments as to why we should also patch this in filterlog, but blocking the PR for now.

@sumo-drosiek
Copy link
Member Author

I am willing to hear arguments as to why we should also patch this in filterlog, but blocking the PR for now.

I wouldn't say this is the same. First of all, the current behavior of bodies is just wrong. We are rejecting all logs which aren't string. Also, the usecase is to have one common configuration for logs, where body may be both maps and strings (not sure if OTTL supports that).

With #18642 in the works I really really don't want to make changes to filterspan, filtermetric, or filterlog anymore. The ability to index body via OTTL has been introduced via #22102 and will be available in the next release of the collector to solve the problem this PR solves.

Totally understand, but raised it because IMHO it's bug in bodies behavior and should be either fixed or removed at all

@TylerHelmuth
Copy link
Member

Also, the usecase is to have one common configuration for logs, where body may be both maps and strings (not sure if OTTL supports that

OTTL currently supports fetching the log as whatever type it is and with version 0.79.0 you'll be able to index it if it is a map or a slice. At the moment filterprocessor can take advantage of the OTTL filter interface, and soon all the other components using internal/filter will as well via #18642.

IMHO it's bug in bodies behavior

Agreed, but I feel the fix to the bug should be to get people off of this configuration and on to OTTL.

That said, only filterprocessor has the option to use the OTTL configuration, so it isn't a viable fix for attributesprocessor (the only other component using filterlog), so that is a reason to move forward with this PR.

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@TylerHelmuth TylerHelmuth merged commit d37df78 into open-telemetry:main May 26, 2023
@github-actions github-actions bot added this to the next release milestone May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants