add rule-based sampler #4152
add rule-based sampler #4152
Conversation
this is based on PHP's implementation of a rules-based sampler, which itself is based on extending Java's implementation.
spec-compliance-matrix.md
Outdated
| @@ -79,6 +79,7 @@ formats is required. Implementing more than one format is optional. | |||
| | [Sampling](specification/trace/sdk.md#sampling) | Optional | Go | Java | JS | Python | Ruby | Erlang | PHP | Rust | C++ | .NET | Swift | | |||
| | Allow samplers to modify tracestate | | + | + | | + | + | + | + | + | + | + | + | | |||
| | ShouldSample gets full parent Context | | + | + | + | + | + | + | + | + | + | - | + | | |||
| | Sampler: RulesBasedSampler | Optional | | | | | | | | | | | | | |||
There was a problem hiding this comment.
is there already a spec for rule based samplers? If so, please link to it. Otherwise we can't add something to compliance matrix that isn't spec-ed yet.
There was a problem hiding this comment.
linked. the spec change to add a rule-based sampler is the main focus of this PR, so I assumed it would be both-or-neither.
| * A `RuleBased` sampler MUST be configured with a set of rules and a fallback | ||
| `Sampler` to use if the `Span` does not match any of the configured rules. The | ||
| rules MUST be processed in the order they were provided. | ||
| * A `Rule` requires a list of tests, and a delegate `Sampler` to be invoked if |
There was a problem hiding this comment.
This describes a specific implementation of Rule. As an abstraction a Rule is just a Sampler, is it not?
And the RuleBased sampler itself seems just like a stand-in for logical expression, discussed in #1844
There was a problem hiding this comment.
This describes a specific implementation of Rule. As an abstraction a Rule is just a Sampler, is it not?
No, it's not a Sampler by itself. A rule is a logical expression which is applied to a component of ShouldSample (eg span name, or attributes), and will evaluate to true or false. If all of the rules in a set are true, then the associated Sampler is invoked.
And the RuleBased sampler itself seems just like a stand-in for logical expression, discussed in #1844
It's related to #1844, because that spawned Java's RuleBasedRoutingSampler which in turn influenced this PR. So I think this PR is the implementation that resolves #1844 at spec level.
There was a problem hiding this comment.
a logical expression which is applied to [a span], and will evaluate to true or false.
That's almost exactly the definition of a sampler.
So I think this PR is the implementation that resolves #1844 at spec level.
Actually I meant this OTEP https://github.com/open-telemetry/oteps/pull/250/files.
|
Advanced rule like this seems weird within an application. I'd personally expect this to be in the collector, not in an SDK. |
@dmathieu all of the options within the rules are based on Sampler's ShouldSample arguments provided for the purpose of making sampling decisions, so it seems fair to make them available for that purpose. I agree that the most asked-for case is simply to ignore health checks though. |
|
cc @jmacd |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
This is based on PHP's implementation of a rules-based sampler, which itself is based on and expands on Java's rule-based samplers.
Looking through other SIGs repositories, I didn't find any other similar implementations to try to integrate with this.
Related: open-telemetry/opentelemetry-configuration#102 (comment)
Prototype: open-telemetry/opentelemetry-php-contrib#279
For non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changesspec-compliance-matrix.mdupdated if necessary