refactor: use package json auto generated export also for npm package name

[blumamir]

This PR is a suggestion to replace this instrumentation base constructor call:

  constructor(config: CucumberInstrumentationConfig = {}) {
    super("@opentelemetry/instrumentation-foo", VERSION, config);
  }

With this one:

  constructor(config: CucumberInstrumentationConfig = {}) {
    super(PACKAGE_NAME, PACKAGE_VERSION, config);
  }

Where the auto generated file version.ts will also include the package name from package.json.

This PR also rename the cryptic VERSION symbol to NPM_PACKAGE_VERSION which I think is more readable.

I think the new format is better since:

• It makes it clear that the value we use is the package name and not something else and carries semantic meaning.
• The value is guaranteed to match the content of the package.json, just like we currently use the VERSION from package.json in instrumentation base constructor. no room for mistakes or misunderstandings.
• The code is better formatted and more consistent

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.38%. Comparing base (dfb2dff) to head (dbe1243).
Report is 137 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2203      +/-   ##
==========================================
- Coverage   90.97%   90.38%   -0.60%     
==========================================
  Files         146      147       +1     
  Lines        7492     7506      +14     
  Branches     1502     1573      +71     
==========================================
- Hits         6816     6784      -32     
- Misses        676      722      +46     

Files	Coverage Δ
...ages/opentelemetry-host-metrics/src/BaseMetrics.ts	100.00% <100.00%> (ø)
.../winston-transport/src/OpenTelemetryTransportV3.ts	93.33% <100.00%> (ø)
...lugins/node/instrumentation-amqplib/src/amqplib.ts	90.40% <100.00%> (ø)
...de/instrumentation-cucumber/src/instrumentation.ts	92.10% <100.00%> (-0.21%)	⬇️
.../instrumentation-dataloader/src/instrumentation.ts	99.06% <100.00%> (-0.02%)	⬇️
...nstrumentation-runtime-node/src/instrumentation.ts	89.18% <100.00%> (ø)
...ode/instrumentation-tedious/src/instrumentation.ts	92.30% <100.00%> (-0.17%)	⬇️
plugins/node/instrumentation-undici/src/undici.ts	96.23% <100.00%> (ø)
...-instrumentation-aws-lambda/src/instrumentation.ts	93.60% <100.00%> (-0.08%)	⬇️
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts	95.02% <100.00%> (-0.11%)	⬇️
... and 29 more

... and 20 files with indirect coverage changes

[JamieDanielson]

This is another change I'm a fan of as it increases consistency and reduces errors. I've left a few suggestions, though some may fall slightly out of scope (package names used elsewhere in the instrumentation).

[JamieDanielson]

As part of this change I think we could remove the MODULE_NAME constant from the constants.ts file. Then we can also rename in the init() there is another constants.MODULE_NAME.

[blumamir]

I totally agree, this pattern should be simplified and refactored to a common one across the repo 👍

the MODULE_NAME export in constants.ts currently looks like this:

export const MODULE_NAME = 'router';

And still being used in instrumentation here. We cannot utilize the new PACKAGE_NAME to remove it altogether at the moment.

This PR touches on the topic of "Instrumentation Package" and the MODULE_NAME we refer to is the "Instrumented Package". In order not to introduce too many things into one PR which can make it harder to review and merge in, I suggest we will target the instrumented package patterns in a follow-up PR, which might also include more cleanups and aligning across the codebase.

[JamieDanielson]

Yeah makes sense to have a new PR for the additional cleanup 👍

[JamieDanielson]

same as router - As part of this change I think we could remove the MODULE_NAME constant from the constants.ts file. Then we can also rename in the init() there is another constants.MODULE_NAME.

[blumamir]

agree. will handle as follow-up PR

[JamieDanielson]

I think we should also update the moduleName here on the next line to be PACKAGE_NAME. It gets used in the enable() and disable() methods.

[blumamir]

This suggested change looks right to me, but it will introduce a change to the instrumentation (it's name) which is probably ok but I don't want to bundle it into this PR, as it will change the scope from refactor to fix.

[JamieDanielson]

I think we can also update the line higher up in the class for readonly component: string = 'react-load'; to instead be readonly component: string = PACKAGE_NAME;

[blumamir]

same comment as above - I support doing this as a dedicated fix PR which only targets the name of the instrumentation (which is an end user public API content).

[JamieDanielson]

same as router - As part of this change I think we could remove the MODULE_NAME constant from the constants.ts file. Then we can also rename in the init() there is another constants.MODULE_NAME.

[blumamir]

@JamieDanielson Thanks for another thorough review.

You justifiably find many opportunities for further improvement, which I choose to keep out of the scope of this PR so it is very specific on one single change.

[JamieDanielson]

LGTM. As discussed in comments we can do further cleanup in a follow-up PR to minimize scope.