OTel instrumentation by estolfo · Pull Request #54 · elastic/elastic-transport-ruby

estolfo · 2023-06-28T09:34:49Z

This PR complements a PR in the elasticsearch-ruby repo to extend the way the transport layer is called by including the endpoint id and path params for a given call to perform_request. It also adds the creation of an OTel span for each request.

The span attributes captured follow the semantic conventions proposed in this PR.

This PR does the following:

Defines a OpenTelemetry class that encapsulates everything having to do with otel instrumentation in the transport layer
Checks if the OpenTelemetry namespace is defined and creates a @otel object on the client
Adds the opentelemetry-sdk to the Gemfile
Requires a ENV variable TEST_WITH_OTEL that should be set to true if the opentelemetry-sdk gem should be required and the otel tests should be run
Creates an OTel span for each request to Elasticsearch

ToDo:

Only set the request body for search-type requests / define what a search-type request is
Add sanitization of the request body when it's set on the span, with a ENV variable to configure it
Figure out how to remove the otel namespace so spans aren't created in all tests when TEST_OTEL=true
Update opentelemetry tests to successfully run with ES available, if needed
Add GitHub action to run tests with TEST_OTEL=true
Inline documentation
General documentation (See this PR for docs)
configuration setting to enable/disable

spec/spec_helper.rb

spec/elastic/transport/opentelemetry_spec.rb

estolfo · 2023-07-03T09:03:12Z

@picandocodigo How do you suggest running these tests with the OTel instrumentation? The client is instrumented if the opentemeltry-sdk gem is required (i.e. the OpenTelemetry module is defined). I check for an ENV variable TEST_OTEL=true in the spec helper to require the open telemetry gem.
I don't want to have OTel instrumentation on by default for the tests so we don't slow them down but it would be nice to test the entire suite with instrumentation on, in case there are some edge cases with endpoint requests. So what do you think about doing a separate test run of the suite with TEST_OTEL=true?

picandocodigo · 2023-07-03T09:41:00Z

So what do you think about doing a separate test run of the suite with TEST_OTEL=true?

Sounds good! We can definitely add a new set of tests on GitHub Actions with the env variable 👍

picandocodigo · 2023-09-18T12:04:05Z

lib/elastic/transport/client.rb

+              span['db.statement'] = body_as_json
+            end
+            span['db.operation'] = opts[:endpoint] if opts[:endpoint]
+            transport.perform_request(method, path, params || {}, body, headers)


Since params is being passed with the default value of {} in the perform_request signature, I think we don't need || {} here and on line 197, right?.

With my testing, there was sometimes a case when there were no params but there was a body so the perform_request method would be called with the params arg explicitly set to nil. So once it gets to this line you get an NoMethodError: undefined method delete' for nil:NilClass` error.

I think this scenario is already possible before these changes:

$ client.perform_request('POST', '/', nil, '{"foo":"bar"}', '{"Content-Type":"application/x-ndjson"}') NoMethodError: undefined method `delete' for nil:NilClass from ..../elastic-transport-ruby/fork/elastic-transport-ruby/lib/elastic/transport/transport/base.rb:283:in `perform_request'

estolfo · 2023-09-21T11:11:11Z

Hi @picandocodigo the outstanding questions I had were:

Can you instantiate different OTel SDKs per process and if so, would there be a use case for passing the sdk into the client for use. This is a valid use case for Java but I confirmed with the OTel Ruby group that they have a single, global SDK.
Should we define a schema_url on the tracer? As of the time of this PR, you cannot. Here is the issue in their repo.

I have no other outstanding questions then that should block this PR :)

estolfo · 2023-09-25T11:55:38Z

Update on this since my last comment:
I found out that it's recommended to allow a OpenTelemetry TracerProvider be specified. So I added an option when creating a client opentelemetry_tracer_provider in this commit.

estolfo commented Jun 28, 2023

View reviewed changes

spec/spec_helper.rb Outdated Show resolved Hide resolved

estolfo mentioned this pull request Jun 28, 2023

OTel instrumentation: Add path patterns and endpoint in opts to perform_request elastic/elasticsearch-ruby#2131

Closed

estolfo commented Jun 28, 2023

View reviewed changes

spec/elastic/transport/opentelemetry_spec.rb Show resolved Hide resolved

estolfo commented Jun 29, 2023

View reviewed changes

spec/elastic/transport/opentelemetry_spec.rb Outdated Show resolved Hide resolved

estolfo force-pushed the otel-instrumentation branch from abebd1c to b393ff7 Compare July 3, 2023 10:12

estolfo marked this pull request as ready for review July 3, 2023 14:13

estolfo marked this pull request as draft August 23, 2023 15:34

estolfo mentioned this pull request Aug 25, 2023

[OTel instrumentation] Add path params and endpoint in opts to perform_request elastic/elasticsearch-ruby#2179

Merged

estolfo force-pushed the otel-instrumentation branch from d4ddf3c to 3dcd313 Compare September 6, 2023 13:58

estolfo marked this pull request as ready for review September 6, 2023 13:58

picandocodigo reviewed Sep 18, 2023

View reviewed changes

estolfo added 15 commits September 26, 2023 21:53

Add path_patterns arg to perform_request

c5217d0

Add otel tracer and create span

aa7b203

Add OpenTelemetry instrumentation

b8df168

Add more tests

1b7a4d1

Update Ruby version check

faae679

Only capture db.statement for search endpoints

0e1a7fb

Add sanitization with ENV config

0ed2a9f

Further refine sanitization

4cec224

Handle case when endpoint and path templates values are nil

f58a82b

Updates to tests

ca8dcfc

Add github action for testing with OTel

6e6922e

Use ENV variable to set hosts in new client

c8aa213

Use ENV variable for test port

128debe

otel span attr server.port should be int

39d5db3

Test when named capture has an underscore

2c090d5

estolfo added 11 commits September 26, 2023 21:53

Add ENV variable for enabling/disabling otel instrumentation

53f8b97

Use opts instead of method args

0812ce2

Pass set variables to perform_method instead

f1a0c0b

Convert array of path params to string

0b0b3e2

Minor update

05022db

Add test for when no endpoint or path params are provided

d1bfcc0

Add option to provide a tracer provider when creating a client

4bcbe8e

Fix typo

2cbc27d

db.system span attribute should always be elasticsearch

d653d91

Adjust names of environment variables

5470c77

Add some more tests for all body strategy options

779c83e

estolfo force-pushed the otel-instrumentation branch from f9fab0b to 779c83e Compare September 26, 2023 19:53

estolfo added 4 commits September 26, 2023 21:54

Remove extra line

a45f52a

Remove remnants of older approach

b04c310

Add note for Open Telemetry wrapper class

350e409

Add inline documentation

7958acc

picandocodigo merged commit a32b791 into elastic:main Sep 27, 2023

pquentin mentioned this pull request Feb 12, 2024

Add OpenTelemetry support elastic/elasticsearch-py#2435

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTel instrumentation#54

OTel instrumentation#54
picandocodigo merged 30 commits intoelastic:mainfrom
estolfo:otel-instrumentation

estolfo commented Jun 28, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

estolfo commented Jul 3, 2023

Uh oh!

picandocodigo commented Jul 3, 2023

Uh oh!

picandocodigo Sep 18, 2023

Uh oh!

estolfo Sep 21, 2023

Uh oh!

estolfo commented Sep 21, 2023 •

edited

Loading

Uh oh!

estolfo commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

estolfo commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

estolfo commented Jul 3, 2023

Uh oh!

picandocodigo commented Jul 3, 2023

Uh oh!

picandocodigo Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

estolfo Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

estolfo commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estolfo commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

estolfo commented Jun 28, 2023 •

edited

Loading

estolfo commented Sep 21, 2023 •

edited

Loading