To support ES 6.0 #7
Conversation
Adding Header fields
|
Yeah, sure
Will change
Thanks
…On Wed, Nov 28, 2018 at 3:30 PM David Backeus ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/newrelic/elasticsearch.rb
<#7 (comment)>
:
> @@ -19,22 +19,31 @@
NewRelic::Agent::MethodTracer.extend(NewRelic::Agent::MethodTracer)
::Elasticsearch::Transport::Client.class_eval do
- def perform_request_with_new_relic(method, path, params={}, body=nil)
+ @@old_version = false
Could this be shortened to @old_version =
method(:perform_request).parameters.length == 4 instead?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#7 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQBehMRp5sBBj7yRaedu_-N7ZFBNODJYks5uzl6hgaJpZM4Y29tG>
.
|
| statement[:scope] = resolver.scope | ||
| statement[:additional_parameters] = resolver.operands | ||
|
|
||
| NewRelic::Agent::Datastores.notice_statement(statement.inspect, elapsed) if statement | ||
| end | ||
|
|
||
| NewRelic::Agent::Datastores.wrap('Elasticsearch', resolver.operation_name, resolver.index, callback) do | ||
| perform_request_without_new_relic(method, path, params, body) | ||
| if @@old_version |
There was a problem hiding this comment.
This is probably not the right way to do this. Pretty sure the Elasticsearch gem we're monkey patching has a VERSION constant. Interrogate that constant with Gem::Version from the Stdlib and I think we can merge this.
There was a problem hiding this comment.
Yeah I think in this case we could reference ::Elasticsearch::Transport::VERSION. This would make it clearer what "old version" actually means.
Could also consider moving the conditional into the metaprogramming level to avoid having to check the boolean on every single elasticsearch query.
There was a problem hiding this comment.
I don't see a need for @@old_version class var, at all. Adds unnecessary complexity. Also, is it thread-safe?
Simplest backward-compat is to not pass headers through if it's nil.
| if @@old_version | |
| if headers.nil? |
Benchmark: having a conditional, at all, is likely going to add 40% overhead to this block, but is (0.3744 - 0.6155) / 10_000_000 = 0.0000000241 seconds per call enough to warrant another solution?
null = nil
obj = { abc: 123 }.freeze
Benchmark.bm(3) do |b|
b.report('direct') { 10_000_000.times { true } }
b.report('if_nil') { 10_000_000.times { if null.nil? then true else false end } }
b.report('if_obj') { 10_000_000.times { if obj.nil? then true else false end } }
end
user system total real
direct 0.372937 0.000791 0.373728 ( 0.374498)
if_nil 0.612204 0.001569 0.613773 ( 0.615512)
if_obj 0.622998 0.002316 0.625314 ( 0.627959)
There was a problem hiding this comment.
With regards to thread safety you can't have more than one gem version installed at a time so the value of @@old_version will never mutate (ie. it's thread safe).
That said - if we compare using the VERSION constant we don't need it anyway.
|
Go ahead and bump the minor in your PR please. |
|
@lokanadham100 could you finish up the last requested changes for the pr? That would be super nice? :) |
Adding Header fields