-
Notifications
You must be signed in to change notification settings - Fork 125
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
Lost parent context when Phoenix LiveView assign_async is used #302
Comments
I took a peak at the source code and yeah, it's not going to be possible to have it be automatic. The tasks aren't the only problem, there's also this message passing which nothing can propagate through. Maybe somebody could get incredibly clever but, in the meantime, you'll need to manually propagate as you have in the 3rd example. The main issue is to make it automatic we'd have to extend those LiveView modules to do that for you. See Task for reference on what would have to be done. |
That sounds bad given how easy to use current |
What is |
I meant to better extend on what @bryannaegele said and not just repeat it, hehe. Should we not provide these functions as part of the Phoenix instrumentation library I mean? Instead of the user doing it. I'm sure someone would be interested in contributing that if we asked. |
Please see assign_async/4 and module async operations but if you're not using LiveView it might not be clear. Under the hood it's spawning Task, with some additional logic to nicely handle failures and results. Before I open any issue on LiveView, I'll explore exactly that - how easy it would be to provide alternative |
Right, I was wondering how it is used. So LiveView has keys that get assigned values and then replaced, with the option to have that replacement done at some future time when a Task completes? So probably used pretty often? I don't think there is anything LiveView can/should do differently that wouldn't involve integrating OpenTelemetry. Instead I think we should provide replacements (@bryannaegele can correct me there if its not something we'd be good to try to provide a drop-in for, like maybe it is one of those things since its a pre-1.0 project that may likely change too much/often internally). That and better document that implicit context is per-process and will not automatically propagate, requiring the user manually set it, like https://opentelemetry.io/docs/languages/erlang/instrumentation/#spans-in-separate-processes |
Absolutely yes, it's a recent LiveView-specific "primitive" but I expect it to be used by almost every LiveView app at some point. I'll test a drop-in in a separate repo and see how much needs to be copied over vs reused from LiveView.
I think the documentation is pretty clear about that and this wasn't a surprise to me, I was mostly surprised that even though Ecto telemetry uses Process Propagator here, any Ecto spans were detached from parent span when |
I had helper macros in my private repo, but I think based on |
Describe the bug
When Phoenix.LiveView
assign_async/3
is used, any spans from async task will not have parent context, even when OpentelemetryProcessPropagator is used. This means that any Ecto query is detached span.Expected behavior
I'd expect parent context to be propagated.
Additional context
Repro script:
This gives the following debug log:
I'd expect to have parent id in
span-with-propagation
.I'm unsure if it's an issue with OpentelemetryProcessPropagator or the way async assign in LiveView works.
The text was updated successfully, but these errors were encountered: