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 3 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
22 changes: 2 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 @@ -34,26 +34,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
31 changes: 10 additions & 21 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,9 +175,17 @@ 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(:version_greater_than_8) { Gem::Version.new(::Elasticsearch::VERSION) >= Gem::Version.new('8.0.0') }

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)
Copy link
Contributor Author

@ekump ekump Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@marcotc @TonyCTHsu When I added back this test to allow custom config on the transport object it initially failed. Turns out that prior to ES 8.0 client.transport refers to the client instance. client.transport.transport refers to the transport instance. With ES > 8.0 client.transport refers to the transport instance. So when I changed the patch of perform_request to not check Datadog.configuration_for(self, :service_name) and only do Datadog.configuration_for(transport, :service_name) it was getting back nil on ES 7.0 since there is a discrepancy between what we set with configure_onto and what we get with configuration_for.

As I see it we have 3 options:

  1. Keep my changes as they are, and update the docs to explain that if you are on ES < 8.0 you need to configure_onto client.transport.transport. This makes our code simpler, but our docs slightly more complicated.
  2. Keep my code changes, update the this test to skip ES < 8.0, and update our documentation to say we don't support custom configuration for transport below ES 8.0
  3. Change the code so that if ES < 8 we support configuration on the client object, but not support it for 8.0+ and remove the deprecation warning.

I think option 1 is the best balance of user-friendliness and code simplicity.

end
end

let(:service_name) { 'bar' }

Expand All @@ -198,25 +206,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