search

sneako / finch

Elixir HTTP client, focused on performance
MIT License
1.23k stars 114 forks source link

Add finch instance name to telemetry metadata #252

Closed arthurnovak closed 5 months ago

arthurnovak commented 7 months ago

Recently, my team have adopted the practice of employing distinct finch instances for handling our requests. In this commit, we aim to include the pool-id/finch-instance-name in the telemetry metadata during request processing. This addition is particularly valuable for collecting statistics on a per-finch-instance basis.

This change aligns with both @aselder 's request and @oliveigah 's PR, but it's a bit more concise. Hope this makes the review and merging process quicker.

sneako commented 7 months ago

Thanks for taking a look at this!

A few comments:

arthurnovak commented 7 months ago

Hey @sneako thank you very much for the review. I've updated :queue docs in telemetry module.

Regarding the other comments, I completely agree that it'd be great to report finch instance name everywhere. However, I'd prefer to make the rest of the updates in separate PRs so that the changes won't appear complex and can be quickly reviewed. Let me know if having only the queue update in this PR would work

isavita commented 7 months ago

If we move the pool_name to opts we will not change the arity etc.

arthurnovak commented 7 months ago

hey @sneako can you give us any feedback please

sneako commented 7 months ago

Thanks! I'd be ok with separate PRs for h1 & h2, but I'd rather not have separate PRs for each individual telemetry event.

The current implementation seems fine to me, I'm not too concerned about changing the arity of these internal/private behaviours and I don't feel it belongs in opts anyway.

arthurnovak commented 7 months ago

hey @sneako thank you for your reply. I've updated the pr by adding finch name to the metadata for :connect, :recv and :send events in http1 and ignored http2 one for now. Please have a look when you have a min. Cheers!

arthurnovak commented 7 months ago

hi @sneako have you had a chance to have a look at the change?

arthurnovak commented 5 months ago

hey @sneako, I had a bit of time around New Year's, so I went ahead and added the finch name to the telemetry meta for the H2 interface, as we discussed earlier. Could you please take a look? Oh, and by the way, happy new year!

sneako commented 5 months ago

hey @sneako, I had a bit of time around New Year's, so I went ahead and added the finch name to the telemetry meta for the H2 interface, as we discussed earlier. Could you please take a look? Oh, and by the way, happy new year!

Happy new year to you as well! Thank you for picking this up and completing the implementation. This looks good to me. Please run mix format and I will merge :)

arthurnovak commented 5 months ago

hey @sneako, I had a bit of time around New Year's, so I went ahead and added the finch name to the telemetry meta for the H2 interface, as we discussed earlier. Could you please take a look? Oh, and by the way, happy new year!

Happy new year to you as well! Thank you for picking this up and completing the implementation. This looks good to me. Please run mix format and I will merge :)

done!