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

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

Open
pellared opened this issue Oct 6, 2022 · 19 comments
Open

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

pellared opened this issue Oct 6, 2022 · 19 comments
Labels
document Documentation-related never-stale

Comments

@pellared
Copy link
Member

pellared commented Oct 6, 2022

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: open-telemetry/opentelemetry.io#1689

@pellared pellared changed the title Avoid using 'automatic instrumentation' term Avoid using 'automatic' and 'automatic instrumentation' terms Oct 6, 2022
@dyladan dyladan added good first issue Good for newcomers up-for-grabs Good for taking. Extra help will be provided by maintainers document Documentation-related labels Oct 6, 2022
@Flarna
Copy link
Member

Flarna commented Oct 6, 2022

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

pellared commented Oct 6, 2022

LGTM. Thanks for clarification

@pellared pellared closed this as completed Oct 6, 2022
@svrnm
Copy link
Member

svrnm commented Oct 11, 2022

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 (open-telemetry/opentelemetry.io#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: #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
Copy link
Member Author

@open-telemetry/technical-committee WDYT?

@svrnm
Copy link
Member

svrnm commented Oct 11, 2022

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

svrnm commented Oct 11, 2022

@Flarna
Copy link
Member

Flarna commented Oct 11, 2022

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

svrnm commented Oct 12, 2022

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

pellared commented Oct 12, 2022

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

Flarna commented Oct 12, 2022

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

svrnm commented Oct 12, 2022

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 (open-telemetry/opentelemetry-specification#2866)

@Flarna
Copy link
Member

Flarna commented Oct 12, 2022

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

svrnm commented Oct 13, 2022

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

@pellared pellared reopened this Oct 13, 2022
@github-actions
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.

@ashutosh887
Copy link

I would like to work on this @pellared

@mannyistyping
Copy link

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

Yup @mannyistyping
They didn't assign

@svrnm
Copy link
Member

svrnm commented Jul 13, 2023

@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 pichlermarc removed the good first issue Good for newcomers label Jul 13, 2023
@pichlermarc pichlermarc removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jul 13, 2023
@pichlermarc
Copy link
Member

pichlermarc commented Jul 13, 2023

@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 (open-telemetry/opentelemetry-specification#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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related never-stale
Projects
None yet
Development

No branches or pull requests

8 participants