OpenTelemetry support #104

JoshMock · 2024-06-18T22:33:47Z

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:

Add @opentelemetry/api and @opentelemetry/auto-instrumentations-node as Node.js dependencies
Add appropriate environment variable values for OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME

require the auto instrumentation registration package at run time:

node --require '@opentelemetry/auto-instrumentations-node/register' index.js

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 elastic/elasticsearch-js#2267

estolfo · 2024-06-20T10:25:24Z

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

Add test for OpenTelemetry error tracking

david-luna

the PR looks good. One minor issue I think we should discuss is there is a package which holds attribute names and values (if they are enums). The package is @opentelemetry/semantic-conventions.

I'm hesitant to request for changes since that package is in the process of a major update and may probably break this instrumentation if we use the package an try to update. Another option is tu use hardcoded strings for now and use the package when ready.

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

Thoughts?

src/Transport.ts

trentm

This is a first pass. I will look again a bit more this afternoon.

trentm · 2024-06-24T17:03:32Z

package.json

@@ -65,10 +66,15 @@
    "tslib": "^2.4.0",
    "undici": "^6.12.0"
  },
+  "peerDependencies": {
+    "@opentelemetry/api": "1.x",


Shouldn't there be an entry in "dependencies" for @opentelemetry/api, as it is imported/required by the using code?

I suppose using peerDeps typically works for modern npm, as peerDeps are installed by default (https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependencies). I say "typically" because some projects unfortunately use the legacy-peer-deps npm config option (https://docs.npmjs.com/cli/v10/using-npm/config#legacy-peer-deps) which results in peerDeps being ignored. The result, in this case, is an @elastic/transport that breaks. Here is a smallish repro of that breakage:

% cat package.json { "name": "asdf.20240624t094529", "version": "1.0.0", "dependencies": { "@elastic/transport": "git+https://github.com/elastic/elastic-transport-js.git#otel" } } % npm install --legacy-peer-deps ... % npm ls -a npm ERR! code ELSPROBLEMS npm ERR! invalid: @elastic/transport@8.6.0 /Users/trentm/tmp/asdf.20240624T094529/node_modules/@elastic/transport npm ERR! missing: @opentelemetry/api@1.x, required by @elastic/transport@8.6.0 npm ERR! missing: @opentelemetry/sdk-trace-base@1.x, required by @elastic/transport@8.6.0 asdf.20240624t094529@1.0.0 /Users/trentm/tmp/asdf.20240624T094529 └─┬ @elastic/transport@8.6.0 invalid: "git+https://github.com/elastic/elastic-transport-js.git#otel" from the root project ├── UNMET DEPENDENCY @opentelemetry/api@1.x ├── UNMET DEPENDENCY @opentelemetry/sdk-trace-base@1.x ├─┬ debug@4.3.5 │ └── ms@2.1.2 ├── hpagent@1.2.0 ├── ms@2.1.3 ├── secure-json-parse@2.7.0 ├── tslib@2.6.3 └── undici@6.19.2 ...

Here I manually copy in the built JS code from a git working copy in a separate dir:

% rsync -av ~/el/elastic-transport-js2/lib/ node_modules/@elastic/transport/lib/

Then importing the package fails:

% node Welcome to Node.js v18.18.2. Type ".help" for more information. > require('@elastic/transport') Uncaught Error: Cannot find module '@opentelemetry/api' Require stack: - /Users/trentm/tmp/asdf.20240624T094529/node_modules/@elastic/transport/lib/Transport.js - /Users/trentm/tmp/asdf.20240624T094529/node_modules/@elastic/transport/index.js - <repl> at Module._resolveFilename (node:internal/modules/cjs/loader:1077:15) at Module._load (node:internal/modules/cjs/loader:922:27) at Module.require (node:internal/modules/cjs/loader:1143:19) at require (node:internal/modules/cjs/helpers:119:18) { code: 'MODULE_NOT_FOUND', requireStack: [ '/Users/trentm/tmp/asdf.20240624T094529/node_modules/@elastic/transport/lib/Transport.js', '/Users/trentm/tmp/asdf.20240624T094529/node_modules/@elastic/transport/index.js', '<repl>' ] }

I don't think there is a significant downside in taking the direct dependency on @opentelemetry/api, is there?

Good call. I'd initially intended to put the SDK in as a peer dependency, but forgot to clean that up. Fixed in ec782d6.

package.json

src/Transport.ts

Co-authored-by: Trent Mick <trent.mick@elastic.co>

JoshMock · 2024-06-27T18:10:13Z

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 #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 · 2024-06-27T18:49:44Z

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

trentm

LGTM. Thanks!

Something to consider for later: configurability of the OTel instrumentation.
Similar to how the older diagnostics events can be configured via TransportOptions.diagnostic, there are a couple things that might be nice to be configurable:

A way to disable this OTel instrumentation. Users of an OTel SDK will be somewhat used to the ability to disable particular instrumentations. Usually this config var is called enabled, in the context of an instrumentation.
A way to suppress child HTTP spans that will be created under the Elasticsearch spans. There are a few existing OTel instrumentations that have this same situation, e.g.: instrumentation-aws-sdk, instrumentation-mongoose. Typically this config var is a boolean called suppressInternalInstrumentation. E.g. https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-sdk/README.md#aws-sdk-instrumentation-options

AFAIK there aren't any other Node.js libraries that have native OTel instrumentation like is being added here, so there isn't prior art on exactly what to name OTel-related config. Perhaps having:

export interface TransportOptions {
  opentelemetry?: {
    enabled?: boolean;
    suppressInternalInstrumentation?: boolean
  }

Should I create a separate issue for this?

JoshMock · 2024-06-28T14:29:15Z

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.

kirrg001 · 2024-08-15T09:50:54Z

src/Transport.ts

+        }
+      }
+
+      return await this[kOtelTracer].startActiveSpan(params.meta.name, { attributes, kind: SpanKind.CLIENT }, async (otelSpan: Span) => {


@JoshMock Hi there!

Why is elastic creating an otel span by default without Otel being activated/used? 🤔

I have not used --require '@opentelemetry/auto-instrumentations-node/register' but I can see while debugging that the otel spans are getting created.

Could you please explain to me why elastic is approaching this? Thanks so much!

According to the OpenTelemetry docs, the code will create no-op traces if the OpenTelemetry SDK has not been initialized. They'll still show up in the stack trace, but they won't go anywhere.

If you need to disable OTel collection for this process, you can set the environment variable OTEL_SDK_DISABLED=true.

trentm · 2024-08-15T22:31:07Z

I hadn't followed up on creating issues for the things I mentioned above in #104 (review)
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 · 2024-08-15T22:32:48Z

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: elastic/elasticsearch-js#2299

trentm · 2024-08-15T22:34:38Z

@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/

JoshMock requested review from pquentin, ezimuel and estolfo June 18, 2024 22:37

JoshMock mentioned this pull request Jun 18, 2024

Documentation for OpenTelemetry support elastic/elasticsearch-js#2289

Merged

JoshMock requested review from trentm and david-luna June 20, 2024 16:22

JoshMock added 3 commits June 20, 2024 14:56

OpenTelemetry support

e32f143

OpenTelemetry tests

46ceb23

Add test for OpenTelemetry error tracking

Set error.type on OpenTelemetry spans

e914c47

JoshMock force-pushed the otel branch from 83742d5 to e914c47 Compare June 20, 2024 19:56

david-luna reviewed Jun 21, 2024

View reviewed changes

src/Transport.ts Outdated Show resolved Hide resolved

trentm reviewed Jun 24, 2024

View reviewed changes

src/Transport.ts Outdated Show resolved Hide resolved

src/Transport.ts Outdated Show resolved Hide resolved

Provide client version when getting tracer

be182dd

Co-authored-by: Trent Mick <trent.mick@elastic.co>

JoshMock added 4 commits June 27, 2024 13:11

Represent OTel dependencies more accurately

ec782d6

Fetch OTel tracer at instantiation

96cb20f

Refactor _request into private method

1cae465

Collect OTel attributes before starting span

264d99e

JoshMock requested a review from trentm June 27, 2024 18:48

trentm approved these changes Jun 27, 2024

View reviewed changes

JoshMock merged commit 4f7b1ef into main Jun 28, 2024
11 checks passed

JoshMock deleted the otel branch June 28, 2024 14:32

trentm mentioned this pull request Jun 28, 2024

Configurability of OpenTelemetry instrumentation elastic/elasticsearch-js#2299

Open

david-luna mentioned this pull request Jul 2, 2024

feat: instrument @elastic/elasticsearch package elastic/elastic-otel-node#28

Open

elasticmachine mentioned this pull request Jul 25, 2024

Upgrade transport to 8.7.0 elastic/elasticsearch-serverless-js#80

Merged

kirrg001 reviewed Aug 15, 2024

View reviewed changes

unflxw mentioned this pull request Sep 6, 2024

Support Elasticsearch library appsignal/appsignal-nodejs#1092

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenTelemetry support #104

OpenTelemetry support #104

JoshMock commented Jun 18, 2024 •

edited

Loading

estolfo commented Jun 20, 2024

david-luna left a comment

trentm left a comment

trentm Jun 24, 2024

JoshMock Jun 27, 2024

JoshMock commented Jun 27, 2024

JoshMock commented Jun 27, 2024

trentm left a comment

JoshMock commented Jun 28, 2024

kirrg001 Aug 15, 2024

JoshMock Aug 15, 2024

trentm commented Aug 15, 2024

trentm commented Aug 15, 2024

trentm commented Aug 15, 2024

OpenTelemetry support #104

OpenTelemetry support #104

Conversation

JoshMock commented Jun 18, 2024 • edited Loading

estolfo commented Jun 20, 2024

david-luna left a comment

Choose a reason for hiding this comment

trentm left a comment

Choose a reason for hiding this comment

trentm Jun 24, 2024

Choose a reason for hiding this comment

JoshMock Jun 27, 2024

Choose a reason for hiding this comment

JoshMock commented Jun 27, 2024

JoshMock commented Jun 27, 2024

trentm left a comment

Choose a reason for hiding this comment

JoshMock commented Jun 28, 2024

kirrg001 Aug 15, 2024

Choose a reason for hiding this comment

JoshMock Aug 15, 2024

Choose a reason for hiding this comment

trentm commented Aug 15, 2024

trentm commented Aug 15, 2024

trentm commented Aug 15, 2024

JoshMock commented Jun 18, 2024 •

edited

Loading