fix(rc): improve trace sampling rules parsing#232
Conversation
Correctly parse `tags` from remote trace sampling rules and improves
overall rule parsing reliability through stricter validation and clearer
error reporting.
Changes:
- Add support for `tags` in trace sampling rules.
- Improve validation with explicit type checks and more descriptive
error messages.
- Update catch-all rule handling to replace an existing one instead of
always prepending, ensuring consistent rule ordering.
- Extended system test request handler to support `span_tags` from
incoming requests.
[APMAPI-863]
[APMAPI-866]
BenchmarksBenchmark execution time: 2025-08-15 17:26:14 Comparing candidate commit e6e4b8f in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 0 metrics, 0 unstable metrics. scenario:BM_TraceTinyCCSource
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
=======================================
Coverage ? 86.64%
=======================================
Files ? 83
Lines ? 5427
Branches ? 0
=======================================
Hits ? 4702
Misses ? 725
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Expected<TraceSamplerRule> parse_rule(const nlohmann::json& json_rule) { | ||
| assert(json_rule.is_object()); | ||
|
|
||
| auto make_error = [&json_rule](StringView field_name) { |
There was a problem hiding this comment.
| auto make_error = [&json_rule](StringView field_name) { | |
| auto make_missing_error = [&json_rule](StringView field_name) { |
nit: as we specify for property it might be easier to read to specify here as well
| const auto old_sampler_cfg = | ||
| config_manager.trace_sampler()->config_json(); | ||
| const auto err = config_manager.on_update(config_update); | ||
| const auto new_sampler_cfg = | ||
| config_manager.trace_sampler()->config_json(); |
There was a problem hiding this comment.
Shouldn't we check that the right values have been set as well ? Currently if an error happened, it might tell that old_sampler_cfg != new_sampler_cfg but maybe the values are not the right ones
There was a problem hiding this comment.
Is your question related to the trace sampler values? If so, config_json() returns a JSON with said values, if there's a change, then the JSON from old/new_sampler_cfg will differ.
There was a problem hiding this comment.
I may not understand everything clearly but from my understanding it is not because there was a change that the right values were set
There was a problem hiding this comment.
My understanding from your take is that, it would be preferable to check the rules in the JSON payload are equals to the rules in new_sampler_cfg? For example, we should make sure there's a rule for service foo, resource GET /hello and so on?
There was a problem hiding this comment.
If we can check that there is indeed a rule for the resource get /hello with the right value/tags, it ensures the correctness of the apply. That may be what you are doing in #234 though
There was a problem hiding this comment.
Got it. Currently we are not validating the correctness because the TraceSampler interface doesn't provide a way to retrieve the rules, and I don't think there's any meaning except for testing purposes. I am planning to revamp the configuration part, one of the requirement is to be able to validate the config sent to telemetry. This would help to validate the correctness.
There was a problem hiding this comment.
Got it, perfect. Ping me if you need an additional hand.
Correctly parse
tagsfrom remote trace sampling rules and improves overall rule parsing reliability through stricter validation and clearer error reporting.Changes:
tagsin trace sampling rules.span_tagsfrom incoming requests.APMAPI-863
APMAPI-866