Skip to content
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.

Sign up for GitHub

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

Jump to bottom

remove access to patching elasticsearch client directly as it is not supported anymore by elasticsearch #3399

Merged
merged 5 commits into from
Jan 29, 2024
Merged

remove access to patching elasticsearch client directly as it is not supported anymore by elasticsearch #3399

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1dc782c
remove access to patching elasticsearch client directly as it is not …
ekump Jan 24, 2024
26b6bf8
wip
ekump Jan 25, 2024
821f631
update test to set correct transport object
ekump Jan 26, 2024
3f16639
linter fixes
ekump Jan 26, 2024
3ac27d8
Pin to transport
TonyCTHsu Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/datadog/tracing/contrib/elasticsearch/patcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def patch
require 'json'
require_relative 'quantize'

transport_module::Client.prepend(DatadogPin)
transport_module::Client.prepend(Client)
end

Expand Down Expand Up @@ -126,6 +127,25 @@ def datadog_configuration
# rubocop:enable Metrics/MethodLength
# rubocop:enable Metrics/AbcSize

# Patch to support both `elasticsearch` and `elastic-transport` versions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TonyCTHsu Is this the pattern we want to follow in situations where the instance we're trying to instrument may be different between versions of an integration? I didn't see any other direct examples of this (We do patch pinning in mongodb for different reasons).

I have two questions/concerns:

  1. Is the complexity / indirection we're adding to our code worth the tradeoff of simpler configuration?
  2. Are we potentially confusing users by masking ES implementation details like this? client.transport refers to a different object depending on ES version and we're providing a way to configure on an instance of that object. I could see the merit of an argument that we shouldn't be changing that object behind the scenes.

I lean slightly towards not doing this, but I don't have a strong opinion. At the end of the day either the library or the user has to set a different instance depending on ES version.

CC: @delner @marcotc in case you have any thoughts on a general strategy for these types of situations. We should probably agree on a pattern so we are consistent the next time it occurs. Some context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing the loop on this: Spoke to @marcotc and @TonyCTHsu and we all agreed with this pattern.

module DatadogPin
def datadog_pin=(pin)
pin.onto(pin_candidate)
end

def datadog_pin
Datadog.configuration_for(pin_candidate)
end

def pin_candidate(candidate = self)
if candidate.respond_to?(:transport)
pin_candidate(candidate.transport)
else
candidate
end
end
end

# `Elasticsearch` namespace renamed to `Elastic` in version 8.0.0 of the transport gem:
# @see https://github.com/elastic/elastic-transport-ruby/commit/ef804cbbd284f2a82d825221f87124f8b5ff823c
def transport_module
Expand Down
9 changes: 1 addition & 8 deletions spec/datadog/tracing/contrib/elasticsearch/transport_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,16 +177,9 @@ def call(env)

describe 'transport configuration override' do
context 'when #service is overridden' do
let(:version_greater_than_8) { Gem::Version.new(::Elasticsearch::VERSION) >= Gem::Version.new('8.0.0') }
let(:service_name) { 'bar' }

before do
if version_greater_than_8
Datadog.configure_onto(client.transport, service_name: service_name)
else
Datadog.configure_onto(client.transport.transport, service_name: service_name)
end
end
before { Datadog.configure_onto(client.transport, service_name: service_name) }

describe 'then a GET request' do
subject(:response) { client.perform_request(method, path) }
Expand Down
Loading