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.

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

OTel instrumentation #54

Merged
merged 30 commits into from
Sep 27, 2023
Merged

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Jun 28, 2023

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 Outdated Show resolved Hide resolved
@estolfo
Copy link
Contributor Author

estolfo commented Jul 3, 2023

@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
Copy link
Member

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 👍

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

Choose a reason for hiding this comment

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

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?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

estolfo commented Sep 21, 2023

Hi @picandocodigo the outstanding questions I had were:

  1. 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.
  2. 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
Copy link
Contributor Author

estolfo commented Sep 25, 2023

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.

@picandocodigo picandocodigo merged commit a32b791 into elastic:main Sep 27, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants