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

Created strongly typed PolicyMatch struct from policy config to match values #3025

Merged
merged 17 commits into from
Nov 9, 2023
Merged

Created strongly typed PolicyMatch struct from policy config to match values #3025

merged 17 commits into from
Nov 9, 2023

Conversation

andriusluk
Copy link
Contributor

@andriusluk andriusluk commented Oct 14, 2023

What this PR does:
Introduced AttributePolicyMatch & IntrinsicPolicyMatch structures to match span attributes based on strongly typed values & precompiled regex which are used to match span attribute values.

Which issue(s) this PR fixes:
Fixes #3015

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2023

CLA assistant check
All committers have signed the CLA.

@andriusluk andriusluk changed the title Separate policy attr match Created strongly typed PolicyMatch struct from policy config to match values Oct 14, 2023
@andriusluk andriusluk marked this pull request as ready for review October 14, 2023 14:41
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

thx for the improvements!

as far as i can tell there should be no change to a user's config correct? the old config will parse and work just fine?

this seems like it would result in some nice perf improvements. dropping the reflection and only doing the regex once. if you had time for benchmarks that would be cool, but no worries if you're busy.

i have one suggestion regarding the interface. i think it would help me see the structure of code better. do you think it makes sense?

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/spanfilter/policymatch/policymatch.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zalegrala zalegrala left a comment

Choose a reason for hiding this comment

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

This looks like a nice change to me, good work. Thanks for the PR.

pkg/spanfilter/spanfilter.go Outdated Show resolved Hide resolved
pkg/spanfilter/splitpolicy.go Outdated Show resolved Hide resolved
pkg/spanfilter/policymatch/attribute.go Show resolved Hide resolved
pkg/spanfilter/policymatch/attribute.go Outdated Show resolved Hide resolved
@andriusluk
Copy link
Contributor Author

@joe-elliott tempodb tests seem to be timing out for some reason. I see the same timeout on the other PRs as well.

=== Failed
=== FAIL: tempodb  (0.00s)
panic: test timed out after 20m0s
...

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

It's looking good. I think the code in attribute.go/intrinsic.go is much more straightforward with your changes.

Looking for some cleanup in splitpolicy.go. Other than that I think this is good to go. Given how we much we rely on this feature I will run it in some on some of our internal clusters after the next round of changes. If everything is looking good then I think this will be ready.

pkg/spanfilter/policymatch/attribute.go Show resolved Hide resolved
pkg/spanfilter/policymatch/attribute.go Show resolved Hide resolved
pkg/spanfilter/policymatch/intrinsic.go Outdated Show resolved Hide resolved
pkg/spanfilter/splitpolicy.go Show resolved Hide resolved
pkg/spanfilter/splitpolicy.go Outdated Show resolved Hide resolved
pkg/spanfilter/splitpolicy.go Outdated Show resolved Hide resolved
pkg/spanfilter/splitpolicy.go Outdated Show resolved Hide resolved
@joe-elliott
Copy link
Member

@joe-elliott tempodb tests seem to be timing out for some reason. I see the same timeout on the other PRs as well.

Yup, this times out occasionally regularly. Don't worry about it. I will re-run in CI that is unrelated to your PR.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

You've gotten this PR into a really nice spot. Code is clear and I love the benchmarks.

I'm going to approve, but not merge just yet to give some maintainers with more experience in this area a chance to review.

@joe-elliott
Copy link
Member

Thanks for the contribution! I think everyone will enjoy the more performant filters.

@joe-elliott joe-elliott merged commit 4c0c132 into grafana:main Nov 9, 2023
14 checks passed
@andriusluk andriusluk deleted the separate-policy-attr-match branch November 9, 2023 17:43
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.

Metrics generator regex policy filter compiles on each attribute value match
5 participants