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/tailsampling] add missing sub policies to AND policy #11505

Merged
merged 12 commits into from
Sep 12, 2022

Conversation

tim-oster
Copy link
Contributor

Description:
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:

Testing:
I ran the already existing tests to check if the config still parses correctly and fixed the COMPOSITE policy which contained invalid config.

Documentation:

@tim-oster tim-oster requested review from a team and jpkrohling as code owners June 23, 2022 15:24
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 23, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@pmm-sumo
Copy link
Contributor

Thanks for the contribution @tim-oster! We need the CLA signed, as per community guidelines

@pmm-sumo
Copy link
Contributor

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

@pmm-sumo
Copy link
Contributor

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?

@jpkrohling
Copy link
Member

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?

@tim-oster
Copy link
Contributor Author

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

CHANGELOG.md Outdated Show resolved Hide resolved
processor/tailsamplingprocessor/and_helper.go Show resolved Hide resolved
processor/tailsamplingprocessor/and_helper_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mottibec mottibec 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 showing the tailsampling some love!

processor/tailsamplingprocessor/config.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Jul 22, 2022
@jpkrohling
Copy link
Member

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

@github-actions github-actions bot removed the Stale label Jul 23, 2022
@tim-oster
Copy link
Contributor Author

@jpkrohling yes, absolutely. sorry for the delay.

@jpkrohling
Copy link
Member

Ping me when it's ready for a review.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM

@tim-oster
Copy link
Contributor Author

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

@jpkrohling
Copy link
Member

@tim-oster, yes, please create one following the contribution guidelines. Basically, you'll just need to add a new file to the unreleased directory.

@tim-oster
Copy link
Contributor Author

@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

@jpkrohling
Copy link
Member

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

@mottibec
Copy link
Contributor

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

@jpkrohling jpkrohling merged commit a61c7f2 into open-telemetry:main Sep 12, 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.

None yet

5 participants