Skip to content

Introduce TraceId.Gen, SpanId.Gen#732

Merged
catostrophe merged 2 commits intomasterfrom
feat/trace-id-gen
Apr 14, 2022
Merged

Introduce TraceId.Gen, SpanId.Gen#732
catostrophe merged 2 commits intomasterfrom
feat/trace-id-gen

Conversation

@catostrophe
Copy link
Copy Markdown
Member

@catostrophe catostrophe commented Apr 14, 2022

General info

During @mrdziuban's work on trace4cats/trace4cats-xray#4 we've realized that we don't have a mechanism for overriding the way of generation of TraceId/SpanId. This PR introduces typeclasses for providing generators for them.

We now require F[_] : TraceId.Gen : SpanId.Gen bounds in the whole model from SpanContext to Span to EntryPoint. That is, we now can provide an X-Ray compatible traceId generator to EntryPoint.apply[F] if needed.

I also replaced wide bounds (e.g. Sync[F]) with fine-grained multiple bounds.

This change is (almost) fully source compatible, i.e. all the newly introduced context bounds are automatically derivable via previously existed bounds (e.g. Sync[F]). If a user didn't provide them explicitly, they shouldn't have any errors with recompilation. And, of course, the change is not binary compatible.

The default instances (as previously) are based on ThreadLocalRandom which is a fast, threadsafe, but cryptographically insecure RNG. Tracing seems to be a proper field of usage for such a generator.

How I see the usage of alternative generators on example of AWS X-Ray

  1. We put alternative generators in a separate package named trace4cats.xray.compat in forms of implicit and explicit instances.

    Code sample
    package io.janstenpickle.trace4cats.xray
    
    import cats.effect.kernel.Sync
    import io.janstenpickle.trace4cats.model.TraceId
    
    package object compat {
      implicit def xrayTraceIdGenInstance[F[_]: Sync]: TraceId.Gen[F] = explicits.xrayTraceIdGen
    
      object explicits {
        def xrayTraceIdGen[F[_]: Sync]: TraceId.Gen[F] = TraceId.Gen.from(???)
      }
    }
  2. When a user constructs an EntryPoint, they should provide the custom instance either via a global import clause or via a local instance.

    Code sample
    import io.janstenpickle.trace4cats.xray.compat._
    
    def entryPoint[F[_]: Async](process: TraceProcess): Resource[F, EntryPoint[F]] = {
      Resource.eval(LogSpanCompleter.create[F](process)).map { completer =>
        EntryPoint[F](SpanSampler.always[F], completer)
      }
    }

    or

    def entryPoint[F[_]: Async](process: TraceProcess): Resource[F, EntryPoint[F]] = {
      Resource.eval(LogSpanCompleter.create[F](process)).map { completer =>
        implicit val xrayTraceIdGen = io.janstenpickle.trace4cats.xray.compat.explicits.xrayTraceIdGen[F]
        EntryPoint[F](SpanSampler.always[F], completer)
      }
    }

There's also an option to provide a custom object with a factory apply method that uses custom generators, but as for me it doesn't seem more discoverable or clearer.

@catostrophe catostrophe added the enhancement New feature or request label Apr 14, 2022
@catostrophe
Copy link
Copy Markdown
Member Author

cc @mrdziuban @ybasket

def gen: F[SpanId] = _gen
}

implicit def threadLocalRandomSpanId[F[_]: Sync]: Gen[F] = Gen.from(Sync[F].delay {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume that cats.effect.std.Random is not an option here (though it expresses the capability of generating some random bytes way better than Sync because it's not in cats-effect-kernel?

Other than that, the PR looks good to me 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I intentionally didn't use Random as a bound on F[_] since it doesn't have auto derivation from Sync.
I could summon Random instance within my implementation, but that's not even shorter and brings cats.effect.std dependency.

@catostrophe
Copy link
Copy Markdown
Member Author

P.S. We force new integrations to be compatible with the rest t4c ecosystem. AWS X-Ray is ok since it uses the same 16-byte traceIds, but not fully random, but partially clock-based. These traceIds will work fine with all the tracing systems we have integration with.

Copy link
Copy Markdown
Collaborator

@janstenpickle janstenpickle 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 to me, thanks 👍

@catostrophe
Copy link
Copy Markdown
Member Author

@mrdziuban please, confirm you're fine with this PR, then I'll merge it and you can proceed with yours

@mrdziuban
Copy link
Copy Markdown
Contributor

👍 yup, looks good to me!

@catostrophe catostrophe merged commit 491cff7 into master Apr 14, 2022
@catostrophe catostrophe deleted the feat/trace-id-gen branch April 14, 2022 18:15
@ybasket ybasket mentioned this pull request Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants