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(otlp-exporter-base): replaced usage of window with _globalThis #4157

Conversation

cristianmadularu
Copy link
Contributor

@cristianmadularu cristianmadularu commented Sep 22, 2023

Which problem is this PR solving?

The issue being fixed is an error being thrown when trying to export logs from code running in a web worker. I noticed that the exporter is using the 'window' object instead of globalThis (or the _globalThis instance provided by the otlp lib). The 'window' is not available to code running in a worker, only to the main app thread.
Please see this issue #3991

Fixes #3991

Short description of the changes

Replaced the usage of 'window' with the '_globalThis' instance exposed by @opentelemetry/core

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

In order to reproduce the issue, try to export logs from code running in a web worker. You would get an error indicating that 'window' is not defined.
I have a patched version replacing 'window' with 'globalThis' in which the issue no longer occurs.
I also ran the 'compile' and 'test' scripts before and after my changes and observed the same results.

Checklist:

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

@cristianmadularu cristianmadularu requested a review from a team September 22, 2023 15:10
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 22, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #4157 (5551852) into main (2b9832e) will decrease coverage by 1.78%.
The diff coverage is n/a.

❗ Current head 5551852 differs from pull request most recent head 021f5a2. Consider uploading reports for the commit 021f5a2 to get more accurate results

@@            Coverage Diff             @@
##             main    #4157      +/-   ##
==========================================
- Coverage   92.29%   90.52%   -1.78%     
==========================================
  Files         329      159     -170     
  Lines        9370     3757    -5613     
  Branches     1991      835    -1156     
==========================================
- Hits         8648     3401    -5247     
+ Misses        722      356     -366     

see 175 files with indirect coverage 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.

Looks good, thanks for taking care of this 🙂
I took the liberty to add the changelog entry to this PR 🙂

@pichlermarc pichlermarc merged commit 2499708 into open-telemetry:main Sep 26, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should use 'globalThis' instead of 'window' for the browser usage
2 participants