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

sugi · 2023-01-17T15:52:39Z

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

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

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

Checklist:

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

linux-foundation-easycla · 2023-01-17T15:52:46Z

The committers listed above are authorized under a signed CLA.

✅ login: sugi / name: Tatsuki Sugiura (f654e84, d5951df, a0aa0c9, 7a36740, 7f66982, 5a714bf, e5f0454, 8a4311e, 6927154, a613a5b, 11cb4a3, af91d48, 1dda106, c52538e, b89476b, 8187bd0, 75e8ed7, 975f9f9, da0c607, f39ea11, fbba203, f409773, 16909ad, 8bda0e1, 72db4f2, 14975a8, 2b22dc7, 5359519, 6918bbc, 45a8f76)
✅ login: pichlermarc / name: Marc Pichler (2b5db3e, 7dd9675)

…ent.

codecov · 2023-01-18T17:10:27Z

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%)`	⬇️
...es/otlp-exporter-base/src/platform/browser/util.ts	`36.11% <39.53%> (+1.26%)`	⬆️

... and 1 file with indirect coverage changes

sugi · 2023-01-18T19:30:48Z

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

…ests.

sugi · 2023-01-20T15:08:56Z

@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.

dyladan · 2023-01-25T16:29:27Z

@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

dyladan

Looks generally good and I appreciate all the tests

dyladan · 2023-01-25T16:31:45Z

experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts

+    } else if (typeof XMLHttpRequest === 'function') {
+      this.sendMethod = 'xhr';
+    } else {
+      this.sendMethod = 'fetch';
+    }


Is there a reason to prefer xhr over fetch? I'm not a browser expert so I'm not sure what the advatages are of one over another.

In this PR, I just prefer xhr to keep full compatibility with the current implementation.

I think fetch is better in modern browsers in general.
In addition, fetch may also be preferred over useBeacon when keepalive fetch is supported.
https://docs.google.com/document/d/1iHJtFa3jOo5n9QXHb6Ok5nK8kavXSk2DrLoubPWi9ys/edit

However, this PR is focused on just enabling opentelemetry-js work in a service/web worker environment.

I'll make a change to the order if you'd like to use fetch by default.

Deno and Cloudflare Workerd dont' support xhr inherently too. Only fetch is available on those platforms. Feature detection is a good choice to support those platforms.

However, I've been thinking that we may need to add a common internal http client that can be shared across exporters -- like the zipkin exporter. Also, adding a new base http implementation currently requires dedicated work to support features like retrying #3207.

I'd propose migrating this to be an internal tool and find a minimum http interface for exporters so that we can reduce the duplication across exporters: #3577.

Oh. I agree with creating a new shared exporter is better.
I'm OK with any method if opentelemetry-js works in a worker environment.

So... should I ask you to determine whether to drop this PR or not?
@legendecas

Sorry for the delayed response. I think this PR is still a good work and can be landed. #3577 updates more than OTLP exporters and needs more tailoring.

I think "defaulting" to XHR as well rather than sendBeacon (unless you know this is occurring during page unload -- which we dont' currently) is a better option as it is very possible that when the payload is larger than 64kb (or the total outstanding payload is > 64kb) you won't be able to send anything.

Which depending on whether there are or are not headers will occur (in the former case)

dyladan · 2023-01-25T17:01:08Z

experimental/packages/otlp-exporter-base/src/platform/browser/util.ts

+        );
+      }
+    })
+    .catch((e: Error) => {


This catch block may also catch errors from the then handler. I think possibly you meant to only catch errors from the fetch itself? If so, I would move this error handler to be the second argument of then

Oh, thank you.

I fixed it (7a36740)

sugi

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

I'm worried it is OK that EasyCLA comments about only the first commit of this PR, or not.
#3542 (comment)

Anyway, I updated the review point.

sugi · 2023-01-28T10:24:32Z

experimental/packages/otlp-exporter-base/src/platform/browser/util.ts

+        );
+      }
+    })
+    .catch((e: Error) => {


Oh, thank you.

I fixed it (7a36740)

sugi · 2023-01-28T12:52:09Z

experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts

+    } else if (typeof XMLHttpRequest === 'function') {
+      this.sendMethod = 'xhr';
+    } else {
+      this.sendMethod = 'fetch';
+    }


In this PR, I just prefer xhr to keep full compatibility with the current implementation.

I think fetch is better in modern browsers in general.
In addition, fetch may also be preferred over useBeacon when keepalive fetch is supported.
https://docs.google.com/document/d/1iHJtFa3jOo5n9QXHb6Ok5nK8kavXSk2DrLoubPWi9ys/edit

However, this PR is focused on just enabling opentelemetry-js work in a service/web worker environment.

I'll make a change to the order if you'd like to use fetch by default.

dyladan · 2023-03-03T19:51:55Z

@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.

sugi · 2023-03-05T17:07:37Z

@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?

pichlermarc · 2023-03-06T05:59:10Z

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 🙂

…ith xhr.

sugi · 2023-03-06T14:05:05Z

@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?

pichlermarc

Looks good, however there's a lot of duplication in the implementation and tests that makes this PR very hard to review, and that I fear will make it even harder to maintain the exporters in the future.

If we want to merge this PR as it is now, we should immediately look into reducing duplication.

experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts

...ckages/opentelemetry-exporter-metrics-otlp-http/test/browser/CollectorMetricExporter.test.ts

experimental/packages/exporter-trace-otlp-http/package.json

experimental/packages/otlp-exporter-base/package.json

experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts

pichlermarc · 2023-04-13T08:25:51Z

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

sugi · 2023-11-03T15:40:34Z

@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.

sugi · 2023-11-04T14:22:25Z

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

sugi · 2023-11-14T16:34:57Z

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.

cdloh · 2024-01-31T15:04:07Z

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

sugi · 2024-02-01T11:46:09Z

Thank you for your mention!

OK. I'll do tomorrow.

sugi · 2024-02-02T17:11:22Z

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

sugi · 2024-02-03T16:23:06Z

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

sugi · 2024-02-07T13:24:01Z

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

Signed-off-by: Tatsuki Sugiura <sugi@nemui.org>

sugi · 2024-02-11T14:16:53Z

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.

sugi · 2024-02-11T16:33:47Z

ZipkinExporter is NOT using globalErrorHandler currently.

So, remove these tests or implement the use of the handler.

Ouch. This point includes my mistake.
ZipkinExporter calls globalErrorHandler on some sate of errors. However, it's not called with tested cases.

Anyway, current tests for globalErrorHandler have not worked expectedly.

dyladan · 2024-02-14T16:16:17Z

@sugi is this ready for review? If not please mark as draft

sugi · 2024-02-14T17:05:10Z

@dyladan yes. please review.

davidstrouk · 2024-03-27T15:20:14Z

@sugi @dyladan any chance we can merge this PR?

We are encountering the same issue and this is one of the last blockers for migrating our Chrome extension to manifest v3.

Would be happy to assist in the matter if needed 😄 Thanks! 🙏🏼

sugi · 2024-03-31T15:34:52Z

@dyladan what should I do next?

davidstrouk · 2024-05-29T10:11:39Z

@dyladan @legendecas @pichlermarc @MSNev could anyone please address this PR? I would be happy to assist with anything.

We disabled logging in Manifest v3 extension, and would be happy to bring it back ASAP 🙏🏼 thanks

hareltus · 2024-05-29T10:44:44Z

We need it ASAP please merge

pichlermarc · 2024-06-10T13:08:17Z

We're working on refactoring transports for the OTLP exporters to ensure that we can maintain this feature (and other features) long-term.

I'll let you know when we can move forward for this. I'm very sorry for the delay, but the exporters (and their tests) are not in shape for adding new features at this point in time. See #4631 (comment) and the issues liked therein.

ilanmai21 · 2024-06-15T20:02:19Z

We're working on refactoring transports for the OTLP exporters to ensure that we can maintain this feature (and other features) long-term.

I'll let you know when we can move forward for this. I'm very sorry for the delay, but the exporters (and their tests) are not in shape for adding new features at this point in time. See #4631 (comment) and the issues liked therein.

Do you have estimation for the time it will take? It is very important for us, we can see logs currently

pichlermarc · 2024-06-18T14:16:44Z

We're working on refactoring transports for the OTLP exporters to ensure that we can maintain this feature (and other features) long-term.
I'll let you know when we can move forward for this. I'm very sorry for the delay, but the exporters (and their tests) are not in shape for adding new features at this point in time. See #4631 (comment) and the issues liked therein.

Do you have estimation for the time it will take? It is very important for us, we can see logs currently

It is very hard to estimate how long things take in open-source land as it's very dependent on what else is going on in the repo.
I'll keep updating #4631 as work goes on.

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

f654e84

…ent.

sugi force-pushed the service-worker-support branch from 255c6a3 to f654e84 Compare January 17, 2023 15:55

sugi marked this pull request as ready for review January 17, 2023 16:04

sugi requested a review from a team January 17, 2023 16:04

pichlermarc mentioned this pull request Jan 18, 2023

Update OTLPExporterBrowserBase.ts #3546

Closed

4 tasks

kohlikohl mentioned this pull request Jan 18, 2023

OTLPExporterBrowserBase not worker compatible #3545

Open

pichlermarc linked an issue Jan 19, 2023 that may be closed by this pull request

OTLPExporterBrowserBase not worker compatible #3545

Open

sugi added 2 commits January 20, 2023 23:40

feat(otlp-exporter-base): fix send method detect condition and more t…

d5951df

…ests.

Merge remote-tracking branch 'upstream/main' into service-worker-support

a0aa0c9

sugi marked this pull request as draft January 20, 2023 14:56

sugi marked this pull request as ready for review January 20, 2023 14:56

dyladan reviewed Jan 25, 2023

View reviewed changes

sugi added 2 commits January 28, 2023 19:36

fix(otlp-exporter-base): improve fetch error handling.

7a36740

Merge remote-tracking branch 'upstream/main' into service-worker-support

7f66982

sugi commented Jan 28, 2023

View reviewed changes

sugi added 3 commits March 6, 2023 16:36

Merge remote-tracking branch 'upstream/main' into service-worker-support

5a714bf

feat(otlp-exporter-base): implement retry on fetch sender to compat w…

e5f0454

…ith xhr.

chore: lint style fix.

8a4311e

pichlermarc linked an issue Mar 7, 2023 that may be closed by this pull request

OTLP exporters shouldn't use window as it won't work in web workers #3168

Closed

pichlermarc reviewed Mar 7, 2023

View reviewed changes

sugi added 5 commits November 4, 2023 18:02

Merge remote-tracking branch 'upstream/main' into service-worker-support

fbba203

chore: add diag message for test.

f409773

chore: import current package-lock.json (adding fetch-mock)

16909ad

fix: Move entry to unreleased.

8bda0e1

chore: update submodules to match with upstream/main.

72db4f2

sugi added 2 commits November 15, 2023 01:31

chore: fix by lint.

14975a8

Merge remote-tracking branch 'upstream/main' into service-worker-support

2b22dc7

Merge remote-tracking branch 'upstream/main' into service-worker-support

5359519

sugi added 2 commits February 11, 2024 15:05

Merge remote-tracking branch 'upstream/main' into service-worker-support

6918bbc

chore: Add fetch mode test for zipkinExporter.

45a8f76

Signed-off-by: Tatsuki Sugiura <sugi@nemui.org>

david-luna mentioned this pull request Apr 17, 2024

TraceExporter for fetch-only environment #4631

Open

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

Are you sure you want to change the base?

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

Conversation

sugi commented Jan 17, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Jan 17, 2023 • edited Loading

codecov bot commented Jan 18, 2023 • edited Loading

Codecov Report

sugi commented Jan 18, 2023 • edited Loading

sugi commented Jan 20, 2023

dyladan commented Jan 25, 2023

dyladan left a comment

Choose a reason for hiding this comment

dyladan Jan 25, 2023

Choose a reason for hiding this comment

sugi Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

legendecas Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

sugi Jan 31, 2023

Choose a reason for hiding this comment

legendecas Mar 24, 2023

Choose a reason for hiding this comment

MSNev Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

dyladan Jan 25, 2023

Choose a reason for hiding this comment

sugi Jan 28, 2023

Choose a reason for hiding this comment

sugi left a comment

Choose a reason for hiding this comment

sugi Jan 28, 2023

Choose a reason for hiding this comment

sugi Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

dyladan commented Mar 3, 2023

sugi commented Mar 5, 2023

pichlermarc commented Mar 6, 2023

sugi commented Mar 6, 2023

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc commented Apr 13, 2023

sugi commented Nov 3, 2023

sugi commented Nov 4, 2023

sugi commented Nov 14, 2023 • edited Loading

cdloh commented Jan 31, 2024

sugi commented Feb 1, 2024 • edited Loading

sugi commented Feb 2, 2024 • edited Loading

sugi commented Feb 3, 2024

sugi commented Feb 7, 2024

sugi commented Feb 11, 2024

sugi commented Feb 11, 2024

dyladan commented Feb 14, 2024

sugi commented Feb 14, 2024

davidstrouk commented Mar 27, 2024

sugi commented Mar 31, 2024

davidstrouk commented May 29, 2024

hareltus commented May 29, 2024

pichlermarc commented Jun 10, 2024

ilanmai21 commented Jun 15, 2024

pichlermarc commented Jun 18, 2024

sugi commented Jan 17, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jan 17, 2023 •

edited

Loading

codecov bot commented Jan 18, 2023 •

edited

Loading

sugi commented Jan 18, 2023 •

edited

Loading

sugi Jan 28, 2023 •

edited

Loading

legendecas Jan 30, 2023 •

edited

Loading

MSNev Oct 17, 2023 •

edited

Loading

sugi Jan 28, 2023 •

edited

Loading

sugi commented Nov 14, 2023 •

edited

Loading

sugi commented Feb 1, 2024 •

edited

Loading

sugi commented Feb 2, 2024 •

edited

Loading