-
Notifications
You must be signed in to change notification settings - Fork 170
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
DTrace probes? #91
Comments
hey @sax - a few thoughts. I'm thinking there are lots of spots to allow named or generic callbacks like that. one case is from #85 where there is a way to have an override the pool selection. @mnelson suggested
|
This sounds like a good solution. I'll work on a pull request now. |
@sax @jacobbednarz any thoughts on these two PRs (#126 and #103)? I'm just catching up and attempting to be a good citizen here. Are you using either one of them in prod? |
I'm no longer at the company where we did this work. They are still using #103 in production, but I don't see any problems with going directly to ActiveSupport::Notifications (unless you want to eventually drop the hard dependency on ActiveSupport). It would be pretty easy to switch over the DTrace probes to hook into AS notifications rather than the custom notification module. #126 doesn't provide any tests for the notifications, if you care about such things. |
Thanks @sax |
The purpose behind my pull request was getting some sort of framework in I will be revisiting the read/write split for our DB's however at this |
I have some DTrace probes added to a branch I created to test out #87 in our infrastructure. Would you be interested in having a pull request for these?
I can understand your response either way... Feature utility vs. feature maintenance. Just let me know!
The text was updated successfully, but these errors were encountered: