opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

[processor/tailsampling] add missing sub policies to AND policy

Open tim-oster opened this issue 3 years ago • 11 comments

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.>

tim-oster avatar Jun 23 '22 15:06 tim-oster

CLA Signed

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

pmm-sumo avatar Jun 23 '22 15:06 pmm-sumo

@tim-oster could you add entry to the changelog?

pmm-sumo avatar Jun 28 '22 07:06 pmm-sumo

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?

pmm-sumo avatar Jun 29 '22 08:06 pmm-sumo

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?

jpkrohling avatar Jul 05 '22 20:07 jpkrohling

@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.

tim-oster avatar Jul 06 '22 19:07 tim-oster

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Jul 22 '22 05:07 github-actions[bot]

@tim-oster, I understand this is very close to being ready. Do you still have interest in this one?

jpkrohling avatar Jul 22 '22 14:07 jpkrohling

@jpkrohling yes, absolutely. sorry for the delay.

tim-oster avatar Jul 26 '22 10:07 tim-oster

Ping me when it's ready for a review.

jpkrohling avatar Aug 01 '22 14:08 jpkrohling

@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 avatar Aug 04 '22 08:08 tim-oster

@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 avatar Aug 08 '22 15:08 jpkrohling

@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

tim-oster avatar Aug 16 '22 14:08 tim-oster

You can add this PR's number. GitHub will redirect to the PR if needed.

jpkrohling avatar Aug 22 '22 14:08 jpkrohling

@tim-oster @jpkrohling can we merge this? this will solve #13929

mottibec avatar Sep 11 '22 07:09 mottibec