Skip to content

Conversation

derekkraan
Copy link
Contributor

@derekkraan derekkraan commented Jul 1, 2025

This ensures that when :on_preloader_spawn is set in the prepare_query/3 callback, it is propagated to the preloader.

(For avoidance of XY issues, I am trying to find a way to propagate opentelemetry tracing context into the preloads, so that preloaded queries can appear in my opentelemetry traces like normal, and not appear as orphaned spans. The on_preloader_spawn option appears to be designed for exactly this. But, there is currently no convenient way to set this globally. Currently it only works when you add it to the opts in Repo.all/3 etc)

@josevalim
Copy link
Member

Fantastic, could we perhaps have a test? We have some unit tests in Ecto.Repo where we send messages or use the process dictionary for the options, perhaps this could be added there? Thank you.

@derekkraan
Copy link
Contributor Author

derekkraan commented Jul 1, 2025

Good god you're quick. I was editing my main comment and got a little jump scare from your reply 😂

Happy to know that it could be merged. I will add a test.

This ensures that when `:on_preloader_spawn` is set in the
`prepare_query/3` callback, it is propagated to the preloader.
@derekkraan derekkraan force-pushed the prepare_on_spawn2 branch from c331940 to c92d8a8 Compare July 1, 2025 12:47
@derekkraan
Copy link
Contributor Author

derekkraan commented Jul 1, 2025

I thought I'd just fix up a test and the PR would be good, but it turns out we'd also have to run prepare_query/3 in Repo.preload/3 somewhere to get the desired effect.

This also turned out to be a red herring -- someone figured out how to link Ecto preloads ages ago (using pdict $callers). The real fix is in opentelemetry_phoenix.

So I've pushed to the branch, and I'm leaving a note here for myself (or anyone else who wants to pick this up later). And closing the PR for now.

@derekkraan derekkraan closed this Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants