Add rule Scope configuration option to rules-based sampler #440
Conversation
This can be used to tell the rules sampler to apply rules to a trace as a whole (the current behavior: a rule matches if each of its conditions is satisfied by a span in the trace – it doesn't have to be the same span) or to individual spans (meaning that a rule is matched only if some span in the trace satisfies each of its conditions)
This preps for adding alternate ways to evaluate a rule (in the next commit)
config/sampler_config.go
Outdated
| @@ -62,6 +62,7 @@ type RulesBasedSamplerRule struct { | |||
| SampleRate int | |||
| Sampler *RulesBasedDownstreamSampler | |||
| Drop bool | |||
| MatchSpan bool | |||
There was a problem hiding this comment.
Probably need to add this to the documentation + example rule configurations.
There was a problem hiding this comment.
How would you feel about using a string to determine type? I think Scope works well, and would have either "trace" or "span", with "trace" being default.
isnotajoke
changed the title
MikeGoldsmith
added
type: enhancement
There was a problem hiding this comment.
Hey @isnotajoke - thank you for putting this together. Sorry for the delay in getting it reviewed.
Overall, I really like what you've done and think it's a great solution. I've left a suggestion for changing the configuration option - let me know you thoughts.
config/sampler_config.go
Outdated
| @@ -62,6 +62,7 @@ type RulesBasedSamplerRule struct { | |||
| SampleRate int | |||
| Sampler *RulesBasedDownstreamSampler | |||
| Drop bool | |||
| MatchSpan bool | |||
There was a problem hiding this comment.
How would you feel about using a string to determine type? I think Scope works well, and would have either "trace" or "span", with "trace" being default.
JamieDanielson
added
the
status: revision needed
|
@MikeGoldsmith makes sense to me. I'll go ahead and make that change. |
|
We're planning to test this PR in our infra this week. Will update with any concerns/follow up tweaks after that, and then it should be good for final review. |
There was a problem hiding this comment.
Looks great, thanks @isnotajoke.
Yes, please do test and report back any findings. We'll hold back on merging this for your feedback.
MikeGoldsmith
added
status: info needed
JamieDanielson
dismissed
MikeGoldsmith’s stale review
accidental approval, holding off for further testing!
MikeGoldsmith
changed the title
|
FYI the test failure is coming from config_test - you just need to update the number of rules in the dataset to be 5. |
Hey @isnotajoke - just wanted to check in to see how your testing went? |
|
@MikeGoldsmith still ongoing. Should be complete this week. |
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
|
Hey @isnotajoke - just wanted to check in to see if you've completed your internal testing? We're eager to move forward with this feature and value your feedback. |
|
Hey @isnotajoke - I'm going to look to move ahead with this change this week. Let me know if you have any additional feedback. |
|
@MikeGoldsmith sorry for the radio silence here. We completed our testing and didn't see any issues. No concerns with moving ahead. |
There was a problem hiding this comment.
Thanks @isnotajoke - we really appreciate your contribution and feedback.
JamieDanielson
added
status: blocked
and removed
status: oncall
robbkidd
removed
status: info needed
…io#440) We'd like to be able to apply all of the conditions in a rules-based sampler rule to a single span. In other words, we'd like a rule to be considered matched only if all of the conditions in that rule are satisfied by a single span in the trace. This allows us to more accurately make per-service sampling decisions in a dataset written to by multiple services. Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Which problem is this PR solving?
We'd like to be able to apply all of the conditions in a rules-based sampler rule to a single span. In other words, we'd like a rule to be considered matched only if all of the conditions in that rule are satisfied by a single span in the trace. This allows us to more accurately make per-service sampling decisions in a dataset written to by multiple services.
Short description of the changes
At a high level, we add a new
Scopeconfiguration option to the rule definition. By default (and if unspecified) it is "trace". If "span", all conditions in the rule must be satisfied together by some span in a trace. This is a subtle but important difference with the default behavior, effectively making a rule more strict than it would otherwise be.Scopeintroduces some complexity to the rule based sampler. While many of the operations performed within theGetSampleRatemethod (extracting a value, checking for a match) are similar between the two approaches, the shape of the logic to compute a match is totally different whenScopeis 'trace' than when it is 'span'. To avoid duplication, we refactor the common methods into private functions; this also makes the logic to loop through & compute a match a little easier to understand at a glance.I added a basic spec to cover the new behavior.