-
Notifications
You must be signed in to change notification settings - Fork 369
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
OpenSearch Integration #2940
OpenSearch Integration #2940
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2940 +/- ##
========================================
Coverage 97.97% 97.97%
========================================
Files 1287 1295 +8
Lines 71207 71564 +357
Branches 3284 3299 +15
========================================
+ Hits 69764 70117 +353
- Misses 1443 1447 +4
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is shaping up very nicely!
The only significant change I see so far that's an issue is the dependence upon Elasticsearch. We should be able to instrument OpenSearch without importing the instrumentation for Elasticsearch. See my comments about our options.
40c18f9
to
3e80a38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, @sarahchen6! 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions, but otherwise looks great. Only thing I need to double check is the use of SpanAttributeSchema
.... although this should happen soon, I didn't think this was yet merged to master
. Want to check with @zarirhamza on this.
|
||
option :service_name do |o| | ||
o.default do | ||
Contrib::SpanAttributeSchema.fetch_service_name( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to see this in here. My understanding is this (SpanAttributeSchema
) was still on a feature branch? Or is this really in master
? cc @zarirhamza
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering my question, this looks like it was a prototype of sorts merged months ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
OpenSearch Integration - Technical Specification.pdf
What does this PR do?
This PR implements the OpenSearch integration with dd-trace-rb code by creating and populating two directories:
/lib/datadog/tracing/contrib/opensearch/
and/spec/datadog/tracing/contrib/opensearch/
.Motivation
OpenSearch has had over 100 million downloads since its fork from Elasticsearch in 2021. It is a widely used project with roughly 500 contributors. Thus, providing OpenSearch integration would benefit a substantial number of users and expand Datadog’s tracing client to offer more valuable trace requests and development visibility.
fixes #2628
Additional Notes
Under Github's checks, there are currently still failures with the Library Injection code.
How to test the change?
This code is covered by unit tests located in the
/spec/datadog/tracing/contrib/opensearch/
directory. To test the change on your machine, rundocker compose run --rm tracer-3.1 /bin/bash
in your terminal to enter the docker environment. Once inside, runbundle exec appraisal ruby-3.1.2-contrib rspec spec/datadog/tracing/contrib/opensearch/patcher_spec.rb
to test the patcher code. Runbundle exec appraisal ruby-3.1.2-contrib rspec spec/datadog/tracing/contrib/opensearch/quantize_spec.rb
to test the quantization code.This PR has also been tested by CircleCI.