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

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 all commits
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
42 changes: 22 additions & 20 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 All @@ -34,26 +35,8 @@ module Client
# rubocop:disable Metrics/MethodLength
# rubocop:disable Metrics/AbcSize
def perform_request(*args)
# DEV-2.0: Remove this access, as `Client#self` in this context is not exposed to the user
# since `elasticsearch` v8.0.0. In contrast, `Client#transport` is always available across
# all `elasticsearch` gem versions and should be used instead.
service = Datadog.configuration_for(self, :service_name)

if service
SELF_DEPRECATION_ONLY_ONCE.run do
Datadog.logger.warn(
'Providing configuration though the Elasticsearch client object is deprecated.' \
'Configure the `client#transport` object instead: ' \
'Datadog.configure_onto(client.transport, service_name: service_name, ...)'
)
end
end

# `Client#transport` is most convenient object both this integration and the library
# user have shared access to across all `elasticsearch` versions.
#
# `Client#self` in this context is an internal object that the library user
# does not have access to since `elasticsearch` v8.0.0.
# `Client#transport` is the most convenient object both for this integration and the library
# as users have shared access to it across all `elasticsearch` versions.
service ||= Datadog.configuration_for(transport, :service_name) || datadog_configuration[:service_name]

method = args[0]
Expand Down Expand Up @@ -144,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
25 changes: 3 additions & 22 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 @@ -175,12 +175,12 @@ def call(env)
end
end

describe 'client configuration override' do
describe 'transport configuration override' do
context 'when #service is overridden' do
before { Datadog.configure_onto(client.transport, service_name: service_name) }

let(:service_name) { 'bar' }

before { Datadog.configure_onto(client.transport, service_name: service_name) }

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

Expand All @@ -198,25 +198,6 @@ def call(env)
expect(span.name).to eq('elasticsearch.query')
expect(span.service).to eq(service_name)
end

context 'configured at the Elasticsearch client level' do
before do
skip('Configuration through client object is not possible in Elasticsearch >= 8.0.0') if version_greater_than_8

Datadog::Tracing::Contrib::Elasticsearch::Patcher::SELF_DEPRECATION_ONLY_ONCE
.send(:reset_ran_once_state_for_tests)

Datadog.configure_onto(client, service_name: 'custom')
end

let(:version_greater_than_8) { Gem::Version.new(::Elasticsearch::VERSION) >= Gem::Version.new('8.0.0') }

it 'warns about deprecated configuration of the Elasticsearch client itself' do
expect { response }.to emit_deprecation_warning(
include('Providing configuration though the Elasticsearch client object is deprecated')
)
end
end
end
end
end
Expand Down
Loading