elastic / elastic-transport-js

Transport classes and utilities shared among Node.js Elastic client libraries
Apache License 2.0
5 stars 25 forks source link

OpenTelemetry support #104

Closed JoshMock closed 3 months ago

JoshMock commented 3 months ago

Adds automatic instrumentation for OpenTelemetry, tracking the lifecycle of each Elasticsearch request. Follows all documented semantic conventions for Elasticsearch, excluding db.query.text, which we do not have a simple way to sanitize.

If someone wants to start shipping request spans to an OpenTelemetry endpoint without making any code changes, they must:

Full documentation will be included in a separate PR to elastic/elasticsearch-js soon.

This change will depend on an improvement to elastic/elasticsearch-js to include an optional meta object when calling transport.request(...), which will include both the endpoint name (db.operation.name) and any dynamic values in the path (db.elasticsearch.path_parts.<key>).

See https://github.com/elastic/elasticsearch-js/issues/2267

estolfo commented 3 months ago

I would suggest also asking @david-luna and @trentm for a review.

JoshMock commented 3 months ago

In both cases we I think wee need a tracking issue to action when the new version of the semantic conventions package is ready.

I've opened https://github.com/elastic/elastic-transport-js/issues/108 and assigned it to myself to remind me to circle back to this later. Feel free to add any useful context to that issue if I missed anything.

JoshMock commented 3 months ago

@trentm thanks so much for all the useful feedback! I'd never used the OTel API before so your expertise is much appreciated. Please take another look and let me know what you think.

JoshMock commented 3 months ago

Should I create a separate issue for this?

That would be fantastic, thank you! As much context and prior art you can provide would be super helpful for me. This PR was definitely meant as a "bare minimum" OTel implementation to have something ready in time for 8.15, so there is plenty of opportunity to make enhancements down the road.

trentm commented 1 month ago

I hadn't followed up on creating issues for the things I mentioned above in https://github.com/elastic/elastic-transport-js/pull/104#pullrequestreview-2146618924 I'll do that now.

@kirrg001 Having a disable option in the Elasticsearch client config would allow a way to disable Elasticsearch OTel instrumentation without having to fully disable the SDK, or add a SpanProcess or something to handle dropping those spans if they are undesired.

trentm commented 1 month ago

I hadn't followed up on creating issues for the things I mentioned above in #104 (review) I'll do that now.

I take that back. I had followed up: the feature request issue is here: https://github.com/elastic/elasticsearch-js/issues/2299

trentm commented 1 month ago

@kirrg001 The argument for having OTel instrumentation directly in a given library (so-called "native" instrumentation) is in the top-section here: https://opentelemetry.io/docs/concepts/instrumentation/libraries/