Skip to content
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

ddtrace/tracer: add sample_rate_limit field to startup log. #1230

Merged
merged 5 commits into from May 10, 2022

Conversation

knusbaum
Copy link
Contributor

@knusbaum knusbaum commented Apr 1, 2022

We want to know when the sample rate limit is in effect and what its value
is. This commit adds that information to the tracer's startup log.

We want to know when the sample rate limit is in effect and what its value
is. This commit adds that information to the tracer's startup log.
@knusbaum knusbaum added this to the 1.38.0 milestone Apr 1, 2022
@knusbaum knusbaum requested a review from a team April 1, 2022 18:24
ddtrace/tracer/log.go Outdated Show resolved Hide resolved
@Julio-Guerra Julio-Guerra modified the milestones: 1.38.0, 1.39.0 Apr 15, 2022
@knusbaum
Copy link
Contributor Author

knusbaum commented May 3, 2022

Added some nil checks and added to the tests to confirm the expected behavior.

@knusbaum knusbaum requested a review from katiehockman May 3, 2022 18:11
ddtrace/tracer/log.go Outdated Show resolved Hide resolved
ddtrace/tracer/log.go Show resolved Hide resolved
ddtrace/tracer/log.go Outdated Show resolved Hide resolved
ddtrace/tracer/log.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I cleaned this up a lot from a simple hack.

LMK what you all think.

// true if rules sampling is enabled. If not present it returns math.NaN() and false.
func (rs *rulesSampler) limit() (float64, bool) {
if rs.enabled() {
return float64(rs.limiter.limiter.Limit()), true
Copy link
Contributor Author

@knusbaum knusbaum May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be OK without nil checks here. Other places throughout the rulesSampler code assume these fields are not nil and all of the constructors populate them.

If we ever changed rulesSampler to allow nil members, we would be unlikely to miss this line, since it lives within the rulesSampler methods.

Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🎉

@@ -280,12 +280,16 @@ func newRateLimiter() *rateLimiter {
}
}

func (rs *rulesSampler) enabled() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

if rs.enabled() {
return float64(rs.limiter.limiter.Limit()), true
}
return math.NaN(), false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but to remove complexity, I would probably return -1, false in this case. Then 284 can just be rs.globalRate >= 0. Unless the rate can actually be negative, which would be surprising.
Don't have a strong preference though.

Copy link
Contributor Author

@knusbaum knusbaum May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 would probably be a better choice, however we already use NaN in various places in the sampler and elsewhere as the canonical "not present" float64 value so I would like to stay consistent.

@knusbaum knusbaum merged commit 46b9da9 into v1 May 10, 2022
@knusbaum knusbaum deleted the knusbaum/startup-report-rate-limit branch May 10, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants