Instrument queries #29
Conversation
Not definite on the event names and payloads here. Just getting this going.
|
@mtodd I've done a quick once over but I haven't worked in this code before. Have you verified that previous behavior works and that we get the logging output we expect since tests are failing? |
|
I'm going to try a slightly different direction that taps in at a single chokepoint. We won't have great details on what generated the query but I'm a little concerned about introducing so much change in a bugfix release. I'll leave this here and start another PR for that. |
|
@mcolyer I haven't. Tests aren't working, and this doesn't really do anything until you subscribe to the events that the instrumentation service will emit when in use. @rtomayko 👌 happy to hear more of what you were thinking. I'm basing this off of gjtorikian/html-pipeline#45 somewhat since it's been super helpful in making the HTML pipeline's internal workings visible for external logging et al. |
|
@mtodd Is this something we'll want to backport? |
|
@MikeMcQuaid I don't think so.
No worries, was just trying to leave helpful comments at the time but now it sounds like @rtomayko is working something up. |
|
Still want to get this incorporated, but not urgently. @rtomayko happy to follow up with specific instrumentation/logging/metrics otherwise. |
Mostly I'm just kind of down on sprinkling these types of instrumentation blocks all over the codebase if you can find a single chokepoint. You lose a little meta information but are guaranteed to instrument calls added in the future and it's less error prone given that there's fewer changes. I tried to run with that approach, though, and the net-ldap library just doesn't make it easy unfortunately. Happy to see you continue work in this branch as planned, was just hoping for something a bit less intense that got us 90% of the way. |
👍 |
Conflicts: lib/github/ldap.rb
Conflicts: lib/github/ldap.rb
Covered by the chokepoint (search) instrumentation.
| rs = @connection.search(options.merge(:base => base), &block) | ||
| result.concat Array(rs) unless rs == false | ||
| end | ||
| instrument "search.github_ldap", options.dup do |payload| |
There was a problem hiding this comment.
This is the most substantive change in this branch, which I think is good to go. We're wrapping the chokepoint with instrumentation to get an idea of every search that this library generates. And what this doesn't instrument, https://github.com/github/ruby-net-ldap/pull/1 should provide us with visibility into.
This is because we modify what comes in (with, at the very least, :result).
Starting work on adding instrumentation around every query reaching out to the LDAP server, especially in loops.
This adds
GitHub::Ldap#instrumentthat takes an event name and payload with a block that will proxy out to the configured instrumentation service (ActiveSupport::Notifications, for instance). If no instrumentation service is configured, the block is called normally.Next step is to wrap queries with
instrument.cc @rtomayko @sbryant @mcolyer @MikeMcQuaid @dbussink @jmickeyd