-
Notifications
You must be signed in to change notification settings - Fork 40
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
sdk-trace: add IdGenerator
#370
Conversation
|
||
object IdGenerator { | ||
|
||
private val InvalidId = 0 |
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.
private val InvalidId = 0 | |
private final val InvalidId = 0 |
sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/IdGenerator.scala
Show resolved
Hide resolved
def generateTraceId: F[ByteVector] = | ||
for { | ||
hi <- Random[F].nextLong | ||
lo <- Random[F].nextLong.iterateUntil(_ != InvalidId) | ||
} yield SpanContext.TraceId.fromLongs(hi, lo) |
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.
What are the rules for trace ids? is it not a valid trace id if hi != 0
but lo == 0
?
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.
Yes. At least it's correct for Java SDK: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/RandomIdGenerator.java#L32-L38
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.
A valid trace identifier is a 16-byte array with at least one non-zero byte.
Isn't that ID generator missing lots and lots of possible valid IDs? Is that intentional 🤔
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.
Aha, there is some explanation here in open-telemetry/opentelemetry-java#3291 (comment). I'm not sure if that's still relevant, or relevant to us.
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 guess we can stick with Java SDK implementation
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.
😱
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.
On the other hand, you rarely need to allocate 1 million random uuids
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.
nextLong
:
for {
r <- f
out <- Sync[F].delay(r.nextLong())
} yield out
nextLongBounded
:
def nextLongBounded(n: Long): F[Long] = {
/*
* Divide n by two until small enough for nextInt. On each
* iteration (at most 31 of them but usually much less),
* randomly choose both whether to include high bit in result
* (offset) and whether to continue with the lower vs upper
* half (which makes a difference only if odd).
*/
for {
_ <- require(n > 0, s"n must be positive, but was $n")
offset <- Ref[F].of(0L)
_n <- Ref[F].of(n)
_ <- Monad[F].whileM_(_n.get.map(_ >= Integer.MAX_VALUE))(
for {
bits <- nextIntBounded(2)
halfn <- _n.get.map(_ >>> 1)
nextN <- if ((bits & 2) == 0) halfn.pure[F] else _n.get.map(_ - halfn)
_ <-
if ((bits & 1) == 0) _n.get.flatMap(n => offset.update(_ + (n - nextN)))
else Applicative[F].unit
_ <- _n.set(nextN)
} yield ()
)
finalOffset <- offset.get
int <- _n.get.flatMap(l => nextIntBounded(l.toInt))
} yield finalOffset + int
}
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.
Gosh, is that really the implementation? I assumed it was delegating to the underlying Random
instance, not that there are atomics and monadic loops in there 🤦
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 believe I'm looking into the right one :D
ec03b44
to
7214565
Compare
7214565
to
c3c9cee
Compare
* @tparam F | ||
* the higher-kinded type of a polymorphic effect | ||
*/ | ||
trait IdGenerator[F[_]] { |
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 hope we don't get screwed over by compatibility 😇 probably not, but the F[_]
makes it impossible for us to add default implementations of new methods where other languages could.
RandomIdGenerator.java