Migrate from individual OTel packages to EDOT#226609
Conversation
afharo
left a comment
There was a problem hiding this comment.
Highlight the fixes to get @elastic/opentelemetry-node/sdk to work in Kibana
| if (req.startsWith('@elastic/opentelemetry-node/sdk')) { | ||
| return Path.resolve(REPO_ROOT, `node_modules/@elastic/opentelemetry-node/lib/sdk.js`); | ||
| } |
There was a problem hiding this comment.
This fixes the runtime imports (including ESLint).
There was a problem hiding this comment.
we can likely add a note here so we don't forget we can remove this once the cjs/esm interop is in
There was a problem hiding this comment.
Can we create an issue and reference it here? That way we can track the technical debt and possibly trigger someone to fix it.
| // Allows for importing from `kibana` package for the exported types. | ||
| "@emotion/core": ["typings/@emotion"] | ||
| "@emotion/core": ["typings/@emotion"], | ||
| "@elastic/opentelemetry-node/sdk": ["typings/@elastic/opentelemetry-node/sdk"], |
There was a problem hiding this comment.
Tell TS where to look for the types of this package.
There was a problem hiding this comment.
we can likely add a note here as well so we don't forget to remove this in the future
mistic
left a comment
There was a problem hiding this comment.
the changes to support @elastic/opentelemetry-node/sdk LGTM
| if (req.startsWith('@elastic/opentelemetry-node/sdk')) { | ||
| return Path.resolve(REPO_ROOT, `node_modules/@elastic/opentelemetry-node/lib/sdk.js`); | ||
| } |
There was a problem hiding this comment.
we can likely add a note here so we don't forget we can remove this once the cjs/esm interop is in
| // Allows for importing from `kibana` package for the exported types. | ||
| "@emotion/core": ["typings/@emotion"] | ||
| "@emotion/core": ["typings/@emotion"], | ||
| "@elastic/opentelemetry-node/sdk": ["typings/@elastic/opentelemetry-node/sdk"], |
There was a problem hiding this comment.
we can likely add a note here as well so we don't forget to remove this in the future
| import { diag, DiagLogger, DiagLogLevel } from '@opentelemetry/api'; | ||
| import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; | ||
| import { ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions'; | ||
| // import { ATTR_SERVICE_INSTANCE_ID } from '@opentelemetry/semantic-conventions/incubating'; |
There was a problem hiding this comment.
iirc, the OTel docs actually mention you should copy them over (while they are in development) and not import directly
| "@opentelemetry/api": "^1.9.0", | ||
| "@opentelemetry/api-metrics": "^0.31.0", | ||
| "@opentelemetry/context-async-hooks": "^2.0.0", | ||
| "@opentelemetry/core": "^2.0.0", |
There was a problem hiding this comment.
These APIs are env-agnostic I think - in the sense that you should be able to use them in both the browser and node.js. How do we plan to handle this if we fully delegate to @elastic/opentelemetry-node which is only for node.js?
There was a problem hiding this comment.
That's a good question.
AFAIK, we're focusing on the server for now.
I would expect that there will be an @elastic/opentelemetry-web/sdk or something like that.
If not, we can install the vanilla packages again.
There was a problem hiding this comment.
I would expect that there will be an @elastic/opentelemetry-web/sdk or something like that.
I'm not sure. In general I'm not sure if we should hide OpenTelemetry APIs behind an elastic distrib. @trentm what are the advantages here? It just feels off to use re-exported OpenTelemetry types where we also have less control about versioning.
There was a problem hiding this comment.
What benefit do we get from maintaining 12 different packages (17 really, but I only managed to remove 12 because we needed the other 5)? Especially with the current state of maturity of OTel.
Our renovate PR didn't manage to identify that @opentelemetry/api-metrics (link), or @opentelemetry/sdk-metrics-base (link) were deprecated 3 years ago!
And don't get me started on potentially incompatibilities across versions. I'd rather push that burden to the owners of EDOT.
I can also see the benefit of "dog-fooding" our products to identify opportunities to improve them: this PR highlights the bad DX of having to move from direct imports of types to namespaced imports: import { MeterProvider } becomes import { metrics } + metrics.MeterProvider 😢
IMO, this is a bad DX, and it's good that we figured it out first.
But I'd love to hear @trentm's thoughts.
There was a problem hiding this comment.
We don't currently have a "web SDK" (i.e. an OTel SDK targetting browsers).
The state of a Web SDK in vanilla OTel JS is also much less mature that the packages for Node.js. See https://opentelemetry.io/docs/languages/js/getting-started/browser/ for a start, but one needs to learn more about the details to use it effectively. @david-luna and I can try to help where needed.
These APIs are env-agnostic I think
Dario, I'm not sure if you were talking about all three of those deps:
- "@opentelemetry/api-metrics": "^0.31.0",
- "@opentelemetry/context-async-hooks": "^2.0.0",
- "@opentelemetry/core": "^2.0.0",
api-metricsis gone now. It was merged into@opentelemetry/api. It being a dep in Kibana's package.json shows that this metrics usage was a quite old usage from before the Metrics parts of@opentelemetry/apiwere stabilized.context-async-hooksis only relevant to Node.js, so you shouldn't need to this dep at all.@elastic/opentelemetry-nodewill setup the appropriate ContextManager for Node.js.core: The only usage of that in kibana.git that I see is:
x-pack/platform/packages/shared/kbn-inference-tracing/src/create_inference_active_span.ts
8:import { isTracingSuppressed } from '@opentelemetry/core';
Is createActiveInferenceSpan() used in Kibana browser code? If so, having a separate dep on @opentelemetry/core will be necessary -- and fine to have alongside @elastic/opentelemetry-node.
If just in Node.js code, the core module is re-exported by @elastic/opentelemetry-node/sdk:
> var edot = require('@elastic/opentelemetry-node/sdk')
> edot.core.isTracingSuppressed
[Function: isTracingSuppressed]
There was a problem hiding this comment.
Thank you @trentm for such informative comments.
Is there a problem with this:
import {metrics} from '@elastic/opentelemetry-node';
// ...
const myReader = new metrics.MetricReader(...)
or this:import {metrics} from '@elastic/opentelemetry-node';
const {MetricReader} = metrics;
// ...
const myReader = new MetricReader(...)
other than a single line import {MetricReader} from 'something' looks nicer?
That makes it nicer (I'll adapt the PR to minimize the number of lines changed). However, the DX is still suboptimal: typically your IDE helps with the imports. This package forces you to intentionally type it because the IDE is never going to find the MetricReader that you're after.
Also, I wonder if this affects performance as it may be worse on tree-shaking: I might be completely wrong here but AFAIK the import in the example below is a noop at runtime and the compiler removes it:
import { MetricReader } from '...';
function doSomething(reader: MetricReader) {}However, I'm not that sure about importing a parent variable metrics (in both flavors that you shared above). I'll need to test to see if the generated compiled code removes it or not.
There was a problem hiding this comment.
@trentm, actually, we need to differentiate between runtime code and types 😢:
import { tracing } from '@elastic/opentelemetry-node/sdk';
const { TraceIdRatioBasedSampler } = tracing;
type SpanProcessor = tracing.SpanProcessor;There was a problem hiding this comment.
What benefit do we get from maintaining 12 different packages (17 really, but I only managed to remove 12 because we needed the other 5)? Especially with the current state of maturity of OTel.
We don't, but i think we'll end up using some of the packages directly anyway. It just feels off to me, but not a blocker for me in any case.
Dario, I'm not sure if you were talking about all three of those deps:
I'm talking about @opentelemetry/api and @opentelemetry/core. E.g., I have helpers that create spans. I want to be able to use that code in the browser and in Node.js. How can I do that if we are expected to go through @elastic/opentelemetry-node and a hypothetical @elastic/opentelemetry-web lib? We would need @elastic/opentelemetry to be created I guess. And we'd have to do that for every instrumentation package we want to use.
There was a problem hiding this comment.
@dgieselaar, I agree with you that this makes it harder to make agnostic code.
TBH, after trying to "simulate" some of the changes in #224933 with this set of packages, I think that creating spans or metrics should be used via imports from @opentelemetry/api (which is a peer dependency of @elastic/opentelemetry-node/sdk, meaning that we'll have to install it separately anyway)
IMO, it's better to do import { metrics } from '@opentelemetry/api'; than import { api } from '@elastic/opentelemetry-node/sdk'; + use api.metrics.
On top of that, given that OTel has this weird requirement about every component in the code having to know the name of the meter/tracer, we'll likely end up with some sort of utils package that exposes the default meter/tracer (the output of metrics.getMeter('kibana')).
So I'd expect that the EDOT imports are reduced to the places where we're configuring the SDK (setting up the providers and exporters), but not the actual code instrumentation.
Does this make sense?
There was a problem hiding this comment.
I'm talking about
@opentelemetry/apiand@opentelemetry/core. E.g., I have helpers that create spans. I want to be able to use that code in the browser and in Node.js. How can I do that if we are expected to go through@elastic/opentelemetry-nodeand a hypothetical@elastic/opentelemetry-weblib?
My understanding of OTel is that separately importing @opentelemetry/api and using it is intentional and a good thing.
That leaves @opentelemetry/core. I think it would be fine for Kibana to have both @elastic/opentelemetry-node and @opentelemetry/core as deps. However, yes it means that you likely want to watch that the version of both are compatible versions so that the expected functionality works. And possibly want to sure that the same exact version is installed to reduce bundle sizes (if that is a concern).
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit cf6458d) # Conflicts: # package.json # src/platform/packages/private/kbn-import-resolver/src/import_resolver.ts # src/platform/packages/shared/kbn-tracing/src/init_tracing.ts # src/platform/packages/shared/kbn-tracing/src/late_binding_span_processor.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/base_inference_span_processor.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/create_inference_active_span.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/langfuse/langfuse_span_processor.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/phoenix/get_chat_span.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/phoenix/get_execute_tool_span.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/phoenix/phoenix_span_processor.ts # x-pack/platform/plugins/private/monitoring_collection/server/plugin.ts # yarn.lock
(cherry picked from commit cf6458d) # Conflicts: # package.json # src/platform/packages/private/kbn-import-resolver/src/import_resolver.ts
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit cf6458d) # Conflicts: # package.json # src/platform/packages/private/kbn-import-resolver/src/import_resolver.ts # src/platform/packages/shared/kbn-tracing/src/init_tracing.ts # src/platform/packages/shared/kbn-tracing/src/late_binding_span_processor.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/base_inference_span_processor.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/create_inference_active_span.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/langfuse/langfuse_span_processor.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/phoenix/get_chat_span.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/phoenix/get_execute_tool_span.ts # x-pack/platform/packages/shared/kbn-inference-tracing/src/phoenix/phoenix_span_processor.ts # x-pack/platform/plugins/private/monitoring_collection/server/plugin.ts # yarn.lock
# Backport This will backport the following commits from `main` to `9.1`: - [Migrate from individual OTel packages to EDOT (#226609)](#226609) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Alejandro Fernández Haro","email":"alejandro.haro@elastic.co"},"sourceCommit":{"committedDate":"2025-07-16T11:32:37Z","message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:version","v8.19.0","v9.2.0","v8.18.4","v9.0.5","v9.1.1"],"title":"Migrate from individual OTel packages to EDOT","number":226609,"url":"https://github.com/elastic/kibana/pull/226609","mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},"sourceBranch":"main","suggestedTargetBranches":["8.19","8.18","9.0","9.1"],"targetPullRequestStates":[{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/226609","number":226609,"mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},{"branch":"8.18","label":"v8.18.4","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.0","label":"v9.0.5","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.1","label":"v9.1.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Alejandro Fernández Haro <alejandro.haro@elastic.co>
# Backport This will backport the following commits from `main` to `8.19`: - [Migrate from individual OTel packages to EDOT (#226609)](#226609) <!--- Backport version: 10.0.1 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Alejandro Fernández Haro","email":"alejandro.haro@elastic.co"},"sourceCommit":{"committedDate":"2025-07-16T11:32:37Z","message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:version","v8.19.0","v9.2.0","v8.18.4","v9.0.5","v9.1.1"],"title":"Migrate from individual OTel packages to EDOT","number":226609,"url":"https://github.com/elastic/kibana/pull/226609","mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},"sourceBranch":"main","suggestedTargetBranches":["8.19","8.18","9.0"],"targetPullRequestStates":[{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/226609","number":226609,"mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},{"branch":"8.18","label":"v8.18.4","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.0","label":"v9.0.5","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.1","label":"v9.1.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/228861","number":228861,"state":"OPEN"}]}] BACKPORT--> Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
# Backport This will backport the following commits from `main` to `9.0`: - [Migrate from individual OTel packages to EDOT (#226609)](#226609) <!--- Backport version: 10.0.1 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Alejandro Fernández Haro","email":"alejandro.haro@elastic.co"},"sourceCommit":{"committedDate":"2025-07-16T11:32:37Z","message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:version","v8.19.0","v9.2.0","v8.18.4","v9.0.5","v9.1.1"],"title":"Migrate from individual OTel packages to EDOT","number":226609,"url":"https://github.com/elastic/kibana/pull/226609","mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},"sourceBranch":"main","suggestedTargetBranches":["8.19","8.18","9.0"],"targetPullRequestStates":[{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/226609","number":226609,"mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},{"branch":"8.18","label":"v8.18.4","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.0","label":"v9.0.5","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.1","label":"v9.1.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/228861","number":228861,"state":"OPEN"}]}] BACKPORT--> --------- Co-authored-by: Tiago Costa <tiago.costa@elastic.co>
# Backport This will backport the following commits from `main` to `8.18`: - [Migrate from individual OTel packages to EDOT (#226609)](#226609) <!--- Backport version: 10.0.1 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Alejandro Fernández Haro","email":"alejandro.haro@elastic.co"},"sourceCommit":{"committedDate":"2025-07-16T11:32:37Z","message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:version","v8.19.0","v9.2.0","v8.18.4","v9.0.5","v9.1.1"],"title":"Migrate from individual OTel packages to EDOT","number":226609,"url":"https://github.com/elastic/kibana/pull/226609","mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},"sourceBranch":"main","suggestedTargetBranches":["8.19","8.18","9.0"],"targetPullRequestStates":[{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/226609","number":226609,"mergeCommit":{"message":"Migrate from individual OTel packages to EDOT (#226609)","sha":"cf6458db470d0ae1f130d4cf6e408a2ec421ad26"}},{"branch":"8.18","label":"v8.18.4","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.0","label":"v9.0.5","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"9.1","label":"v9.1.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/228861","number":228861,"state":"OPEN"}]}] BACKPORT-->
Summary
Migrating as much as we can from the myriad of
@opentelemetry/*packages to@elastic/opentelemetry-node/sdk.We still need some individual packages installed for very specific things.
Checklist
backport:*labels.