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

Add instrumentation for OpenSearch #2228

Closed
Earlopain opened this issue Sep 20, 2023 · 13 comments · Fixed by #2796
Closed

Add instrumentation for OpenSearch #2228

Earlopain opened this issue Sep 20, 2023 · 13 comments · Fixed by #2796
Assignees
Labels
3 Story Point Estimate community To tag external issues and PRs submitted by the community feature request To tag feature request after Hero Triage for PM to disposition hacktoberfest oct-dec qtr Possible FY Q3 candidate

Comments

@Earlopain
Copy link
Contributor

Feature Description

OpenSearch is a fork of Elasticsearch spearheaded by Amazon. Elasticsearch instrumentation was added with #1469. Since this is a fork, much of the work necessary will be highly similar to what is currently present, for example the perform_request method is found in OpenSearch::Transport::Client.

Describe Alternatives

None

Priority

We are currently in the process of migrating to OpenSearch and loosing instrumentation is the only downside. Of course the instrumentation for Elasticsearch was added rather recently and we have been doing fine before that, so I'll put this as "Really Want".

@Earlopain Earlopain added the feature request To tag feature request after Hero Triage for PM to disposition label Sep 20, 2023
@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Sep 20, 2023
@hannahramadan hannahramadan added the estimate Issue needing estimation label Sep 20, 2023
@hannahramadan
Copy link
Contributor

Hi @Earlopain - thank you for this feature request! The maintainers will discuss adding instrumentation for OpenSearch and keep you posted here.

Please feel free to direct other OpenSearch users here to lend their support (via emoji and/or comments) for the feature.

@kford-newrelic kford-newrelic added 3 Story Point Estimate jan-mar qtr Possible FY Q4 candidate and removed estimate Issue needing estimation labels Sep 26, 2023
@hannahramadan
Copy link
Contributor

Hi @Earlopain - We discussed this one as a team and have added OpenSearch instrumentation to our roadmap, with plans to complete it between January and March 2024. We will update you when OpenSearch instrumentation becomes available. Thanks again for this feature request!

@Earlopain
Copy link
Contributor Author

That's awesome, much appreciated for the update (:

@workato-integration
Copy link

@kford-newrelic kford-newrelic removed the jan-mar qtr Possible FY Q4 candidate label Jan 10, 2024
@kford-newrelic kford-newrelic added the apr-jun qtr Represents proposed work item for the Apr-Jun quarter label Feb 8, 2024
@newrelic newrelic deleted a comment from workato-integration bot Mar 7, 2024
@kford-newrelic kford-newrelic added jul-sep qtr Represents proposed work item for the Jul-Sep quarter and removed apr-jun qtr Represents proposed work item for the Apr-Jun quarter labels Mar 13, 2024
@hannahramadan
Copy link
Contributor

Hi again @Earlopain. An update here—our team had to shift some priorities around and unfortunately we aren't able to complete OpenSearch instrumentation within the originally planned timeframe. That said, it is still on our roadmap for this year and we will contiue to keep you in the loop!

@Earlopain
Copy link
Contributor Author

Time really does fly. Thank you for that update, I appreciate it 👍

@praveen-ks
Copy link

praveen-ks commented Jul 8, 2024

Hi @hannahramadan

We are currently in the process of migrating to OpenSearch, but the lack of instrumentation is causing some delays.

Could you please provide an estimated timeline for when the OpenSearch instrumentation will be added? Additionally, if there are any workarounds available in the meantime, that would be very helpful.

@tannalynn
Copy link
Contributor

@praveen-ks
We aren't currently working on this due to other priorities, but it's something we tentatively hope to address sometime this year.

As for workarounds, the main workaround for a situation where a gem lacks instrumentation is to add custom instrumentation using the API. The most useful one to start with would probably be NewRelic::Agent::Tracer.start_datastore_segment. For reference, this is how we create the segment for our elasticsearch instrumentation, in case that helps!

We will update this issue in the future when we add the instrumentation.
Hopefully this helps.

@praveen-ks
Copy link

praveen-ks commented Aug 3, 2024

@praveen-ks We aren't currently working on this due to other priorities, but it's something we tentatively hope to address sometime this year.

As for workarounds, the main workaround for a situation where a gem lacks instrumentation is to add custom instrumentation using the API. The most useful one to start with would probably be NewRelic::Agent::Tracer.start_datastore_segment. For reference, this is how we create the segment for our elasticsearch instrumentation, in case that helps!

We will update this issue in the future when we add the instrumentation. Hopefully this helps.

Thanks @tannalynn for the references.

I have come up with below code to make newrelic work opensearch, let me know if I am missing anything.

NEWRELIC_OPENSEARCH_CAPTURE_QUERIES = true
NEWRELIC_OPENSEARCH_OBFUSCATE_STATEMENT = true
OPENSEARCH_RUBY_API_FILE_PATH = "lib/opensearch/api"

module NewRelic::Agent::Instrumentation
  module OpenSearch
    PRODUCT_NAME = 'Elasticsearch'
    OPERATION = 'perform_request'

    def perform_request_with_tracing(method, path, params = {}, body = nil, headers = nil)
      return yield unless NewRelic::Agent::Tracer.tracing_enabled?

      segment = NewRelic::Agent::Tracer.start_datastore_segment(
        product: PRODUCT_NAME,
        operation: nr_operation || OPERATION,
        host: nr_hosts[:host],
        port_path_or_id: path,
        database_name: nr_cluster_name
      )
      begin
        NewRelic::Agent::Tracer.capture_segment_error(segment) { yield }
      ensure
        if segment
          segment.notice_nosql_statement(nr_reported_query(body || params))
          segment.finish
        end
      end
    end

    private

    def nr_operation
      operation_index = caller_locations.index do |line|
        string = line.to_s
        string.include?(OPENSEARCH_RUBY_API_FILE_PATH) && !string.include?(OPERATION)
      end
      return nil unless operation_index

      caller_locations[operation_index].to_s.split('`')[-1].gsub(/\W/, "")
    end

    def nr_reported_query(query)
      return unless NEWRELIC_OPENSEARCH_CAPTURE_QUERIES
      return query unless NEWRELIC_OPENSEARCH_OBFUSCATE_STATEMENT

      NewRelic::Agent::Datastores::NosqlObfuscator.obfuscate_statement(query)
    end

    def nr_cluster_name
      return @nr_cluster_name if @nr_cluster_name
      return if nr_hosts.empty?

      NewRelic::Agent.disable_all_tracing do
        @nr_cluster_name ||= perform_request('GET', '_cluster/health').body["cluster_name"]
      end
    rescue StandardError => e
      NewRelic::Agent.logger.error("Failed to get cluster name for opensearch", e)
      nil
    end

    def nr_hosts
      @nr_hosts ||= (transport.hosts.first || NewRelic::EMPTY_HASH)
    end
  end
end

module NewRelic::Agent::Instrumentation
  module OpenSearch::Prepend
    include NewRelic::Agent::Instrumentation::OpenSearch

    def perform_request(*args)
      perform_request_with_tracing(*args) { super }
    end
  end
end


OpenSearch::Transport::Client.prepend NewRelic::Agent::Instrumentation::OpenSearch::Prepend

@kaylareopelle
Copy link
Contributor

Hi @praveen-ks, thanks for putting this together! What you have looks good! There are two things I'd recommend changing.

  1. PRODUCT_NAME should be "OpenSearch" instead of "Elasticsearch"
  2. In perform_request_with_tracing, when the segment is started, the port_path_or_id should point to nr_hosts[:port] instead of port (I hit an error running this code without this change)

We updated the Elasticsearch instrumentation recently to have two improvements that I don't see in your OpenSearch example:

  1. The operation name extraction was updated for compatibility with Ruby 3.4
  2. The nr_cluster_name is more performant by requesting a different endpoint and updating retry logic.

Here's the latest version of the Elasticsearch instrumentation if you're interested in the details: https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/new_relic/agent/instrumentation/elasticsearch/instrumentation.rb

You also inspired me to pull together the instrumentation to make things more dynamic. If you'd like to test out the WIP instrumentation, you can do so by installing the agent from the opensearch-instrumentation branch.

gem 'newrelic_rpm', github: 'newrelic/newrelic-ruby-agent', branch: 'opensearch-instrumentation'

The code is still in process at #2796. My remaining known TODOs are related to Docker configuration in the development and CI environments.

If you decide to test it out, please let us know how it goes! We'd love to know how we can improve the telemetry for OpenSearch to make it more valuable.

@praveen-ks
Copy link

praveen-ks commented Aug 7, 2024

Hi @praveen-ks, thanks for putting this together! What you have looks good! There are two things I'd recommend changing.

  1. PRODUCT_NAME should be "OpenSearch" instead of "Elasticsearch"
  2. In perform_request_with_tracing, when the segment is started, the port_path_or_id should point to nr_hosts[:port] instead of port (I hit an error running this code without this change)

We updated the Elasticsearch instrumentation recently to have two improvements that I don't see in your OpenSearch example:

  1. The operation name extraction was updated for compatibility with Ruby 3.4
  2. The nr_cluster_name is more performant by requesting a different endpoint and updating retry logic.

Here's the latest version of the Elasticsearch instrumentation if you're interested in the details: https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/new_relic/agent/instrumentation/elasticsearch/instrumentation.rb

You also inspired me to pull together the instrumentation to make things more dynamic. If you'd like to test out the WIP instrumentation, you can do so by installing the agent from the opensearch-instrumentation branch.

gem 'newrelic_rpm', github: 'newrelic/newrelic-ruby-agent', branch: 'opensearch-instrumentation'

The code is still in process at #2796. My remaining known TODOs are related to Docker configuration in the development and CI environments.

If you decide to test it out, please let us know how it goes! We'd love to know how we can improve the telemetry for OpenSearch to make it more valuable.

@kaylareopelle Thanks for the inputs 👍

I won't be able to test it because I am currently using gem version 8.16.0 and don't have enough bandwidth for upgrading it to latest version for my application. It's planned in coming months, I can test it that time only.

@praveen-ks
Copy link

Hi @praveen-ks, thanks for putting this together! What you have looks good! There are two things I'd recommend changing.

  1. PRODUCT_NAME should be "OpenSearch" instead of "Elasticsearch"
  2. In perform_request_with_tracing, when the segment is started, the port_path_or_id should point to nr_hosts[:port] instead of port (I hit an error running this code without this change)

We updated the Elasticsearch instrumentation recently to have two improvements that I don't see in your OpenSearch example:

  1. The operation name extraction was updated for compatibility with Ruby 3.4
  2. The nr_cluster_name is more performant by requesting a different endpoint and updating retry logic.

Here's the latest version of the Elasticsearch instrumentation if you're interested in the details: https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/new_relic/agent/instrumentation/elasticsearch/instrumentation.rb

You also inspired me to pull together the instrumentation to make things more dynamic. If you'd like to test out the WIP instrumentation, you can do so by installing the agent from the opensearch-instrumentation branch.

gem 'newrelic_rpm', github: 'newrelic/newrelic-ruby-agent', branch: 'opensearch-instrumentation'

The code is still in process at #2796. My remaining known TODOs are related to Docker configuration in the development and CI environments.

If you decide to test it out, please let us know how it goes! We'd love to know how we can improve the telemetry for OpenSearch to make it more valuable.

@kaylareopelle I am using newrelic_ruby_agent version8.16.0 and Ruby 3.2, Do I still need to make changes except PRODUCT_NAME change?

Because I have tested the above version code and was working fine. Just don't want more iterations.

@kaylareopelle
Copy link
Contributor

@praveen-ks - No problem! Thanks for the update!

If the code was working fine on your machine, then yes, the only change would be to PRODUCT_NAME. 🙂

@github-project-automation github-project-automation bot moved this from In Sprint to Code Complete/Done in Ruby Engineering Board Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 Story Point Estimate community To tag external issues and PRs submitted by the community feature request To tag feature request after Hero Triage for PM to disposition hacktoberfest oct-dec qtr Possible FY Q3 candidate
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants