Conversation
Adds support for OpenTelemetry tracing, as described in the included docs. Custom instrumentation is not yet supported.
|
Review requested:
|
nodejs-github-bot
added
lib / src
legendecas
left a comment
legendecas
left a comment
There was a problem hiding this comment.
AFAICT, it would create confusion that the built-in support does not work with the @opentelemetry/api package, which is intended to be the global provider for otel components, like propagators and exporters.
|
I personally find the opentelemetry apis really cumbersome to work with. Maybe there is something better that can be done. But I also concur that some kind of high level compatibility api would need to exist. |
|
It would be really nice to get ahold of https://www.npmjs.com/package/otel and add this new core module without the prefix. |
| otel.start({ endpoint: 'http://collector.example.com:4318' }); | ||
|
|
||
| // ... application code ... | ||
| otel.stop(); |
There was a problem hiding this comment.
More a style nit... feel free to ignore. I find this start()/stop() pattern a bit cumbersome. I'd be happier something like...
const stop = otel.start();
stop();which also allows for...
{
using session = otel.start();
// automatically stops
}
| * `options` {Object} | ||
| * `endpoint` {string} The OTLP collector endpoint URL. | ||
| **Default:** `'http://localhost:4318'`. | ||
| * `filter` {string|string\[]} Optional comma-separated string or array of |
There was a problem hiding this comment.
Nit: can we pick one? I'd prefer just string[].
Agree that an OpenTelemetry SDK is not built-in to the Node.js instrumentation. However, this implementation is not fully compliant with the OpenTelemetry design. The instrumentation should not implement the OpenTelemetry SDK components, like exporters and tracer SDK. The three OpenTelemetry layers, API, SDK, and instrumentations, have their clear scopes and allows open customization without any vendor lock-in. That is, instrumentations will use the API to create spans and metrics, but do not directly depend on the SDK. This allows instrumentations to be vendor-agnostic. And observability vendors could provide their SDK (or just an exporter) to do whatever propriatary works, like exporting. I think a Node.js built-in |
Qard
left a comment
Qard
left a comment
There was a problem hiding this comment.
Some small questions/ideas, but otherwise LGTM!
| ### `OTEL_SERVICE_NAME` | ||
|
|
||
| Standard OpenTelemetry environment variable used to set the service name in | ||
| exported resource attributes. Defaults to `node-<pid>`. |
There was a problem hiding this comment.
We already have to load the package.json during startup, don't we? So perhaps we could make this use the name field from it, if present, and fallback to that?
| let timeOriginNs; | ||
| function getTimeOriginNs() { | ||
| if (timeOriginNs === undefined) { | ||
| // getTimeOriginTimestamp() returns ms since Unix epoch. | ||
| // Multiply by 1e6 to get nanoseconds. | ||
| const originMs = getTimeOriginTimestamp(); | ||
| timeOriginNs = BigInt(MathRound(originMs * 1e6)); | ||
| } | ||
| return timeOriginNs; | ||
| } | ||
|
|
||
| function hrTimeToNanos() { | ||
| const relativeMs = now(); | ||
| const ns = getTimeOriginNs() + BigInt(MathRound(relativeMs * 1e6)); | ||
| return `${ns}`; | ||
| } |
There was a problem hiding this comment.
I'm wondering a bit about the perf of this. Did you try a high/low pair like process.hrtime() returns to be able to do the 64-bit timestamp rather than this string approach? I'm a bit worried about the perf implications of lots of bigint timestamps being generated all the time. 🤔
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #61907 +/- ##
==========================================
+ Coverage 89.73% 89.75% +0.01%
==========================================
Files 675 680 +5
Lines 204674 205883 +1209
Branches 39330 39565 +235
==========================================
+ Hits 183667 184781 +1114
- Misses 13283 13387 +104
+ Partials 7724 7715 -9
🚀 New features to boost your workflow:
|
| urlStr + '/v1/traces'; | ||
| } | ||
|
|
||
| const parsed = new URL(urlStr); |
There was a problem hiding this comment.
Could this condition be simplified to the following?:
const parsed = new URL('/v1/traces', endpoint);Tests with the Node REPL:
> new URL("/v1/traces", "http://localhost:4318/hello?foo=bar").href
'http://localhost:4318/v1/traces'
> new URL("/v1/traces", "http://localhost:4318/").href
'http://localhost:4318/v1/traces'
> new URL("/v1/traces", "http://localhost:4318").href
'http://localhost:4318/v1/traces'
> new URL("/v1/traces", "http://localhost:4318/v1").href
'http://localhost:4318/v1/traces'
> new URL("/v1/traces", "http://localhost:4318/v1/traces").href
'http://localhost:4318/v1/traces'
> new URL("/v1/traces", "http://localhost:4318/v1/traces/").href
'http://localhost:4318/v1/traces'
8 participants
Adds support for OpenTelemetry tracing, as described in the included docs. Custom instrumentation is not yet supported.
This PR is intended to lay the groundwork for future PRs where API support for custom instrumentations is added. The OpenTelemetry SDK is explicitly not used, mainly to avoid bloat.
A programmatic API to enable and disable tracing is added as a new module,
node:otel, but I'm very open to suggestions in terms of where to add it instead, keeping in mind that the intent is to eventually expose an API for custom instrumentation alongside it.Disclaimer: As an experiment, Claude Code was used to build this PR, based on a hand-written prototype I built (but did not publish) about a year ago. Everything was thoroughly reviewed by me prior to PR submission, including some hand-edits.