attribute-based sampler #1208
attribute-based sampler #1208
Conversation
this adds an attribute-based sampler, which can be configured to inspect a semconv attribute on a span, and either sample it in our out by applying a PCRE regex to it. adding an alternative syntax to define samplers and sampler args from env vars. now we also support "sampler1,...,samplerN" chaining of samplers, and the ability to define sampler args via "<samplername>.<arg>=<value>,..."
|
The primary use-case for this would be to sample out URLs like |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1208 +/- ##
============================================
+ Coverage 83.20% 83.38% +0.18%
- Complexity 2262 2293 +31
============================================
Files 285 286 +1
Lines 6435 6507 +72
============================================
+ Hits 5354 5426 +72
Misses 1081 1081
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
I think we should at least allow matching on SpanKind too, similar to Javas RuleBasedRoutingSampler.
If we want to allow for a wider range of configuration options: I did some prototyping that unifies Javas RuleBasedRoutingSampler and LinksBasedSampler in a single RuleBasedSampler. (We could still keep the "attribute" env configuration and create a RuleBasedSampler with an AttributeRule (+ optional attribute.spankind SpanKindRule)).
| switch ($this->mode) { | ||
| case self::ALLOW: | ||
| if (!$attributes->has($this->attribute)) { | ||
| return new SamplingResult(SamplingResult::DROP); |
There was a problem hiding this comment.
| return new SamplingResult(SamplingResult::DROP); | |
| break; |
There was a problem hiding this comment.
I did some prototyping that unifies Javas RuleBasedRoutingSampler and LinksBasedSampler
I was just reading about the idea of a rule-based sampler in this OTEP and I like the idea of using that approach over what I've done here, given the increased usefulness (and hopefully also compatibility with the proposed sampling configuration).
The prototype looks great, I think we should go with that approach. For the time being, it might not be configurable via env vars (and so not available to auto-instrumentation/sdk-autoloading) due to the complexity of the configuration.
| @@ -19,6 +19,7 @@ | |||
| "require": { | |||
| "php": "^7.4 || ^8.0", | |||
| "ext-json": "*", | |||
| "ext-pcre": "*", | |||
There was a problem hiding this comment.
IMO we shouldn't list core extensions as requirement
|
Setting back to draft, because it probably won't be merged as-is... |
this adds an attribute-based sampler, which can be configured to inspect a semconv attribute on a span, and either sample it in or out by applying a PCRE regex to it.
adding an alternative syntax to define samplers and sampler args from env vars. now we also support "sampler1,...,samplerN" chaining of samplers, and the ability to define sampler args via ".=,..."