-
Notifications
You must be signed in to change notification settings - Fork 417
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
tracer: dual sampling, make sure priorities -1 and 2 are handled #137
Conversation
if priority >= 0 { | ||
s.SetMetric(samplingPriorityKey, float64(priority)) | ||
} | ||
s.SetMetric(samplingPriorityKey, float64(priority)) |
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.
LGTM. Maybe add the -1 case to the method's comment?
b3d4afe
to
57d20a6
Compare
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.
Left a question that I think we should address before the merge.
tracer/span.go
Outdated
// SetSamplingPriority sets the sampling priority. | ||
// Default is 0, any higher value is interpreted as a hint on | ||
// how interesting this span is, and should be kept by the backend. | ||
// SetSamplingPriority sets the sampling priority. Possible values are: |
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.
Can't we provide constants for these values?
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.
@ufoot a couple typos
// PriorityAutoReject informs the backend that a trace should be rejected and not stored. | ||
// This is used by the builtin sampler. | ||
PriorityAutoReject = 0 | ||
// PriorityAutoReject informs the backend that a trace should be kept and not stored. |
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.
@ufoot Looks like the name here (PriorityAutoReject
) doesn't match the description (PriorityAutoKeep
)? Typo?
// PriorityAutoReject informs the backend that a trace should be kept and not stored. | ||
// This is used by the builtin sampler. | ||
PriorityAutoKeep = 1 | ||
// PriorityUserReject informs the backend that a trace should be kept and not stored. |
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.
@ufoot And here
Allow usage of
-1
as a priority sampling value,2
was already handled, but negative values were blocked.