Skip to content
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

Add a variant of Trace.ioTrace that takes an existing IOLocal #1061

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Sep 11, 2024

In absence of a built-in IOLocal[A] => Local[IO, A] which would allow using natchezMtlTraceForLocal (might eventually be available if typelevel/cats-effect#3429 ever gets merged and released), this would allow for some useful Java interop in a style similar to what otel4s suggests.

The ability to directly create (and thus, read afterwards) the IOLocal that holds the current span, lets some Java interop apply the following process:

  • capture the current value in an effect
  • downcast to a particular Span implementation, e.g. DDSpan
  • extract the underlying java Span, e.g. the opentracing Span
  • in a delay block, set the current span of the global Tracer instance to the span it just found.

I acknowledge this might not be a popular usecase (given nobody requested it by now), but it'd be quite helpful to my team at the moment.

@kubukoz
Copy link
Member Author

kubukoz commented Sep 13, 2024

Kindly requesting a look @mpilquist 🥺

@bpholt
Copy link
Member

bpholt commented Oct 4, 2024

I was going to suggest copying catsMtlEffectLocalForIO into natchez-mtl until it's adding to cats-effect, but maybe this is fine instead.

My main hesitation is that we'd want to be careful (both now and in the future) that this Trace[IO] doesn't do anything that can't be encoded using Local[IO, Span[IO]] semantics, since I think it's been concluded that Local semantics are safe, but using IOLocal directly can be dangerous in some scenarios? (See typelevel/cats-effect#3385 for some additional discussion on this point. And to be clear, I'm sure @kubukoz has seen that, I'm just noting it for the record here.)

@kubukoz
Copy link
Member Author

kubukoz commented Oct 4, 2024

yeah, I would've liked to have Local[IO, E] at the ready and to implement this in terms of Local, but I'm hopeful that we can get typelevel/cats-effect#3429 (or similar, but indeed a solution for typelevel/cats-effect#3385) soon, and I wouldn't want there to be an implicit conflict. I guess we could keep the instance, and then un-implicit it when CE gets one of their own?

@bpholt
Copy link
Member

bpholt commented Oct 15, 2024

If we didn't want to publicly expose a copy of catsMtlEffectLocalForIO (although I do think including it here and then making it no longer implicit once it's available in cats-effect should be fine), one compromise would be to make a private copy and use that to implement def ioTraceLocal(local: IOLocal[Span[IO]]): Trace[IO]. Just brainstorming, really. 🤔

@kubukoz
Copy link
Member Author

kubukoz commented Oct 17, 2024

Sorry for the long wait, it's a busy time at work and I have no puter time left. I think you're right, we can always hide that implicit when there's an alternative with the same behavior.

@mergify mergify bot added the core label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants