open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.64k stars 762 forks source link

Should examples be updated to use NodeSDK instead of NodeTracerProvider? #4644

Open JamieDanielson opened 4 months ago

JamieDanielson commented 4 months ago

In public docs as well as the opentelemetry-js README, we show how to setup the NodeSDK. In other docs, including the auto-instrumentations-node README and various examples, we show how to setup a NodeTracerProvider.

This inconsistency can be confusing to newcomers who aren't clear on whether they should use NodeSDK or the NodeTracerProvider directly. For most, I believe the NodeSDK is the better option, and that's generally what's been recommended as of late.

We should review our various examples and convert most to use the NodeSDK, but before splitting that up into a checklist of places to update, I wanted to check if there is an objection to this. I understand the NodeSDK is experimental whereas NodeTracerProvider is stable, so that is probably the main reason it hasn't been updated everywhere. But with over a million weekly downloads on npm, it seems widespread enough in usage that the documentation change is warranted, at least in the contrib repo.

Flarna commented 4 months ago

Not sure if recommending NodeSDK is a good thing as of now. Besides the experimental state it comes with quite a lot dependencies like 4 trace exporters - but no metrics or logs exporter.

A similar topic is the auto-instrumentation-node package. It comes with a lot stuff where default config is not the best in my opinion (e.g. creating fs spans in a CJS project is quite noise at startup). Also having several cloud resource detectors (which partially do HTTP requests to cloud specific IPs) included and enabled on default might cause issue as users might wonder why there are such HTTP requests.

As a result I think the NodeSDK should be improved in that regard to offer full power without the need to depend on all of it.

I think documentation should be more clear about the pro/conts of the two variants and when to use what.

cartermp commented 4 months ago

I'm all for listing tradeoffs, but I believe that all documentation should default to the NodeSDK now. In my experience, it just doesn't break meaningfully that often, and all the nicer defaults are a better overall experience for most folks than having to configure everything the most optimally.

austinlparker commented 4 months ago

Generally I think it's preferable for the examples and the website docs to be consistent. If we're using NodeSDK on the website docs, we should probably use it for examples as well?

trentm commented 4 months ago

For "Usage" docs in instrumentation package READMEs and for examples, I think using NodeSDK would be helpful to users.

My impression is that the sdk-node package is the intended entry point -- at least eventually -- for users of OTel for Node.js. (I wasn't around when sdk-node was created; nor auto-instrumentations-node.) Maybe there are still reservations or debate on this?

Using the primitives -- configuring {Tracer,Metrics,Logger}Provider instances, registering them, registerInstrumentations, learning which of the fairly large number of @opentelemetry/* packages to use -- feels like "advanced" usage that most users hopefully shouldn't have to wade through.

Not sure if recommending NodeSDK is a good thing as of now. Besides the experimental state it comes with quite a lot dependencies like 4 trace exporters - but no metrics or logs exporter.

With all the instrumentations and OTLP exporters being experimental as well, I don't feel it is too out of line to be recommending the experimental NodeSDK as the entry-point for users.

The number of 'sdk-node's deps aren't too much more than what a user effectively needs to trace and export via OTLP. I agree that it would be nice to (a) add at least one metrics and logs OTLP exporter and (b) perhaps discuss limiting the number of included trace exporters by default.

A similar topic is the auto-instrumentation-node package.

I feel the current state of having documented user entry points in both sdk-node and auto-instrumentations-node is currently awkward. I agree that auto-instrumentations-node has issues with its defaults, so I wouldn't currently suggest it widely in usage docs. Eventually, IMO, it would be helpful to users to have something like node -r .../register.js app.js be the obvious first thing to use.

blumamir commented 4 months ago

I support imporvments to the NodeSdk and auto-instrumentation-node as suggested above, in parallel to updating the docs which @JamieDanielson suggested.

Most of OTEL end users who are reading the READMEs will most likely not need to use Providers directly and I think current content can confuse people and make them implement un-necessary complex setups.

pichlermarc commented 4 months ago

I happen to have a lot of opinions about the @opentelemetry/sdk-node package. They mostly align with @Flarna's, especially concerning:

As a result I think the NodeSDK should be improved in that regard to offer full power without the need to depend on all of it.

Concerning examples:

I think there should be a few examples that use NodeSDK as setup, but I don't think all of them should use NodeSDK. I'd prefer to have at least one manual setup example for each signal, and one or more NodeSDK examples which combines all three.

I also think we should clearly list what tradeoffs users make by using NodeSDK over the other SDK packages and vice-versa.

Since the opentelemetry.io docs are now NodeSDK only this repo is also the only place where users can actually see an example on how to set it up without NodeSDK. Having some exampels how to manually set it up can be beneficial when they have to do something that the NodeSDK does not offer yet.

Concerning @opentelemetry/sdk-node features/quality:

IMO changing the examples provides some value short-term, but long-term value is delivered by improving @opentelemetry/sdk-node both feature and code-quality-wise.

I think it's easy to agree that @opentelemetry/sdk-node is not the most polished piece of software in this repo. The public interface for it is lacking and confusing, and I expect that adding more features for metrics and logs will likely not improve the situation.

Another crucial point is that, there's no way for people to add extensions to it, like there is for other language implementations of auto-configure packages (Java comes to mind). This causes this weird split where we actually need @opentelemetry/auto-instrumentations-node. If you want to have your resource detectors added, you have to do that manually via the NodeSDK constructor, but @opentelemetry/sdk-node does not offer you any way to turn them on or off with env vars, so you need to duplicate the logic (reading env vars and instantiated them based on the contents) in @opentelemetry/auto-instrumentations-node and @opentelemetry/sdk-node (for the default detectors).

The same is true for exporters, propagators, instrumentations, ...

To summarize, we always have to add extensions as a dependency to @opentelemetry/sdk-node, which hugely limits what people can do with it, and it also makes our lives harder here in the core repo, as we sometimes have to shift packages around (see #4494) to avoid depending on contrib from here (depending on contrib from here would be a circular dep).

My vision for @opentelemetry/sdk-node is for it to include a way to register extensions (exporters, propagators, instrumentations, resourcedetectors, ...), which can then auto-configured via environment variables (or file config, or directly via code, ideally aligning it with what the structure of file config).

This would deliver value to users, contributors and third-parties (vendors, possibly other open-source project looking to leverage OTel autoconfiguration) by helping:

What this topic needs, though is someone to drive it (starting out with prototyping, planning, creating issues, plus someone to implement and someone dedicated to review the changes).

(I myself am currently focusing on #4585, and once completed, will move my focus on delivering a stable logs sdk/api, then focus on delivering a stable instrumentation base. IMO it only becomes then viable to implement these changes in NodeSDK otherwise we'd be building a lot logic on top of unstable interfaces, that does not mean that we cannot plan and prototype while these things are being done, we just need someone who's willing to do it and also has the bandwidth to do so. :slightly_smiling_face: )

github-actions[bot] commented 2 months ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

JamieDanielson commented 1 month ago

I'm setting this to "never-stale" right now because the details here are valuable for next steps, and leaving in the backlog.