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

fix(core): added falsy check to make otel core work with browser where webpack config had process as false or null #3617

Merged
merged 8 commits into from
Feb 27, 2023

Conversation

ravindra-dyte
Copy link
Contributor

@ravindra-dyte ravindra-dyte commented Feb 16, 2023

Which problem is this PR solving?

Fixes #3613

Some customers of ours are using webpack where they had set all node globals to false (https://webpack.js.org/migrate/5/), as suggested by Stack overflow responses such as https://stackoverflow.com/questions/64557638/how-to-polyfill-node-core-modules-in-webpack-5 where best voted answers suggest devs to set Node Globals as false as a fallback. Current check on process was doing a plain type check to see equality with undefined however typeof false is boolean & typeof null is object.

To ensure that OTEL works with any config having undefined/null/false, this PR was raised.

Short description of the changes

Added falsy checks while using process in browser environment to ensure that the OTEL can work with projects where webpack config has process set to false/null. Refer https://stackoverflow.com/questions/64557638/how-to-polyfill-node-core-modules-in-webpack-5 to see why process is set to false/null.

Type of change

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

How Has This Been Tested?

Created a separate file to write tests. A separate file for impact testing globals would make it expandable in the future to test out other globals such as URL, and crypto. For this new Test file, replaced the globals.process in before to test the scenario.

  1. Added test to see if WebTraceProvider would initialise normally when globals process is set as false.
  2. Added if it would work with a custom trace/span id generator.

Before change:
image

(If I set globals.process to false with existing tests)
image

After:
image

Checklist:

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

…th webpack config having process as false or null
@ravindra-dyte ravindra-dyte requested a review from a team February 16, 2023 06:52
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 16, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@Flarna
Copy link
Member

Flarna commented Feb 16, 2023

Could you please add add a test to verify the use case you want to support is actually fixed?

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #3617 (bcb6e2b) into main (1fbc828) will increase coverage by 0.39%.
The diff coverage is n/a.

❗ Current head bcb6e2b differs from pull request most recent head 5492ed1. Consider uploading reports for the commit 5492ed1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3617      +/-   ##
==========================================
+ Coverage   93.89%   94.28%   +0.39%     
==========================================
  Files         265        1     -264     
  Lines        7371      175    -7196     
  Branches     1497       39    -1458     
==========================================
- Hits         6921      165    -6756     
+ Misses        450       10     -440     
Impacted Files Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts
api/src/trace/NoopTracerProvider.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
api/src/propagation/NoopTextMapPropagator.ts
...kages/otlp-exporter-base/src/platform/node/util.ts
...mental/packages/api-events/src/NoopEventEmitter.ts
api/src/baggage/utils.ts
...ackages/opentelemetry-core/src/trace/TraceState.ts
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts
.../exponential-histogram/mapping/LogarithmMapping.ts
... and 254 more

@ravindra-dyte
Copy link
Contributor Author

ravindra-dyte commented Feb 16, 2023

Added the tests & changelog. Let me know if this would suffice @Flarna

@Flarna
Copy link
Member

Flarna commented Feb 16, 2023

I thought more about a more direct test of getEnvWithoutDefaults() but I guess the existing one is also ok.

@ravindra-dyte
Copy link
Contributor Author

ravindra-dyte commented Feb 16, 2023

My bad. 🤦‍♂️ I am used to auto-linting in the commit phase. Fixed the linting issues.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Chengzhong Wu <[email protected]>
@ravindra-dyte ravindra-dyte changed the title fix(process-usage): added falsy check to make otel core work with browser where webpack config had process as false or null fix(core): added falsy check to make otel core work with browser where webpack config had process as false or null Feb 22, 2023
@dyladan dyladan added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc labels Feb 22, 2023
@pichlermarc
Copy link
Member

Merging this as the failing tests are unrelated to this PR (#3628)

@pichlermarc pichlermarc merged commit 7f63a6f into open-telemetry:main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read properties of undefined. Reading OTEL_SDK_DISABLED
5 participants