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

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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

sugi
Copy link

@sugi sugi commented Jan 17, 2023

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
Copy link

linux-foundation-easycla bot commented Jan 17, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@sugi sugi force-pushed the service-worker-support branch from 255c6a3 to f654e84 Compare January 17, 2023 15:55
@sugi sugi marked this pull request as ready for review January 17, 2023 16:04
@sugi sugi requested a review from a team January 17, 2023 16:04
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Attention: Patch coverage is 47.45763% with 31 lines in your changes missing coverage. Please review.

Project coverage is 92.15%. Comparing base (f86251d) to head (45a8f76).
Report is 419 commits behind head on main.

Files with missing lines Patch % Lines
...es/otlp-exporter-base/src/platform/browser/util.ts 39.53% 26 Missing ⚠️
...metry-exporter-zipkin/src/platform/browser/util.ts 68.75% 5 Missing ⚠️
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 with missing lines 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
Copy link
Author

sugi commented Jan 18, 2023

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

@pichlermarc pichlermarc linked an issue Jan 19, 2023 that may be closed by this pull request
@sugi sugi marked this pull request as draft January 20, 2023 14:56
@sugi sugi marked this pull request as ready for review January 20, 2023 14:56
@sugi
Copy link
Author

sugi commented Jan 20, 2023

@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
Copy link
Member

dyladan commented Jan 25, 2023

@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

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks generally good and I appreciate all the tests

Comment on lines 43 to 47
} else if (typeof XMLHttpRequest === 'function') {
this.sendMethod = 'xhr';
} else {
this.sendMethod = 'fetch';
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@sugi sugi Jan 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

@legendecas legendecas Jan 30, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@MSNev MSNev Oct 17, 2023

Choose a reason for hiding this comment

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

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)

);
}
})
.catch((e: Error) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Oh, thank you.

I fixed it (7a36740)

Copy link
Author

@sugi sugi left a comment

Choose a reason for hiding this comment

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

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.

);
}
})
.catch((e: Error) => {
Copy link
Author

Choose a reason for hiding this comment

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

Oh, thank you.

I fixed it (7a36740)

Comment on lines 43 to 47
} else if (typeof XMLHttpRequest === 'function') {
this.sendMethod = 'xhr';
} else {
this.sendMethod = 'fetch';
}
Copy link
Author

@sugi sugi Jan 28, 2023

Choose a reason for hiding this comment

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

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
Copy link
Member

dyladan commented Mar 3, 2023

@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
Copy link
Author

sugi commented Mar 5, 2023

@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
Copy link
Member

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 🙂

@sugi
Copy link
Author

sugi commented Mar 6, 2023

@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 pichlermarc linked an issue Mar 7, 2023 that may be closed by this pull request
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, 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.

@pichlermarc
Copy link
Member

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

@sugi
Copy link
Author

sugi commented Nov 4, 2023

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

@sugi
Copy link
Author

sugi commented Nov 14, 2023

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
Copy link

cdloh commented Jan 31, 2024

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

@sugi
Copy link
Author

sugi commented Feb 1, 2024

Thank you for your mention!

OK. I'll do tomorrow.

@sugi
Copy link
Author

sugi commented Feb 2, 2024

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

@sugi
Copy link
Author

sugi commented Feb 3, 2024

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
Copy link
Author

sugi commented Feb 7, 2024

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

@sugi
Copy link
Author

sugi commented Feb 11, 2024

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
Copy link
Author

sugi commented Feb 11, 2024

  • 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
Copy link
Member

dyladan commented Feb 14, 2024

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

@sugi
Copy link
Author

sugi commented Feb 14, 2024

@dyladan yes. please review.

@davidstrouk
Copy link

@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
Copy link
Author

sugi commented Mar 31, 2024

@dyladan what should I do next?

@davidstrouk
Copy link

@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
Copy link

We need it ASAP please merge

@pichlermarc
Copy link
Member

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
Copy link

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
Copy link
Member

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.

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet