-
Notifications
You must be signed in to change notification settings - Fork 805
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: use Blob in sendBeacon to add application/json type #2336
Conversation
… sendBeacon to add type application/json
Codecov Report
@@ Coverage Diff @@
## main #2336 +/- ##
==========================================
- Coverage 92.64% 92.62% -0.03%
==========================================
Files 137 137
Lines 4974 4974
Branches 1047 1047
==========================================
- Hits 4608 4607 -1
- Misses 366 367 +1
|
This is not normally a response from sendBeacon() as the error indicates that you are sending credentials with "include" which you can't do via sendBeacon(). To me this seems like there is a sendBeacon() polyfill that is using fetch under the covers with the "credentials" set to "include" (https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) |
But beacon API is implemented in browser (https://developer.mozilla.org/en-US/docs/Web/API/Beacon_API) |
So in short changing to cors mode with an OPTIONS call is not going to change the issue. As the real issue is detailed in the error message
So the correct (and only IMHO) fix for this is that the response (when the URL is going across origins (domains)) is that the server sending the response need to include the "Access-Control-Allow-Credentials", because as you pointed out we can't change the processing model. |
Ok.
The Access-Control-Allow-Credentials problem is in the server (collector) and for the moment I can't do anything about this in this fix (or have a patch version of the collector 0.29.1 ? 😊 ) This fix add for the moment the good type in sendBeacon. |
You could just build a collector with the fix and roll it out when you need it. As @MSNev said this should not fix the issue since JSON is consired safe by beacon API and will still result in a CORS call being made |
Collector 0.30.0 is here https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.30.0 I tried this collector with and without this fix. Both of them work now. So I think this fix is useless... Am I right? |
Technically, anyone could call this exported function with any type of string encoding (but it will probably always be JSon), so if we do use Blob (it's not necessary to do so), then perhaps we should also allow an optional contentType param (which defaults to json)??? |
Yeah i think that should be the way to go, @jufab are you good with that ? |
@vmarchaud yes, but not sure what am I suppose to do... 😊 |
packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/test/browser/CollectorTraceExporter.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts
Outdated
Show resolved
Hide resolved
Please mark conversations as resolved when they are resolved. It helps us to know when a PR is mergeable |
Which problem is this PR solving?
Short description of the changes
During test, i had some troubles with CORS.
Preflight is ok but i have this issue :
Access to resource at 'http://localhost:55681/v1/traces' from origin 'http://localhost:8090' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Credentials' header in the response is '' which must be 'true' when the request's credentials mode is 'include'.
After some research, the problem is in collector (ACAC is empty). But, it's now ok for the next release. (see this commit
open-telemetry/opentelemetry-collector@1ab4d95#diff-1fadfcd3b4194004717150126d27e8a32fdaa8292ca99e81a937d26abe0998fa)
So, I can't validate this fix without the next release....