Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exception to instrumentation payload #145

Closed
wants to merge 1 commit into from

Conversation

marcotc
Copy link

@marcotc marcotc commented Nov 22, 2024

While trying to instrument Karafka for the datadog gem (DataDog/dd-trace-rb#4147), we noticed that Notifications#instrument does not invoke the event listeners when the instrumented block errors.

This happens because an error in the instrumented block exits the Notifications#instrument method before it has a chance to invoke its listeners.

This means that we cannot use Notifications#instrument to capture all invocations of the instrumented block. On top of that, we also do not have access to information on the exception that was raised if we wish to report on it (which we do in datadog).

This PR ensures that listeners are involved regardless of errors happening in the instrument block, as well as providing the exception object in the event payload when an exception happens.

Some inspiration was taken from how ActiveSupport::Notifications implements its error path: https://github.com/rails/rails/blob/main/activesupport/lib/active_support/notifications/instrumenter.rb#L60-L61

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -117,27 +117,30 @@ def instrument(event_id, payload = EMPTY_HASH)
return yield if assigned_listeners.empty?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that we might also want to remove this early safeguard (if assigned_listeners.empty?) here above, given there is a similar safeguard in the ensure block below.
I understand that we are adding the overhead of two calls to monotonic_now in such case, in exchange for one less assigned_listeners.empty? check.

@mensfeld
Copy link
Member

This PR cannot be merged for two reasons:

  1. The CLA is not signed
  2. The proposed change breaks the current instrumentation contract, drastically changing how the listener API works, thus affecting all the other components in the chain.

This means that we cannot use Notifications#instrument to capture all invocations of the instrumented block. On top of that, we also do not have access to information on the exception that was raised if we wish to report on it (which we do in datadog).

This is why Karafka and WaterDrop already ship with a solution for this that is already used by OpenTelemetry for example and several custom integrations:

https://karafka.io/docs/Monitoring-and-Logging/#monitor-wrapping-and-replacement

You can use a custom monitor that will provide APIs needed by DataDog without breaking current contracts for other listeners. Your change would not only break my current DD integration but also others like AppSignal and even some things in the Web UI.

I understand that "my" way of doing things is different than the one provided by ActiveSupport, but I fundamentally do not believe your proposition improves things to a degree that would justify such an extreme API change. This would be a huge breaking change in the whole ecosystem without extreme benefit because of the reason mentioned above (ability to do exactly what you need via a custom monitor compatible with "yours" and "my" APIs).

@mensfeld mensfeld self-requested a review November 22, 2024 22:13
@mensfeld mensfeld assigned mensfeld and marcotc and unassigned mensfeld Nov 22, 2024
Copy link
Member

@mensfeld mensfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in the comment.

@mensfeld
Copy link
Member

Since I do not see any activity here I will close it.

@mensfeld mensfeld closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants