search

riak-ripple / ripple

A rich Ruby modeling layer for Riak, Basho's distributed database
Other
619 stars 152 forks source link

Activesupport instrumentation #292

Closed nyarly closed 12 years ago

nyarly commented 12 years ago

As it stands, this is sort of a bogus PR - I'm looking for feedback mostly on how to proceed.

I'm not entirely pleased with this implementation because a) alias_method_chain and b) it reaches deep into riak-client to get what it wants.

The alternative would be to modify riak-client to accept a "instrumentor" option and use that if it's available (essentially fold lib/ripple/instrumentation/http.rb and lib/ripple/instrumentation/protobuffs.rb into riak-client) and have Ripple send ActiveSupport::Notifications in (a la Excon).

And I'm not expecting this to be merged in this from because I've written no specs at all for it yet. Full stop.

nyarly commented 12 years ago

@seancribbs Could I get a review of this?

seancribbs commented 12 years ago

@nyarly Sorry for the delay. Generally I like your approach but I do understand your reservations about alias_method_chain. It would probably be better to place the instrumentations in the riak-client code with some sort of neutral interface, into which Ripple could inject the ActiveSupport notifications as the receiver. The default could be a sort of 'null' notifier that simply throws out the information. There are also other things that might be important to instrument as well.

nyarly commented 12 years ago

Hey, if you don't mind me nagging you a little bit, don't worry about it. :)

Okay, I'll migrate this code around to the appropriate places and issue new PRs.

seancribbs commented 12 years ago

@nyarly You're not nagging, you're keeping me honest. :smile: