opentelemetry-collector-contrib
opentelemetry-collector-contrib copied to clipboard
[processor/tailsampling] add missing sub policies to AND policy
Description: <Describe what has changed.>
Added parsing support to AND policy for all sub policies mentioned in AndCfg (Latency was missing). Instead of copying the parsing code, I am suggesting a refactored approach that unifies all conversions in one place for better extensibility in the future.
Furthermore, I added error handling for both the AND & COMPOSITE policy parsing. Without it, errors during parsing were ignored and nil PolicyEvaluators were appended to the sub policies which caused the process to panic once it tried to evaluate any of the two composite policies.
Link to tracking Issue: <Issue number if applicable>
Testing: <Describe what testing was performed and which tests were added.> I ran the already existing tests to check if the config still parses correctly and fixed the COMPOSITE policy which contained invalid config.
Documentation: <Describe the documentation added.>
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: tim-oster / name: Tim Oster (c5ee68fbf7eaf0518bf95fce941380cf64ce45ff, 40af8992e2516589a7d25b56a3c8ca9e16c67d67)
Thanks for the contribution @tim-oster! We need the CLA signed, as per community guidelines
@tim-oster could you add entry to the changelog?
This looks good overall. I like how it cleans up things
Furthermore, I added error handling for both the AND & COMPOSITE policy parsing. Without it, errors during parsing were ignored and nil PolicyEvaluators were appended to the sub policies which caused the process to panic once it tried to evaluate any of the two composite policies.
Can you add this case to unit tests?
I'll look at it in detail later, but I wanted to thank you for this refactoring. It was way overdue!
@mottibec, would you be interested in reviewing this one as well?
@pmm-sumo sorry for the delay! I added the two test cases and refactored the composite test so that it tests all of the implemented behavior.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
@tim-oster, I understand this is very close to being ready. Do you still have interest in this one?
@jpkrohling yes, absolutely. sorry for the delay.
Ping me when it's ready for a review.
@jpkrohling the changelog linter complains about FAIL: validate: specify one or more issues #'s. I haven't found any related issues. Should I create one and add it to the changelog or this there a way to skip this step?
@tim-oster, yes, please create one following the contribution guidelines. Basically, you'll just need to add a new file to the unreleased directory.
@jpkrohling sorry for the confusion. I already added the file, I meant if I should also create a new issue to track this change because otherwise I cannot link to it in the file in /unreleased
You can add this PR's number. GitHub will redirect to the PR if needed.
@tim-oster @jpkrohling can we merge this? this will solve #13929