fix(sdk-node): fix exporter to be read only OTEL_TRACES_EXPORTER is set to a valid exporter

[svetlanabrennan]

…mpty

Signed-off-by: Svetlana Brennan [email protected]

Which problem is this PR solving?

BasicTracerProvider was attempting to read exporter from env (by using __buildExporterFromEnv) because OTEL_TRACES_EXPORTER default value was changed from "none" to "otlp" in this pr. .

So this pr fixes that issue by changing OTEL_TRACES_EXPORTER to default value of "". But this also affects TracerProviderWithEnvExporters class which is used by the NodeSDK so the necessary logic has to be changed to now handle OTEL_TRACES_EXPORTER default value set to "".

Fixes # (#3449) and (#3422)

Short description of the changes

• OTEL_TRACES_EXPORTER default value set to ""
• __buildExporterFromEnv in BasicTracerProvider now doesn't set up an exporter if OTEL_TRACES_EXPORTER is "" and instead creates a NoopSpanProcessor.
• TracerProviderWithEnvExporters now sets up an OTLP exporter as the default exporter when OTEL_TRACES_EXPORTER is set to "". Previously it didn't set up any exporters when OTEL_TRACES_EXPORTER was set to "".
• added additional tests to handle this new logic
• added tests to make sure that there was no diag error when a user sets up the BasicTracerProvider. See issues tracer provider always attempts to read exporter from env #3449 and OTEL_TRACES_EXPORTER default value incompatible with @opentelemetry/sdk-trace-base #3422

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Unit Tests

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[codecov]

Codecov Report

Merging #3492 (0f00fd6) into main (c93ab9e) will decrease coverage by 0.36%.
The diff coverage is 100.00%.

❗ Current head 0f00fd6 differs from pull request most recent head 947b307. Consider uploading reports for the commit 947b307 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3492      +/-   ##
==========================================
- Coverage   93.78%   93.41%   -0.37%     
==========================================
  Files         249      228      -21     
  Lines        7612     6334    -1278     
  Branches     1587     1351     -236     
==========================================
- Hits         7139     5917    -1222     
+ Misses        473      417      -56     

Impacted Files	Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts	92.30% <ø> (ø)
...etry-sdk-node/src/TracerProviderWithEnvExporter.ts	98.71% <100.00%> (+0.16%)	⬆️
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts	95.53% <100.00%> (ø)
packages/opentelemetry-sdk-trace-web/src/utils.ts	65.83% <0.00%> (-29.20%)	⬇️
...emetry-instrumentation-xml-http-request/src/xhr.ts
...ckages/opentelemetry-browser-detector/src/types.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
...es/opentelemetry-context-zone-peer-dep/src/util.ts
...es/opentelemetry-instrumentation-grpc/src/utils.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
... and 15 more

[svetlanabrennan]

The two failing tests are related to the karma server:

 [karma-server]: Error: error:0308010C:digital envelope routines::unsupported

[legendecas]

The browser tests are failing: (https://github.com/open-telemetry/opentelemetry-js/actions/runs/3708747298/jobs/6286637020)

  BasicTracerProvider
    constructor
      when options not defined
        ✓ should construct an instance
        ✓ should use noop span processor by default
        ✓ should use noop span processor by default and no diag error
      when user sets unavailable exporter
        ✗ should use noop span processor by default and show diag error
	AssertError: expected error to be called with arguments 
	    at Object.fail (webpack-internal:///../../node_modules/sinon/lib/sinon/assert.js:120:25)
	    at failAssertion (webpack-internal:///../../node_modules/sinon/lib/sinon/assert.js:66:20)
	    at assert.<computed> [as calledWith] (webpack-internal:///../../node_modules/sinon/lib/sinon/assert.js:95:17)
	    at Context.eval (webpack-internal:///./test/common/BasicTracerProvider.test.ts:80:30)

Chrome Headless 108.0.5359.98 (Linux x86_64) BasicTracerProvider constructor when user sets unavailable exporter should use noop span processor by default and show diag error FAILED
	AssertError: expected error to be called with arguments 
	    at Object.fail (webpack-internal:///../../node_modules/sinon/lib/sinon/assert.js:120:25)
	    at failAssertion (webpack-internal:///../../node_modules/sinon/lib/sinon/assert.js:66:20)
	    at assert.<computed> [as calledWith] (webpack-internal:///../../node_modules/sinon/lib/sinon/assert.js:95:17)
	    at Context.eval (webpack-internal:///./test/common/BasicTracerProvider.test.ts:80:30)

When running the browser tests locally, use node.js v16 instead of a higher version: https://stackoverflow.com/questions/69692842/error-message-error0308010cdigital-envelope-routinesunsupported.

[pichlermarc]

Great work, thank you for fixing this! 🙂
Once we have the Browser tests working this should be good to go, I have put a suggestion in the comments 🙂