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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jan 3, 2023
Merged

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

merged 6 commits into from
Jan 3, 2023

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Dec 15, 2022 •
edited
Loading

…mpty

Signed-off-by: Svetlana Brennan 50715937+svetlanabrennan@users.noreply.github.com

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

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

@svetlanabrennan
fix(sdk-node): refactor to use otlp as default exporter when env is e…
cabd67a 
…mpty

Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
@svetlanabrennan svetlanabrennan requested a review from a team December 15, 2022 23:02
@svetlanabrennan svetlanabrennan mentioned this pull request Dec 15, 2022
Closed
@codecov
Copy link

codecov bot commented Dec 15, 2022 •
edited
Loading

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 svetlanabrennan changed the title fix(sdk-node): fix tracer provider from always reading exporter from env fix(sdk-node): fix exporter to be read only OTEL_TRACES_EXPORTER is set to a valid exporter Dec 15, 2022
@svetlanabrennan
fix(sdk-node): add changelog
3eacba6 
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
@svetlanabrennan
Merge branch 'fix-default-tracer-from-env' of github.com:svetlanabren…
7f7917a 
…nan/opentelemetry-js into fix-default-tracer-from-env
@svetlanabrennan
Copy link
Contributor Author

The two failing tests are related to the karma server:

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

@legendecas
Copy link
Member

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 pichlermarc mentioned this pull request Jan 3, 2023
Closed
pichlermarc
pichlermarc reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts Outdated Show resolved Hide resolved
@svetlanabrennan
(sdk-node): using existing env source
ac38923 
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
@svetlanabrennan
Merge branch 'main' into fix-default-tracer-from-env
947b307
@svetlanabrennan svetlanabrennan mentioned this pull request Jan 3, 2023
Merged
@dyladan dyladan merged commit 126ae93 into open-telemetry:main Jan 3, 2023
@pichlermarc pichlermarc mentioned this pull request Mar 8, 2023
Closed
2 tasks
@Smarker Smarker mentioned this pull request Jul 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc approved these changes

@legendecas legendecas legendecas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@svetlanabrennan @legendecas @pichlermarc @dyladan