Skip to content

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

Merged
joe-elliott merged 17 commits into
grafana:mainfrom
andriusluk:separate-policy-attr-match
Nov 9, 2023
Merged

Created strongly typed PolicyMatch struct from policy config to match values#3025
joe-elliott merged 17 commits into
grafana:mainfrom
andriusluk:separate-policy-attr-match

Conversation

@andriusluk

@andriusluk andriusluk commented Oct 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

CLAassistant commented Oct 14, 2023 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.
@andriusluk andriusluk changed the title Separate policy attr match Oct 14, 2023
@andriusluk
andriusluk marked this pull request as ready for review October 14, 2023 14:41
knylander-grafana
knylander-grafana reviewed Oct 17, 2023
View reviewed changes
Comment thread CHANGELOG.md Outdated
joe-elliott
joe-elliott reviewed Oct 17, 2023
View reviewed changes

@joe-elliott joe-elliott left a comment

Copy link
Copy Markdown
Collaborator

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?
Comment thread CHANGELOG.md Outdated
Comment thread pkg/spanfilter/policymatch/policymatch.go Outdated
zalegrala
zalegrala reviewed Oct 17, 2023
View reviewed changes

@zalegrala zalegrala left a comment

Copy link
Copy Markdown
Contributor

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.
joe-elliott
joe-elliott reviewed Oct 24, 2023
View reviewed changes
Comment thread pkg/spanfilter/spanfilter.go Outdated
Comment thread pkg/spanfilter/splitpolicy.go Outdated
Comment thread pkg/spanfilter/policymatch/attribute.go
Comment thread pkg/spanfilter/policymatch/attribute.go Outdated
@andriusluk

andriusluk commented Oct 25, 2023

Copy link
Copy Markdown
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
...
joe-elliott
joe-elliott reviewed Oct 27, 2023
View reviewed changes

@joe-elliott joe-elliott left a comment

Copy link
Copy Markdown
Collaborator

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.
Comment thread pkg/spanfilter/policymatch/attribute.go
Comment thread pkg/spanfilter/policymatch/attribute.go
Comment thread pkg/spanfilter/policymatch/intrinsic.go Outdated
Comment thread pkg/spanfilter/splitpolicy.go
Comment thread pkg/spanfilter/splitpolicy.go Outdated
Comment thread pkg/spanfilter/splitpolicy.go Outdated
Comment thread pkg/spanfilter/splitpolicy.go Outdated
@joe-elliott

joe-elliott commented Oct 27, 2023

Copy link
Copy Markdown
Collaborator

@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.
joe-elliott
joe-elliott approved these changes Nov 6, 2023
View reviewed changes

@joe-elliott joe-elliott left a comment

Copy link
Copy Markdown
Collaborator

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

joe-elliott commented Nov 9, 2023

Copy link
Copy Markdown
Collaborator

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
@andriusluk
andriusluk deleted the separate-policy-attr-match branch November 9, 2023 17:43
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
@andriusluk
Created strongly typed PolicyMatch struct from policy config to match…
dc5f56e 
… values (grafana#3025)

* policy match attr struct

* optimizations

* optimizations

* separate policy matches for intrinsics & attributes

* CHANGELOG

* comment on benchmarks

* linter fixes

* building split policy intrinsic filters by attr.Intrinsic value

* revert pointer

* AttributeFilter do not match by status/kind, just string

* errors for default filter construction cases

* newSplitPolicy tests

* constructor NewStrictIntrinsicFilter

* safe casts

* removed unnecessary intrinsic filter ctor value type checks

* splitpolicy scoped attributes tests

* invalid regexp tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants

@andriusluk @CLAassistant @joe-elliott @zalegrala @knylander-grafana