feat(telemetry): add :telemetry.span for handle_failed/2 callback#369
Conversation
yordis
force-pushed
the
yordis/handle-failed-telemetry-span
branch
3 times, most recently
from
b22bfe7 to
61375f6
Compare
yordis
force-pushed
the
yordis/handle-failed-telemetry-span
branch
5 times, most recently
from
f76f8e5 to
c22a417
Compare
yordis
force-pushed
the
yordis/handle-failed-telemetry-span
branch
from
c22a417 to
a989ad9
Compare
There was a problem hiding this comment.
This try-catch most likely should be outside of the telemetry, otherwise you are hiding failures. We should probably have a test for that.
| fn -> | ||
| result = do_handle_failed_messages(messages, module, context) | ||
| {result, Map.put(metadata, :messages, result)} | ||
| end |
There was a problem hiding this comment.
Also please investigate if the handle_failed is not included in the other telemetries. It may be. In this case, a regular event may be better than a span.
There was a problem hiding this comment.
As far as I can tell, they are, unless I made a silly mistake here
|
Thank you. Comments inline. Keep in mind you can implement |
|
@josevalim I am aware of the own callback; I am doing that today. The reason I added it to Broadway is that it is the recurrent situation of creating macros and other things for stuff like this. After a while, I feel that the pattern holds water, primarily so that most engineers do not need to worry as much about doing the right thing. Should I continue working on it, or do you prefer I continue copy+pasting? I do not have strong preferences, just this time decided to open a PR to at least bring it upstream. I have been working on OTEL and o11y, my goal is to have very good insights from the getgo. |
|
I think we can merge it if the concerns above are addressed, but I also need to catch up with the codebase and see if there are any inconsistencies by doing this change. |
|
@josevalim I have another PR I want to make, same vane, OTEL stuff to be able to instrument OTEL a bit better, I will keep going 😄 Addressing your concerns at the moment |
yordis
force-pushed
the
yordis/handle-failed-telemetry-span
branch
from
a989ad9 to
a2364af
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/handle-failed-telemetry-span
branch
from
a2364af to
09cf1de
Compare
|
@josevalim sorry for the diff-ing 😭 if you have a better way, let me know. I tried to add enough tests to cover the situation |
|
If you have another PR, then please let me know or submit a proposal. I'd rather understand all the telemetry events that need to be added, so I can take a holistic view, rather than adding incompatible ones one by one. |
|
main...yordis:broadway:yordis/add-producer-to-telemetry @josevalim they are unrelated, the other one is simply adding the |
|
💚 💙 💜 💛 ❤️ |
2 participants
Add
[:broadway, :handle_failed, :start | :stop | :exception]telemetry span events for thehandle_failed/2callback. It will help with the o11y around handling the failure case, we can hook metrics, and OTEL spans as well