Bulk helper should instrument entire operation #105

Activity

1. chrismwendt commented on Sep 10, 2015

   

   chrismwendt

   on Sep 10, 2015

   Contributor

   More actions

   How will the arguments to Client#instrument for the entire operation differ from the individual bulk requests? It seems appropriate to avoid ambiguity between the two different concepts by passing a different path, but there is no path associated with the entire operation since it's a concept and not an actual request to ES.

   Here is the bulk request: https://github.com/github/elastomer-client/blob/master/lib/elastomer/client/bulk.rb#L41
2. grantr commented on Sep 10, 2015

   

   grantr

   on Sep 10, 2015

   ContributorAuthor

   More actions

   I'm thinking we could omit the body, keep the path the same, and set the action to something synthetic, like bulk.total or bulk.block or something.

   Maybe something like this:

   def bulk( body = nil, params = nil )
         if block_given?
           params, body = (body || {}), nil
           instrument(path, body, params.merge(:action => "bulk.block") do
             yield bulk_obj = Bulk.new(self, params)
             bulk_obj.call
           end
         else

   What do you think @TwP?
3. chrismwendt commented on Sep 17, 2015

   

   chrismwendt

   on Sep 17, 2015

   Contributor

   More actions

   delete_by_query should also be instrumented.
4. TwP commented on Sep 17, 2015

   

   TwP

   on Sep 17, 2015

   Contributor

   More actions

   I'm on the fence about instrumenting an entire block of bulk operations. The resulting metric will be a weird superposition of application timing information and Elasticsearch timing information. If it were possible to aggregate just the Elasticsearch timing information and store that as a single metric, then I would be 👍💯

   Diving a little bit into implementation, we could achieve this by accumulating the timing data in the bulk instance itself. At the end of the block this timing data could be instrumented. I'm not sure if ActiveSupport::Notifications support a form where you can pass in the raw timing data or not (instead of relying on the block notation).

   That seems like the best path forward to me. Just my $0.02
5. chrismwendt commented on Sep 17, 2015

   

   chrismwendt

   on Sep 17, 2015

   Contributor

   More actions

   Getting the timing data from Elasticsearch would be easy using bulk_stream_responses in #112, but it doesn't look like there is a way to override the instrument block duration with your own value (see instrument, start, and finish):

   http://api.rubyonrails.org/classes/ActiveSupport/Notifications/Instrumenter.html#method-i-instrument

   The application timing could also affect delete_by_query if that were instrumented.