Instrumentation: phoenix multiple endpoints support

[PragTob]

👋

First of all thank you a lot for your awesome work everyone 💚

The diff here is a bit more brutal than what actually is happening, to ease review the implementation is also split up into 2 commits.

Problem to fix

An application has multiple endpoints all using the same webserver (scenario: one for API, one for web). I had noticed that traces/spans weren't recorded correctly and the current span context wasn't set.

After some debugging I noticed that the root cause to the best of my ability is the little fix I did to lib - it seems to me that one would need to call OpentelemetryPhoenix.setup() multiple times - with the adequate event_prefixes.

However, that does not work since the :telemetry.attach call inside it used the static id of {__MODULE__, :endpoint_start} which means that subsequent handlers would not be attached. This showed itself in the behavior that setting up more than one in our application never worked.

Proposed fix

I propose the fix here to make the endpoint_prefix part of the telemetry handler id. This makes sense to me, as each one of those listens to different events so being their own separate entity makes sense. This also keeps the same API and just allows you to call it multiple times. Due to the id-centric behavior of :telemetry other handlers won't be attached twice or thrice (i.e. router_start_handler will only be attached once). Hence, I think this ifs fine.

Noise around the fix

I wanted to test this all and for the tests to show it (much to my surprise bandit worked without the fix 🤔 ).

As such, I extended the test to setup one additional "endpoint variant" each, defining the modules and everything. To support that, I needed to move some code around. The new endpoints are always defined, but only started in my added tests.

I tried not to change as much. I'm also not 100% happy with the data structure I ended up with but it seems to work so far.

---

Thank you for your consideration and all your work! 💚

[linux-foundation-easycla]

• ❌ - login: @PragTob / name: Tobias Pfeiffer . The commit (944d563, 6303324) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[PragTob]

(I did see the CLA things, I'd need to find someone in my org authorized for this)

[bryannaegele]

Hey @PragTob, amazing this was never caught.

We don't need to add all these tests for this fix. It would be fine to cover it with a single test doing two setups and verifying they're in the telemetry handlers.

[PragTob]

👋

Sorry, for the silence - managing my notifications can be kind of hard 😅

Not quite sure what you mean with the test - I did some reworking of the tests to fit my approach for testing. The real test is just one (although done with different adapters and protocols as iirc the other tests did).

However, I haven't managed to get a person at my company to be in charge of singing the CLA/authorizing the CLA. I'll try again next week, but it may be better for someone else to take implementing this and taking this as an issue report :(