Update samplers to include root and trace id ratio based options #3315
Conversation
We have the allowlist functionality now that is faster.
default, adaptive, always_on, always_off are now available for use when a trace is root, remote_parent_sampled or remote_parent_not_sampled. default is first in the case list because it is the most common. I predict adaptive will be far less common than always_on or always_off, so I put it last for a slight performance benefit.
Remove the to_sym calls and string interpolation and pass in the ratio config as a separate argument in the case that the remote parent config is set to trace_id_ratio_based. Also, mark the APIs in SamplingDecision to be private
Now, the config validation will occur on start up when configs are being evaluated from different sources rather than validating on every sampling decision. Also, make sure allowed_from_server is false for all sampler configs.
| constants | ||
| end | ||
|
|
||
| def self.enforce_fallback(allowed_values: nil, fallback: nil) |
There was a problem hiding this comment.
This wasn't being used. We have the allowlist functionality now that is more performant
| }, | ||
| { | ||
| "test_name": "skip_no_headers_root_uses_ratio_sampler", | ||
| "test_name": "no_headers_root_always_on", |
There was a problem hiding this comment.
This isn't currently part of the cross-agent tests. If we think it's valuable, I'll open a PR.
| strategies = %w[root remote_parent_sampled remote_parent_not_sampled] | ||
| key = strategies.find { |f| test_case[f] == 'trace_id_ratio_based' } | ||
| config["distributed_tracing.sampler.#{key}.trace_id_ratio_based.ratio".to_sym] = test_case['ratio'] |
There was a problem hiding this comment.
This expects only one ratio to ever be configured in a test case, which is how the fixture is currently set up. I can adjust it to be more robust if you'd like to account for the possibility of multiple configs having ratios.
| :default => 'default', | ||
| :public => true, | ||
| :type => String, | ||
| :allowed_from_server => false, |
There was a problem hiding this comment.
I don't think we want to allow these from the server at this time, but happy to change.
| trace_id = transaction.trace_id | ||
| config_value = NewRelic::Agent.config[:'distributed_tracing.sampler.root'] | ||
|
|
||
| case config_value |
There was a problem hiding this comment.
We could consider refactoring this to memoize the config value and skip the case statement evaluation every time. The downside is that we would have a harder time adapting if we get a config value change from server side config while the agent is running. I don't know if that's intended as a future feature.
This registers them in our orphan configuration test Also, clean up unnecessary comments
pick def772589 Revert spacing changes to attribute_configuration.json
This PR:
adaptiveoption to all DT samplersdefault,adaptive,trace_id_ratio_based,always_on,always_offCloses #3284
Questions
Todos