-
Notifications
You must be signed in to change notification settings - Fork 78
Add method to run code with activated span for interop with Java libraries #1189
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
base: main
Are you sure you want to change the base?
Conversation
943e660
to
6afd135
Compare
* | ||
* This should always be wrapped in `Sync.suspend` or equivalent. | ||
*/ | ||
def unsafeRunWithActivatedSpan[T](run: => T): T = run |
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.
This is a surprising API, in my opinion. I think it's very likely to be a footgun for inexperienced developers, despite the note about it needing to be wrapped in Sync.suspend
. Is there a way we can do more to enforce that requirement? Why not accept and return an F[T]
?
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.
@bpholt there's no way to take an F[T]
and ensure that the span is activated every time it runs compute tasks (which is a prerequisite to having the desired semantics). You can't even force it to run on one thread with evalOn
because it might contain blocking
inside.
I have dealt with this exact problem before, and had a custom implementation that made it work... but the only good thing we can do here is return F[T]
, imo.
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 can't call it from inside suspend
if I take or return F[T]
. This is meant to be used entirely on impure side hence unsafe in the name.
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.
coming back to this, I think it might be possible to take an F. With caveats:
- if your F contains async boundaries, auto-cedes, etc. you'd be calling that
unsafeRunWithActivatedSpan
every time you go back to "compute" tasks. - if there are any
evalOn
/blocking
s inside the effect, they wouldn't have the span activated.
Something like this:
def runWithActivatedSpan[T](ft: F[T]): F[T] = {
val ec = new ExecutionContext {
def execute(r: Runnable): Unit = unsafeRunWithActivatedSpan(r.run())
def reportFailure(e: Throwable): Unit = throw e
}
ft.evalOn(ec)
}
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 is your use case? The libraries that require such integration do not use cats (or scala).
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.
The usecase is when you already have an effect that wraps Java code, and you want to give it the scope. This might happen if you're using a library that wraps Java (so you can't modify it to use the underlying unsafe function) or you want to centralize that kind of context handling in a middleware of sorts
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.
An effect wrapping Java code would have to wrap it in blocking
(pretty much any code using tracing is blocking) and you just said this code does not work with blocking
inside effect.
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.
lol fair enough, I take that back then 😆
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.
though not all Java code is blocking... still I don't think it's very useful without support for blocking blocks
for the record, I don't like the idea of a custom |
I'm not sure what you mean by a custom def runWithActivatedSpan[T](run: F[T])(implicit F: Sync[F]): F[T] (edit: maybe you weren't replying to my message, in which case, please disregard 🙂) |
@bpholt yeah I was referring to the first snippet in the PR's description :) |
That's why I didn't add it in this PR but I couldn't find a better way. |
With this method in place I can fix all my datadog java instrumentation issues by simply activating span on
Sync.suspend
by providing custom instance like so:And then when lifting resources to Kleisli:
I suppose I could add this to natchez but supporting scala 2 would require a lot of boilerplate code in natchez.
Ideally paired with #1185.
For
ioTrace
something like this should work (with tagless final code) although I haven't tested it: