-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Migrate from individual OTel packages to EDOT #226609
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
Changes from all commits
692bb49
3d94c59
8eea9c5
d094ef8
c26dc9d
7499887
df45e7a
ce7353b
41a7b80
95df9ed
8707894
944b130
7ce45b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,7 +178,7 @@ | |
| "@aws-crypto/sha256-js", | ||
| "@aws-crypto/util", | ||
| "@langtrase/trace-attributes", | ||
| "@opentelemetry/sdk-trace-base", | ||
| "@arizeai/openinference-semantic-conventions", | ||
| "@smithy/eventstream-serde-node", | ||
| "@smithy/node-http-handler", | ||
| "@smithy/types", | ||
|
|
@@ -3370,7 +3370,7 @@ | |
| ], | ||
| "minimumReleaseAge": "7 days", | ||
| "enabled": true | ||
| }, | ||
| }, | ||
| { | ||
| "groupName": "redux-actions", | ||
| "matchDepNames": [ | ||
|
|
@@ -4346,36 +4346,33 @@ | |
| "groupName": "OpenTelemetry modules", | ||
| "matchDepNames": [ | ||
| "@grpc/grpc-js", | ||
| "@elastic/opentelemetry-node", | ||
| "@opentelemetry/api", | ||
| "@opentelemetry/api-metrics", | ||
| "@opentelemetry/core", | ||
| "@opentelemetry/context-async-hooks", | ||
| "@opentelemetry/exporter-metrics-otlp-grpc", | ||
| "@opentelemetry/exporter-prometheus", | ||
| "@opentelemetry/resources", | ||
| "@opentelemetry/sdk-metrics-base", | ||
| "@opentelemetry/semantic-conventions", | ||
| "@arizeai/openinference-semantic-conventions", | ||
| "@opentelemetry/context-async-hooks", | ||
| "@opentelemetry/exporter-trace-otlp-grpc", | ||
| "@opentelemetry/exporter-trace-otlp-http", | ||
| "@opentelemetry/exporter-trace-otlp-proto", | ||
| "@opentelemetry/otlp-exporter-base", | ||
| "@opentelemetry/sdk-node", | ||
| "@opentelemetry/sdk-trace-node", | ||
| "@opentelemetry/instrumentation", | ||
| "@opentelemetry/instrumentation-http", | ||
| "@opentelemetry/instrumentation-undici" | ||
| "@opentelemetry/instrumentation-undici", | ||
| "@opentelemetry/otlp-exporter-base", | ||
| "@opentelemetry/semantic-conventions" | ||
| ], | ||
| "reviewers": [ | ||
| "team:stack-monitoring", | ||
| "team:kibana-core" | ||
| "team:appex-ai-infra", | ||
| "team:kibana-core", | ||
| "team:kibana-security", | ||
| "team:stack-monitoring" | ||
|
Comment on lines
+4362
to
+4365
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New owners... do you all agree? I was tempted to split the list based on who uses it:
But I thought that there might be potential issues with mismatching versions, so it might be best to upgrade them in the same PR together. WDYT? cc @elastic/kibana-core @elastic/kibana-security @elastic/appex-ai-infra @elastic/stack-monitoring |
||
| ], | ||
| "matchBaseBranches": [ | ||
| "main" | ||
| ], | ||
| "labels": [ | ||
| "Team:AI Infra", | ||
| "Team:Monitoring", | ||
| "backport:all-open", | ||
| "Team:Core", | ||
| "Team:Security", | ||
| "backport:prev-minor", | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll have to manually switch to |
||
| "release_note:skip" | ||
| ], | ||
| "minimumReleaseAge": "7 days", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,12 @@ export class ImportResolver { | |
| return Path.resolve(REPO_ROOT, `node_modules/@modelcontextprotocol/sdk/dist/esm/${relPath}`); | ||
| } | ||
|
|
||
| // We need this "hack" because our current import-resolver doesn't support "exports" in package.json. | ||
| // We should be able to remove this once we support cjs/esm interop. | ||
| if (req.startsWith('@elastic/opentelemetry-node/sdk')) { | ||
| return Path.resolve(REPO_ROOT, `node_modules/@elastic/opentelemetry-node/lib/sdk.js`); | ||
| } | ||
|
Comment on lines
+152
to
+154
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes the runtime imports (including ESLint).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can likely add a note here so we don't forget we can remove this once the cjs/esm interop is in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we create an issue and reference it here? That way we can track the technical debt and possibly trigger someone to fix it. |
||
|
|
||
| // turn root-relative paths into relative paths | ||
| if ( | ||
| req.startsWith('src/') || | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2268,7 +2268,10 @@ | |
| "@kbn/zod-helpers/*": ["src/platform/packages/shared/kbn-zod-helpers/*"], | ||
| // END AUTOMATED PACKAGE LISTING | ||
| // Allows for importing from `kibana` package for the exported types. | ||
| "@emotion/core": ["typings/@emotion"] | ||
| "@emotion/core": ["typings/@emotion"], | ||
| // We need the custom typings "proxy" because our current import-resolver doesn't support "exports" in package.json. | ||
| // We should be able to remove this once we support cjs/esm interop. | ||
| "@elastic/opentelemetry-node/sdk": ["typings/@elastic/opentelemetry-node/sdk"], | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tell TS where to look for the types of this package.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can likely add a note here as well so we don't forget to remove this in the future |
||
| }, | ||
| // Support .tsx files and transform JSX into calls to React.createElement | ||
| "jsx": "react", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| declare module '@elastic/opentelemetry-node/sdk' { | ||
| export * from '@elastic/opentelemetry-node/types/sdk'; | ||
| } |
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.
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-nodewhich is only for node.js?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.
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/sdkor something like that.If not, we can install the vanilla packages again.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }becomesimport { 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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Dario, I'm not sure if you were talking about all three of those deps:
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:Is
createActiveInferenceSpan()used in Kibana browser code? If so, having a separate dep on@opentelemetry/corewill be necessary -- and fine to have alongside@elastic/opentelemetry-node.If just in Node.js code, the
coremodule is re-exported by@elastic/opentelemetry-node/sdk:Uh oh!
There was an error while loading. Please reload this page.
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.
Thank you @trentm for such informative comments.
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
MetricReaderthat 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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trentm, actually, we need to differentiate between runtime code and types 😢:
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.
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.
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? We would need@elastic/opentelemetryto 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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';thanimport { api } from '@elastic/opentelemetry-node/sdk';+ useapi.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of OTel is that separately importing
@opentelemetry/apiand 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-nodeand@opentelemetry/coreas 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).