Skip to content
This repository was archived by the owner on May 23, 2024. It is now read-only.

Potential overflow in probabilistic sampling #229

Closed
isaachier opened this issue Nov 15, 2017 · 3 comments
Closed

Potential overflow in probabilistic sampling #229

isaachier opened this issue Nov 15, 2017 · 3 comments

Comments

@isaachier
Copy link
Contributor

See here: jaegertracing/jaeger-client-cpp#6. This should affect Go as well, not just C++.

@black-adder
Copy link
Contributor

In go, we skirt around the problem by using const maxRandomNumber = ^(uint64(1) << 63), converting to float and back doesn't cause overflow. Seems like we do the same for all the other clients.

@black-adder
Copy link
Contributor

black-adder commented Nov 15, 2017

https://play.golang.org/p/UUBenw5X15

So we do kinda "overflow" in that we go over max(int63), but logically it still works since we do:
s.samplingBoundary >= id.Low

@isaachier
Copy link
Contributor Author

Maybe worth clarifying in a comment there.

// It relies on the fact that new trace IDs are 63bit random numbers themselves, thus making the sampling decision
// without generating a new random number, but simply calculating if traceID < (samplingRate * 2^63).
// TODO remove the error from this function for next major release

Not sure what the TODO is, but changing this in any way could break it. I'm wondering whether or not just to take @rnburn's advice to drop the boundary in favor of a new pseudo-random number here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants