Skip to content
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

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

Open
JamieDanielson opened this issue Apr 17, 2024 · 8 comments

Comments

@JamieDanielson
Copy link
Member

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
Copy link
Member

Flarna commented Apr 18, 2024

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

trentm commented Apr 19, 2024

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
Copy link
Member

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
Copy link
Member

pichlermarc commented Apr 22, 2024

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:

  • users to create their own, auto-configurable NodeSDK-like thing if they don't want to depend on everything (for instance: why include an XRay propagator, aws-instrumentations when all your apps are running on Azure)
  • us (JS SIG) by helping tor reduce confusion about what goes where:
    • As users can then easily build their own, we can decide to provide three "distros":
      • base (just an auto-configure package, there's nothing in it, but it allows you to pass in which extensions you want to have and they'll get auto-configured)
      • core/plain (what SDK-node is today, only packages that are in the spec and not thrid-party)
      • contrib (all the things from contrib)
  • third-parties to create their own "distro", which includes a pre-defined set of dependencies they decide on (think pre-configured/auto-configured exporters, side-by-side configurable with OTLP exporters, vendor-specific resource detectors, etc)
    • these could also be made for specific use-cases, like a lambda-distro where you take the core distro and add the lambda propagator to it.

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. 🙂 )

Copy link

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants