opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

Avoid using 'automatic' and 'automatic instrumentation' terms

Open pellared opened this issue 2 years ago • 14 comments

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

pellared avatar Oct 06 '22 12:10 pellared

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.

Flarna avatar Oct 06 '22 17:10 Flarna

LGTM. Thanks for clarification

pellared avatar Oct 06 '22 17:10 pellared

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?

svrnm avatar Oct 11 '22 13:10 svrnm

@open-telemetry/technical-committee WDYT?

pellared avatar Oct 11 '22 13:10 pellared

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 avatar Oct 11 '22 13:10 svrnm

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

svrnm avatar Oct 11 '22 15:10 svrnm

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.

Flarna avatar Oct 11 '22 16:10 Flarna

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?

svrnm avatar Oct 12 '22 08:10 svrnm

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)

pellared avatar Oct 12 '22 09:10 pellared

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.

Flarna avatar Oct 12 '22 09:10 Flarna

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)

svrnm avatar Oct 12 '22 10:10 svrnm

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.

Flarna avatar Oct 12 '22 10:10 Flarna

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

svrnm avatar Oct 13 '22 09:10 svrnm

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.

github-actions[bot] avatar Dec 19 '22 06:12 github-actions[bot]

I would like to work on this @pellared

ashutosh887 avatar Apr 19 '23 11:04 ashutosh887

@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.

mannyistyping avatar Jul 13 '23 03:07 mannyistyping

Yup @mannyistyping They didn't assign

ashutosh887 avatar Jul 13 '23 03:07 ashutosh887

@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

svrnm avatar Jul 13 '23 07:07 svrnm

@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?

pichlermarc avatar Jul 13 '23 13:07 pichlermarc