Iterating on X-Ray support#452
Conversation
…ser-passed kernel doesn't contain the necessary information
| def fromKernel[F[_] : Concurrent : Clock : Random : XRayEnvironment]( | ||
| name: String, | ||
| kernel: Kernel, | ||
| entry: XRayEntryPoint[F] | ||
| ): F[XRaySpan[F]] = | ||
| kernel.toHeaders | ||
| .get(Header) | ||
| .flatMap(parseHeader) | ||
| .map(x => fromHeader(name, x, entry)) | ||
| .get | ||
| entry: XRayEntryPoint[F], | ||
| useEnvironmentFallback: Boolean = true, | ||
| ): F[Option[XRaySpan[F]]] = | ||
| OptionT.fromOption[F](kernel.toHeaders.get(Header)) | ||
| .subflatMap(parseHeader) | ||
| .semiflatMap(fromHeader(name, _, entry)) | ||
| .orElse { | ||
| OptionT.whenF(useEnvironmentFallback) { | ||
| XRayEnvironment[F] | ||
| .kernelFromEnvironment | ||
| .flatMap(XRaySpan.fromKernel(name, _, entry, useEnvironmentFallback = false)) | ||
| } | ||
| .flattenOption | ||
| } | ||
| .value |
There was a problem hiding this comment.
I think a better way is to just read the environment variables and construct the kernel before fromKernel is called, so that people can use whatever method they prefer for getting the trace context.
There was a problem hiding this comment.
Sorry, I couldn't quite follow. Do you mean, we should only read from the environment variables once, during setup?
There was a problem hiding this comment.
Not exactly. When you use X-Ray you always know whether you should use some headers or the environment variables. I don't think the concept of a fallback will ever be necessary. The construction of the kernel is application specific and you probably never mix environment variables and HTTP headers for instance.
If I am using AWS lambda I will always be using environment variables. If I am writing a http4s service I will always be using HTTP headers.
There was a problem hiding this comment.
Oh, I see, thanks for explaining. So basically, expose this as a config option when creating the EntryPoint? Or do you mean, the EntryPoint itself should never grab the kernel from the environment, and it should be up to the user to do this manually?
There was a problem hiding this comment.
In the entry point you always pass the kernel when you want to create an initial span from an existing trace. How this kernel is constructed is up to you.
There was a problem hiding this comment.
Apologies if I missed something, but I think the only hole in the current config is if someone wants to raise an error (or create a root) if the provided kernel is lacking even if the environment can in fact provide a Trace ID as a fallback
They can do this if the set the useEnvironmentFallback param to false, right?
There was a problem hiding this comment.
I think this option is on a package-private method and furthermore not available in the natchez Entrypoint abstraction. So the only place a user can hypothetically adjust this config is when they create the XRay entrypoint.
There was a problem hiding this comment.
Oh, right. Ok, so we need to propagate that option outward.
There was a problem hiding this comment.
I updated the PR with what I think were the suggested changes. Please let me know what you think!
There was a problem hiding this comment.
👍 yes, exactly what I had in mind, thanks :) @christiankjaer what do you think?
…nstructor to span creation
|
This looks good, can you rebase or otherwise resolve? |
|
I'll do that! |
|
superseded by #468 |
4 participants
This addresses some PR comments in #427 that we decided to hold up for a followup PR.