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

feat: instrument @elastic/elasticsearch package #28

Open
david-luna opened this issue Jan 28, 2024 · 7 comments · May be fixed by #264
Open

feat: instrument @elastic/elasticsearch package #28

david-luna opened this issue Jan 28, 2024 · 7 comments · May be fixed by #264
Assignees

Comments

@david-luna
Copy link
Member

This instrumentation is not in any of the opentelemetry repos. That would be a good example for instrumentations we could provide earlier in our distro before promoting them to upstream.

@david-luna david-luna changed the title Instrument @elastic/elasticsearch package feat: instrument @elastic/elasticsearch package Jan 31, 2024
@trentm trentm added this to the alpha milestone Jan 31, 2024
@trentm trentm removed this from the alpha milestone Feb 1, 2024
@trentm
Copy link
Member

trentm commented Feb 5, 2024

Note to self: check with clients team first

@trentm
Copy link
Member

trentm commented Mar 27, 2024

@david-luna
Copy link
Member Author

IIUC this PR adds OTEL instrumentation natively in elasticsearch client rigtht? If so we should add only instrumentation for elasticsearch's versions using a @elastic/transport 8.6.0 or below which is @elastic/elasticsearch <=8.15

@trentm
Copy link
Member

trentm commented Jul 2, 2024

We should add a test/isntr-elasticsearch.test.js to sanity check that it works. (I tried locally and it does not.) I got this trace:

------ trace 6ed383 (1 span) ------
       span 7ce6ce "tcp.connect" (7.8ms, SPAN_KIND_INTERNAL)
------ trace ee3359 (2 spans) ------
       span eb3d59 "GET" (3.0ms, STATUS_CODE_ERROR, SPAN_KIND_CLIENT, GET http://metadata.google.internal./computeMetadata/v1/instance)
  +0ms `- span b2ce11 "tcp.connect" (2.7ms, STATUS_CODE_ERROR, SPAN_KIND_INTERNAL)
------ trace ede1e1 (2 spans) ------
       span 52dbcc "GET" (3003.2ms, STATUS_CODE_ERROR, SPAN_KIND_CLIENT, GET http://169.254.169.254/computeMetadata/v1/instance)
  +0ms `- span 83b5f5 "tcp.connect" (3002.5ms, SPAN_KIND_INTERNAL)
------ trace d451bb (1 span) ------
       span f65260 "GET" (21.4ms, SPAN_KIND_CLIENT, GET http://localhost:9200/_search?q=pants -> 200)

using:

node -r @elastic/opentelemetry-node trace-elasticsearch8.js

@david-luna david-luna self-assigned this Jul 3, 2024
@trentm
Copy link
Member

trentm commented Jul 4, 2024

@david-luna I could steal this one if you like.

I had a start at adding instr-elasticsearch.test.js locally.
I know why it isn't working yet: It is because @elastic/[email protected] is only part of the requirement for OTel native instr working. It also requires @elastic/[email protected], which includes a "meta" object with ES client request details passed down to @elastic/transport. That was added in https://github.com/elastic/elastic-client-generator-js/pull/42, but needs a new @elastic/elasticsearch release.

(Note to self: g eon3)

@trentm trentm linked a pull request Jul 4, 2024 that will close this issue
1 task
@trentm
Copy link
Member

trentm commented Jul 4, 2024

I had a start at adding instr-elasticsearch.test.js locally.

I have a draft PR to add a test here: #264
It is waiting on an es client 8.15.0 release first.

@trentm trentm assigned trentm and unassigned david-luna Jul 4, 2024
@david-luna
Copy link
Member Author

@trentm ill be happy to review your PR :)

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 a pull request may close this issue.

2 participants