ActiveSupport::Notifications::Event#parent_of? is not reliable in environments with poor clock resolution.#5932
ActiveSupport::Notifications::Event#parent_of? is not reliable in environments with poor clock resolution.#5932gnarg wants to merge 1 commit intorails:3-2-stablefrom
Conversation
…ironments with poor clock resolution. This is an attempt to resolve that by implementing per-transaction entry and exit counters.
|
If we backport f08f875, we can apply this patch to make Event not time based: |
|
Hi Aaron, Brilliant, this is even better. I'll try cooking up a prototype agent that exercises this then get back to you with the results. I anticipate that this is all we need (with the possible exception of #5933) but I'll confirm that. Thanks for this! |
|
Hi Aaron, I hacked up a quick proof of concept. I set this in a Rails 4 project (with the above patch applied) and everything seems to be working well. To walk the call graph of a node I need access to Event's "@children", so I had to use: event.instance_variable_get(:@children) An "attr_reader :children" would be swell. https://gist.github.com/2938578 It would be great if you landed the patch above in Rails 4, but I don't think back-porting to Rails 3 is necessary yet, at least from New Relic's perspective. We can wait until we are at least close to having an agent that makes full use of the mechanism. Thanks for you're help with this. I'm going to continue to work on this agent prototype and will let you know if I run into any snags. -jon |
|
@gnarg cool, thanks for the feedback. Ya, this missing |
|
If I apply my patch to master, do you still need this patch against 3-2-stable? Or should I just close this? |
|
You can close it, thanks! |
make events not use date and time to determine parent_of. fixes rails#5932
`ActiveJob` attaches a single `LogSubscriber` to be notified of events. When one job enqueues another, that's treated as a child event and stored on the same stack as the parent. If a large number of children are enqueued that leads to R14 errors on Heroku. The fix here is to unsubscribe all existing "enqueue" listeners and then add a new subscriber for just that event. The original handler is copied from the parent class using `define_method` so that it's picked up by `attach_to`. Related: rails/rails#21036 rails/rails#5932 Before and after: 
`ActiveJob` attaches a single `LogSubscriber` to be notified of events. When one job enqueues another, that's treated as a child event and stored on the same stack as the parent. If a large number of children are enqueued that leads to R14 errors on Heroku. The fix here is to unsubscribe all existing "enqueue" listeners and then add a new subscriber for just that event. The original handler is copied from the parent class using `define_method` so that it's picked up by `attach_to`. Related: rails/rails#21036 rails/rails#5932 Before and after: 
`ActiveJob` attaches a single `LogSubscriber` to be notified of events. When one job enqueues another, that's treated as a child event and stored on the same stack as the parent. If a large number of children are enqueued that leads to R14 errors on Heroku. The fix here is to unsubscribe all existing "enqueue" listeners and then add a new subscriber for just that event. The original handler is copied from the parent class using `define_method` so that it's picked up by `attach_to`. Related: rails/rails#21036 rails/rails#5932 Before and after: 
Hi,
Some system clocks, particularly on virtual non-realtime environments can as tick with very low clock resolution. In such an environment the Event#parent_of? method can erroneously conclude that multiple instrumentation events that take place within a single tick are all each other's parents.
To work around this I implemented entry and exit counters for instrumentation events. These counters should reset with each transaction.
I'm not married to this implementation, but I would love to work with you guys on the issue. It's one of the things that is holding us up from implementing the Notifications API in the New Relic Ruby agent.
Regards,
Jon Guymon