-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Omit _dd.span_sampling.max_per_second
when not configured
#2715
base: master
Are you sure you want to change the base?
Conversation
@@ -22,7 +22,7 @@ class Rule | |||
def initialize( | |||
matcher, | |||
sample_rate: Span::Ext::DEFAULT_SAMPLE_RATE, | |||
rate_limit: Span::Ext::DEFAULT_MAX_PER_SECOND | |||
rate_limit: nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Why demote this from constructor args to constructor body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to know when rate_limit
was not set (thus nil
).
Before, it would fall back to the default unlimited value -1
, but then I can't tell if the user has explicitly set -1
or not set anything at all.
We only want to remove the span_op.set_metric(Span::Ext::TAG_MAX_PER_SECOND, ...
tag if the user did not provide a rate_limit
.
@@ -406,7 +406,7 @@ def agent_receives_span_step3(previous_success) | |||
it do | |||
expect(single_sampled_span.get_metric('_dd.span_sampling.mechanism')).to eq(8) | |||
expect(single_sampled_span.get_metric('_dd.span_sampling.rule_rate')).to eq(1.0) | |||
expect(single_sampled_span.get_metric('_dd.span_sampling.max_per_second')).to eq(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add test that checks that _dd.span_sampling.max_per_second
is set when rate_limit
is provided?
For Single Span Sampling, when no rate limit is provided, we should not include the
_dd.span_sampling.max_per_second
.Before this PR,
_dd.span_sampling.max_per_second
was included with a default value of-1
, meaning unlimited.After this PR,
_dd.span_sampling.max_per_second
will be omitted when not configured. The backend will assume an unlimited rate for such cases.This fixes an inconsistency we had with the Single Span Sampling specification.