opentelemetry-js feat(otlp-exporter-base): Add fetch sender for ServiceWorker environment

Which problem is this PR solving?

The current implementation of OTLP exporter is not working in ServiceWorker of browsers, due to XMLHttpRequest and window are not supported in the environment.

This PR makes OTLP exporter works in the ServiceWorker.

Short description of the changes

Add new spans/metrics sender that uses fetch()
Use _globalThis from the core library instead of window

Type of change

[x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

[X] Add unit tests same as existing for XMLHttpRequest, and these are passed all
[X] Works on actual service worker environment

Checklist:

[x] Followed the style guidelines of this project
[x] Unit tests have been added
[ ] ~~Documentation has been updated~~ (skip: No breaking changes, no exported API, keep full compatibility)

Jan 17 '23 15:01 sugi

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: sugi / name: Tatsuki Sugiura (f654e84d23739b9123d76d684e166feea116e863, d5951df657da5235e2420dae14970a021d816b26, a0aa0c9bf300fbb6a575614c9e0e43a95a8b34e5, 7a367406a220b26c1b182a2939c39bf1e41cbc74, 7f6698248dd6e9f42d2938f2b3bb793895ea54ef, 5a714bf62e1229f24d687ee0a283cce4cff67618, e5f04545e66a7570239d31e4e3334db440f5e270, 8a4311e25db521b8ac2a1006f153e80593c44fbc, 692715464912bb639b964cd42a8b36b57159c29c, a613a5b42cb4fcc328d64746f87282a00c658bd0, 11cb4a3c1bc2dd39eb37c0f5573872319af0df93, af91d48e1be4c1b06badd75dd6eb1ca49e3f8fa9, 1dda10676243091c63238bffd56af1e894939d28, c52538e07405482d74049bdc52b1e80779b2ee23, b89476b15a1dc124d1116150f62609fddc82ec1c, 8187bd0b0bfb64a29d4a50a03b851ee3f7858de3, 75e8ed7754f9debf11e26be0e8ea58032dad0ad4, 975f9f985d715f85fc3f0f75b76392e4afb03b99, da0c60774af9d43653451009b347cb60b31e2779, f39ea115f124431bdc3357e90d3965ab18ffd506, fbba203cb9e959c2a5b40a56d8bd4510ad588297, f40977352ff2d948abfcdd414d1b2036bf2e12fc, 16909ad3aabdc4fe059c5717e61bc112811d0fc3, 8bda0e1331fa87e048f4e1b0ccd8559102805db9, 72db4f2fba6e9e8f90a33351b71bdaf04da0940b, 14975a8415bcdb2cca226e60d1861d45cbdfc384, 2b22dc7a2dcf3ab7210b41a370455b921299f017, 53595195b885e261040d91f94be63d61c9423feb, 6918bbc691efeae719aa7828ec69c549b1b9bf86, 45a8f76ddabb60e80e7eaaabb09668f72123b462)
:white_check_mark: login: pichlermarc / name: Marc Pichler (2b5db3e10e2c447489216aa6d8d96727fac99fc6, 7dd9675340aa379704f2049205f11bffa071cfc7)

Jan 17 '23 15:01 linux-foundation-easycla[bot]

Codecov Report

Merging #3542 (45a8f76) into main (f86251d) will decrease coverage by 0.27%. Report is 4 commits behind head on main. The diff coverage is 47.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3542      +/-   ##
==========================================
- Coverage   92.42%   92.15%   -0.27%     
==========================================
  Files         330      330              
  Lines        9520     9577      +57     
  Branches     2031     2048      +17     
==========================================
+ Hits         8799     8826      +27     
- Misses        721      751      +30

Files	Coverage Δ
...metry-exporter-zipkin/src/platform/browser/util.ts	`82.14% <68.75%> (-5.67%)`	:arrow_down:
...es/otlp-exporter-base/src/platform/browser/util.ts	`36.11% <39.53%> (+1.26%)`	:arrow_up:

... and 1 file with indirect coverage changes

Jan 18 '23 17:01 codecov[bot]

Oh? Another test fails in CI. I'll check it. Please give me more 1-2 days.

Jan 18 '23 19:01 sugi

@open-telemetry/javascript-approvers I confirmed all tests are passed locally and added more tests. This PR is ready to review now.

BTW, How can I sign added commits by EasyCLA? I cannot find the way.

Jan 20 '23 15:01 sugi

@open-telemetry/javascript-approvers I confirmed all tests are passed locally and added more tests. This PR is ready to review now.

BTW, How can I sign added commits by EasyCLA? I cannot find the way.

not sure what you mean? EasyCLA doesn't add any commits

Jan 25 '23 16:01 dyladan

@sugi sorry for the slow cycle here this fell off my radar. I think for now you can go ahead with the PR and the unified http client can come as a later enhancement.

Mar 03 '23 19:03 dyladan

@dyladan Thank you for your reply!

I'm happy to hear that. So... What should I do next? Is it OK to merge the upstream master into this PR, then notify you for ready?

Mar 05 '23 17:03 sugi

I'm happy to hear that. So... What should I do next? Is it OK to merge the upstream master into this PR, then notify you for ready?

Yes, merging the upstream master and then notifying sounds good. Thank you :slightly_smiling_face:

Mar 06 '23 05:03 pichlermarc

@pichlermarc @dyladan OK. Now I merged with upstream/master and implemented a retry feature to the fetch sender that has been introduced to XHR while this PR is open.

Would you please review this again?

Mar 06 '23 14:03 sugi

@sugi any update? :slightly_smiling_face: Let me know if there's anything I can do to help this PR along :slightly_smiling_face:

Apr 13 '23 08:04 pichlermarc

@sugi any update? slightly_smiling_face Let me know if there's anything I can do to help this PR along slightly_smiling_face

Ouch... I'm sorry. I'll update and merge upstream again this weekend. Please give time for me.

Apr 17 '23 15:04 sugi

Ouch... I'm sorry. I'll update and merge upstream again this weekend. Please give time for me.

No worries, this is great work - looking forward to merging this soon. :slightly_smiling_face:

Apr 18 '23 04:04 pichlermarc

I'm looking at using this in a worker environment - was happy to see a PR up for this, thanks!

May 29 '23 16:05 jgodson

Ouch... sorry. I forgot to leave a comment on the last merge. I'll do it again this weekend.

Jun 01 '23 06:06 sugi

I suggest we just drop support for XHR in the exporters. Discussion: #3845

Jun 03 '23 16:06 llc1123

@pichlermarc I merged the main upstream branch and resolved the conflict.

@llc1123

I suggest we just drop support for XHR in the exporters. Discussion: https://github.com/open-telemetry/opentelemetry-js/issues/3845

I indeed agree with you; XHR should be dropped. However, it should be done by another PR.

Jun 04 '23 15:06 sugi

👋 @sugi came across this PR when trying to implement metrics for cloudflare workers and would love to get this merged upstream. Anything I can help with to push this along?

Sep 12 '23 18:09 ChanChar

@ChanChar Ouch. I'm sorry.

@MSNev I'll merge upstream and reply your suggestions in this weekend.

Sep 28 '23 04:09 sugi

I'm sorry for being late by getting sick. Now I'm merging upstream and making fixes to catch up the changes.

Oct 16 '23 14:10 sugi

@MSNev @sugi is there anything I can do to get this across the line? Very keen to use this in a few projects.

Nov 02 '23 11:11 cdloh

@sugi, please update the branch and resolve any issues. The basic change looks good to me, so I've approved the test run so that in the meantime it will identify if there are any test failures.

Nov 02 '23 19:11 MSNev

@sugi, please update the branch and resolve any issues. The basic change looks good to me, so I've approved the test run so that in the meantime it will identify if there are any test failures.

OK. I'll update 1-2 days.

Nov 03 '23 15:11 sugi

@MSNev I has updated the branch with upstream main, fixed test issues and check all test passed on local machine.

Nov 04 '23 14:11 sugi

Thank you for running workflows.

And I'm sorry to forge to run lint fix.

I updated files with npm run lint -- -- --fix and merge current upstream branch now.

Nov 14 '23 16:11 sugi

@sugi do you have time to finish off the last few tests etc?

Jan 31 '24 15:01 cdloh

Thank you for your mention!

OK. I'll do tomorrow.

Feb 01 '24 11:02 sugi

I confirmed and reproduced the error on CI after merging the current upstream. I'm now fixing it...

Feb 02 '24 17:02 sugi

I confirmed and reproduced the error on CI after merging the current upstream.

This is my misunderstanding. A merge miss caused my error.

Anyway, I updated the branch against upstream and confirmed all tests and lints are passed on my local machine. Would you please run tests on CI? @cdloh

I'm wondering if the test may fail on CI because I remember the tests of the previous push were also successful on my local machine.

@cdloh

Feb 03 '24 16:02 sugi

Oh..., the coverage test has failed. I'll fix it this weekend.

Feb 07 '24 13:02 sugi

I added tests for ZipkinExporter in the same manner to fix the coverage issue. Could you please run CI again? @cdloh

BTW, I found some bugs in the original tests.

All of the browser tests of ZipkinExporter for globalErrorHandler have two problems;

These tests do NOT call done() with suing async timer. Therefore, it will be successful even if it is actually failed.
ZipkinExporter is NOT using globalErrorHandler currently.
- So, remove these tests or implement the use of the handler.

I skipped adding the test of globalErrorHandler for fetch and left the tests described above. It should be out of the scope of this PR.

Feb 11 '24 14:02 sugi

opentelemetry-js opentelemetry-js copied to clipboard

feat(otlp-exporter-base): Add fetch sender for ServiceWorker environment

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

Codecov Report

opentelemetry-js
opentelemetry-js copied to clipboard