open-telemetry / opentelemetry-js

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

Avoid using 'automatic' and 'automatic instrumentation' terms #3298

Open pellared opened 1 year ago

pellared commented 1 year ago

Could you please change the docs here and https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node so that we do not confuse the users that JavaScript is doing automatic instrumentation?

I see that it has library instrumentations, a bundle (auto-instrumentations-node), Otel SDKs, OTel API, but I do not see any method that does not require changing the source code.

Reference issue: https://github.com/open-telemetry/opentelemetry.io/issues/1689

Flarna commented 1 year ago

The idea is to do the OTel init for the instrumentations in a separate file and preload it by using -r command line argument or via NODE_OPTIONS environment. This should work without toching any user app file.

pellared commented 1 year ago

LGTM. Thanks for clarification

svrnm commented 1 year ago

I forgot to subscribe on that issue, can we please re-open it (or I raise a new one), because I still have my concerns:

The idea is to do the OTel init for the instrumentations in a separate file and preload it by using -r command line argument or via NODE_OPTIONS environment. This should work without toching any user app file.

That "separate file" is not contained in the auto-instrumentations-node package, as of today I need to write it myself. So auto-instrumentations-node is right now just a meta package of instrumentation libraries, comparable to ruby's opentelemetry-instrumentation-all.

@pellared, some other folks and I had a lengthy discussion (https://github.com/open-telemetry/opentelemetry.io/issues/1689) about the meaning of "Auto Instrumentation" and my conclusion is that if "Auto Instrumentation" stands for that all-in-one-do-not-touch-my-code-solution (Related: https://github.com/open-telemetry/opentelemetry-js/pull/2891). Right now, auto-instrumentation-node does not cover that.

So I am wondering, are there plans to turn that package into "Auto Instrumentation" in that sense?

pellared commented 1 year ago

@open-telemetry/technical-committee WDYT?

svrnm commented 1 year ago

I try to raise an issue over at https://github.com/open-telemetry/opentelemetry-specification, because this is not only an issue with the nodejs auto-instrumentation-node package and I think it belongs there?

Will need a little bit & link it here.

svrnm commented 1 year ago

https://github.com/open-telemetry/opentelemetry-specification/issues/2866

Flarna commented 1 year ago

I agree that something like instrumentations-node-all would be better then auto-instrumentations-node. But actually all is also not correct because there are instrumentations hosted somewhere else which are not included.

svrnm commented 1 year ago

I agree that something like instrumentations-node-all would be better then auto-instrumentations-node. But actually all is also not correct because there are instrumentations hosted somewhere else which are not included.

The question is, would it be possible to rename that package or would that break a lot of things?

pellared commented 1 year ago

The question is, would it be possible to rename that package or would that break a lot of things?

It would "break" at least:

  1. Any package bumping automation I am aware of (like @dependabot, npm update)
  2. Existing docs and tutorials (they will still work but suggest the using "legacy" package)
Flarna commented 1 year ago

We did similar breaking renames in the past, e.g. @opentelemetry/tracing is now @opentelemetry/sdk-trace-base (it's interesting that the download numbers for the deprecated package are still quite high).

Tends to result in quite some issues raised. Easy to solve but clearly the fun factor is quite low.

svrnm commented 1 year ago

If such a change is feasible, it would take away some confusion, although I would think it's worth it to look where the discussion on that goes at the spec (https://github.com/open-telemetry/opentelemetry-specification/issues/2866)

Flarna commented 1 year ago

I think the download numbers for @opentelemetry/auto-instrumentations-node are currently lower as for @opentelemetry/tracing and friends at the time they were renamed.

But we should for sure avoid to rename more then once so better wait on spec discussion.

svrnm commented 1 year ago

@pellared , @Flarna can we keep this issue open until the underlying discussion is resolved?

github-actions[bot] commented 1 year 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.

ashutosh887 commented 1 year ago

I would like to work on this @pellared

mannyistyping commented 1 year ago

@svrnm is this still an open issue or can it be closed now with #2852?

I am curious if this is still a "Good First Issue" for folks to grab, I see @ashutosh887 has requested to work on this. If it is abandoned, I'd be happy to take this up.

ashutosh887 commented 1 year ago

Yup @mannyistyping They didn't assign

svrnm commented 1 year ago

@mannyistyping not sure how #2852 is related to that?

I think this might not be relevant anymore, since the auto-instrumentations-node package now has an auto instrumentation capability built in.

cc @samimusallam

pichlermarc commented 1 year ago

@svrnm is this still an open issue or can it be closed now with #2852?

I am curious if this is still a "Good First Issue" for folks to grab, I see @ashutosh887 has requested to work on this. If it is abandoned, I'd be happy to take this up.

Yep I'd also consider this not up-for-grabs or good-first-issue at the moment. We first need to figure out what we want to do before marking it this way.

I read through the issue on the spec (https://github.com/open-telemetry/opentelemetry-specification/issues/2866), and, as @svrnm said, we now have a no-code-changes approach in the auto-instrumentations package (even though it's far from feature-complete), so I'd say there's no need to rename this package anymore. However, we should consider reviewing the use of "automatic instrumentation" in our docs (the ones here on opentelemetry-js/opentelemetry-js-contrib) and replacing it with "instrumentation library" or similar terminology. WDYT @svrnm?