fix(sdk-node): move @opentelemetry/exporter-jaeger to dev dependencies

[Vadman97]

Which problem is this PR solving?

@opentelemetry/sdk-node depends on @opentelemetry/exporter-jaeger despite never using it directly.
This increases the number of dependencies required to install @opentelemetry/sdk-node even if
@opentelemetry/exporter-jaeger is not used.

Fixes #3759

Short description of the changes

The change moves @opentelemetry/exporter-jaeger to the dev dependencies to resolve it in tests
but avoid requiring it for general usage of @opentelemetry/sdk-node. This is consistent with the
@opentelemetry/sdk-node documentation which states
that exporters must be explicit dependencies.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Locally building and testing the package after running npm install in opentelemetry-js/experimental/packages/opentelemetry-sdk-node.

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added - N/A
• Documentation has been updated - NA (already consistent)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Vadman97 / name: Vadim Korolik (ae4acdc, 4c1dc82)

[codecov]

Codecov Report

Merging #4049 (09b7308) into main (4f28f90) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 09b7308 differs from pull request most recent head 4c1dc82. Consider uploading reports for the commit 4c1dc82 to get more accurate results

@@            Coverage Diff             @@
##             main    #4049      +/-   ##
==========================================
+ Coverage   92.56%   92.57%   +0.01%     
==========================================
  Files         292      292              
  Lines        8178     8178              
  Branches     1692     1692              
==========================================
+ Hits         7570     7571       +1     
+ Misses        608      607       -1     

[Flarna]

As you can see in the test run a changelog entry is needed.
Not sure if this should be marked as breaking. At least a clears statement in the changelog should me made that an explicit dependency is needed now. Maybe even improve the corresponding section in readme to make this clearer.

[Vadman97]

Thank you @Flarna! Will add the changelog entry shortly and will update the readme as well.

[pichlermarc]

Thank you for your contribution @Vadman97, removing this exporter has been long overdue 🙂

The exporter is lazy-required in sdk-node to allow sdk-node to work with bundling tools as the Jaeger exporter does not support them. We can remove it, but it will break users with the environment variable set.

opentelemetry-js/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts

Line 76 in d3436bf

const { JaegerExporter } = require('@opentelemetry/exporter-jaeger');

We'll also have to adapt the error message that the exporter has to be installed manually.

opentelemetry-js/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts

Lines 79 to 81 in d3436bf

	throw new Error(
	`Could not instantiate JaegerExporter. This could be due to the JaegerExporter's lack of support for bundling. If possible, use @opentelemetry/exporter-trace-otlp-proto instead. Original Error: ${e}`
	);

Previously we've gone the way of releasing a version that adds a postinstall script that alerts users that a feature will disappear in the next release (see #3833).

As @opentelemetry/sdk-node is widely used, I propose we do the same before merging this PR. It will delay this PR for a bit, but it will give users some advance notice that their telemetry will go missing when they update. 🤔

See also:

• jaeger-client is now deprecated #2618
• fix(sdk-node): lazy require @opentelemetry/exporter-jaeger #3739
• chore(instrumentation-grpc): add 'grpc' deprecation notice postinstall script #3833
• NodeSDK fails in strict mode with Legacy octal escape is not permitted in strict mode #3759
• Can't update to @opentelemetry/sdk-node v0.34.0 using esbuild #3521

[pichlermarc]

Thank you for working on this; this looks good as-is 🙂

For lack of a better way to do it, I'm just using the "Changes Requested" state to block this until #4068 is merged and released (as mentioned in #4049 (comment)). Once we have it released, we can merge this PR 🙂

[Vadman97]

Thanks for your help @pichlermarc , and sorry for the delay here. I'm updating the branch now to change TracerProviderWithEnvExporter.ts as you described.

[Vadman97]

@pichlermarc - is this ready to merge now that #4068 is in?

[pichlermarc]

@pichlermarc - is this ready to merge now that #4068 is in?

Yes, but we still need to cut a release first to get #4068 out 🙂
I'm going to release 0.42.0 on Monday (trying not to release on Friday) - after that is done we can merge this in.

Sorry for the delay.

[erikshestopal]

@pichlermarc Seems like we can merge this PR since #4068 is out?

[pichlermarc]

ready to merge now that the release it out.

[Vadman97]

hey @pichlermarc , sorry to pester here - is there any estimate as to when the next version will be released to npm so that we can update our dependency on @opentelemetry/sdk-node?

[pichlermarc]

The release with the warning message went out last week; I'd cut another release mid-next week.

[Vadman97]

Hey @pichlermarc , any updates here on the next release being shipped?

[pichlermarc]

@Vadman97 release PR is open at #4183