spandex-project / spandex_phoenix

Phoenix Instrumentation tracer
MIT License
82 stars 29 forks source link

feat!: Add Phx 1.5 Telemetry Handlers #26

Closed novaugust closed 4 years ago

novaugust commented 4 years ago

This is work to close #23

Design

Check out the readme update - does that seem like a good approach for getting people to attach event handlers from this lib?

Note that we're losing the ability to instrument views in 1.5

Phx 1.5 & Spandex Phoenix..

Moving the test dep on phx to 1.5 breaks a lot of the existing tests, unsurprisingly. Would you be interested in releasing a backwards incompatible version of Spandex Phoenix for Phoenix >= 1.5?

If we did go to a hard requirement on Phx 1.5 +, we might look at using telemetry events only and removing the reliance on the use Phoenix.Spandex macro.

All Phx Telemetry events are here, and i'll excerpt relevant ones below:

The first two replace the def call in the endpoint, and the third one replaces most of the error handling but maybe not all? I'd have to do more digging into error rendered to see.

Testing

Can't write tests for it until we have Phx 1.5, so blocked on the above =)

novaugust commented 4 years ago

@GregMefford lemme know what you think. In the meantime, implementing this shows me I can use the current Spandex Phoenix w/ phx 1.5 in my own app by just setting up these telemetry listeners and not using the instrumenter, so woohoo.

zachdaniel commented 4 years ago

This looks good to me so far, had a commend and a question.

egze commented 4 years ago

Is there any way I can help? Would really love to have something for Phoenix 1.5.

novaugust commented 4 years ago

Eh, it's blocked on my questions re: phx 1.5 (testing), but also on the fact that I only have a day or two every other month to work on OSS ;) if someone else has something that works, i'm pull my draft down and point at theirs. i'd point out that anyone waiting for phx 1.5 integration can just manually integrate w/ telemetry like i'm doing here, test it in their own app, and report back with findings. i spiked it out at my own workplace but haven't been able to push the project forward

GregMefford commented 4 years ago

Yep, sorry this has sat for so long. I’ve also just been limited by time and energy lately outside of work. I’d like for this upgrade to just work smoothly for folks, so we need to be careful to do what we can there. For example, we don’t want to double-trace things, but we also don’t want to break things for people by requiring that they do something that they didn’t need to do before.

novaugust commented 4 years ago

@GregMefford dude, i feel you there. noooo worries.

So, I got traces working via telemetry. Installation documentation is easy: something in the readme for "Phx >= 1.5" and one part for older phx. The crux comes down to what you want for testing. Getting phx 1.5 into this repo breaks all the tests, and I'm only so excited about rewriting the entire test story for this lib xD If no tests works for you, it'd work for me! Orrr I could add telemetry as a test dep and manually release those events just to prove that traces get created for them. LMK what approach works for ya'll and I can get it done in the next month

novaugust commented 4 years ago

I’d like for this upgrade to just work smoothly for folks, so we need to be careful to do what we can there.

i should be clear on what the upgrade path would be. it would be:

  1. undo any installation from previous spandex_phx
  2. call Spandex.Phoenix.Telemetry.install/1 in your application startup

and i should also point out, there's no dependency on phx for that and this library is really almost not even needed. could just add a new module in spandex itself ;) (this is what i've done at work)

egze commented 4 years ago

I will start upgrading our phx app to 1.5 tomorrow and will surely test this PR as well. But I can already say that I love how it looks. ❤️

zachdaniel commented 4 years ago

This looks good to me. @novaugust if you wouldn't mind, can you add a bit more text to the section on using this w/ phoenix 1.5 to explain that you should remove any setup you currently have as well? I'll merge and release when that is done.

novaugust commented 4 years ago

@zachdaniel sure, i'll do a bit of cleanup and documentation and ping you

novaugust commented 4 years ago

@zachdaniel i think that's the money

zachdaniel commented 4 years ago

This has been released as version 1.0.0.

🚀Thank you for your contribution! 🚀