feat: support Remote Config sampling rules #116
Conversation
dmehala
requested review from
pablomartinezbernardo
and removed request for
a team
| if (auto error = maybe_rules.if_error()) { | ||
| trace_sampling_rules_metadata.error = std::move(*error); | ||
| } else { | ||
| rules.merge(*maybe_rules); |
There was a problem hiding this comment.
I'm looking at the docs for this and if I'm understanding correctly, items already in rules will not be overriden by those in maybe_rules. Is that the intended behavior?
dmehala
force-pushed
the
dmehala/support-rc-sampling-rules
branch
from
bdf01a0
to
f26dd83
Compare
BenchmarksBenchmark execution time: 2024-05-23 15:29:15 Comparing candidate commit 3df7c9f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
According to the Remote Configuration specification, `DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS` support floating point input and can be set to `0`.
Now, sampling remote configuration do not create a new trace sampler which has the side effect to reset the rate limiter.
- report remote trace sample rate as RULE instead of REMOTE_RULE for legacy reasons - update REMOTE_RULES and REMOTE_ADAPTIVE_RULE values to match the spec - report default sample rate for telemetry - add _dd.psr for new remote rules
dmehala
force-pushed
the
dmehala/support-rc-sampling-rules
branch
from
05c0459
to
23bb27c
Compare
|
Since your review I added 3 commits: I fixed the rule override issue you mentioned but also remote configuration system tests revealed part of the implementation I overlooked. |
| auto j = r.to_json(); | ||
| j["sample_rate"] = r.sample_rate; | ||
| if (r.max_per_second) { | ||
| j["max_per_second"] = *r.max_per_second; | ||
| } | ||
| res.emplace_back(std::move(j)); |
There was a problem hiding this comment.
Curious to know, as they are being set in to_json, why is it necessary to set them again?
There was a problem hiding this comment.
to_json is not overloaded by SpanSamplerConfig::Rule, meaning SpanMatcher::to_json is called instead.
No description provided.