Add finch instance name to telemetry metadata #252
Conversation
|
Thanks for taking a look at this! A few comments:
|
arthurnovak
force-pushed
the
add_finch_instance_to_req_telemetry_meta
branch
from
da9a831
to
6fcfc78
Compare
|
Hey @sneako thank you very much for the review. I've updated 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 |
|
If we move the |
|
hey @sneako can you give us any feedback please |
lib/finch/pool.ex
Outdated
| Finch.Request.t(), | ||
| acc, | ||
| Finch.stream(acc), | ||
| atom(), |
There was a problem hiding this comment.
| atom(), | |
| Finch.name(), |
lib/finch/pool.ex
Outdated
| @callback async_request( | ||
| pid(), | ||
| Finch.Request.t(), | ||
| atom(), |
There was a problem hiding this comment.
| atom(), | |
| Finch.name(), |
|
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 |
arthurnovak
force-pushed
the
add_finch_instance_to_req_telemetry_meta
branch
3 times, most recently
from
13887a7
to
094cc48
Compare
|
hey @sneako thank you for your reply. I've updated the pr by adding finch name to the metadata for |
|
hi @sneako have you had a chance to have a look at the change? |
arthurnovak
force-pushed
the
add_finch_instance_to_req_telemetry_meta
branch
from
094cc48
to
d7de556
Compare
arthurnovak
changed the title
arthurnovak
force-pushed
the
add_finch_instance_to_req_telemetry_meta
branch
from
d7de556
to
77fd65e
Compare
|
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
changed the title
Happy new year to you as well! Thank you for picking this up and completing the implementation. This looks good to me. Please run |
done! |
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.