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: use Blob in sendBeacon to add application/json type #2336

Merged
merged 14 commits into from
Aug 10, 2021

Conversation

jufab
Copy link
Contributor

@jufab jufab commented Jul 8, 2021

Which problem is this PR solving?

Short description of the changes

  • Add a Blob instead string body to transform text/plain to application/json and be ok with http otel collector 0.29.0

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

@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #2336 (5ccc02e) into main (eb3cd50) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
...ry-exporter-collector/src/CollectorExporterBase.ts 92.15% <100.00%> (ø)
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️

@MSNev
Copy link
Contributor

MSNev commented Jul 8, 2021

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'

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)

@jufab
Copy link
Contributor Author

jufab commented Jul 8, 2021

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'

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)
Not sure we can change the processing model? (https://w3c.github.io/beacon/#sec-processing-model)

@MSNev
Copy link
Contributor

MSNev commented Jul 8, 2021

But beacon API is implemented in browser (https://developer.mozilla.org/en-US/docs/Web/API/Beacon_API)
I'm not saying that it isn't, I'm saying that the only time I normally see this is when a polyfill is being used.

Not sure we can change the processing model? (https://w3c.github.io/beacon/#sec-processing-model)
And I'm not suggesting that we are, should or need to, if you read the spec

    1. Let corsMode be "no-cors".
  • 6.3. If contentType is not null: (and not in safe list) (which json is not) set coreMode to "cors" (Which will force an OPTIONS call)

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

The value of the 'Access-Control-Allow-Credentials' header in the >>>response<<<

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.

@jufab
Copy link
Contributor Author

jufab commented Jul 8, 2021

Ok.
So, we say the same thing about server when I say

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

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.

@vmarchaud
Copy link
Member

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 ? blush )

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

@jufab
Copy link
Contributor Author

jufab commented Jul 14, 2021

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?

@vmarchaud
Copy link
Member

@jufab Well i think it still make sense to use Blob so by default we sent the correct content-type header, WDYT @MSNev ?

@MSNev
Copy link
Contributor

MSNev commented Jul 19, 2021

@jufab Well i think it still make sense to use Blob so by default we sent the correct content-type header, WDYT @MSNev ?

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

@vmarchaud
Copy link
Member

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 ?

@jufab
Copy link
Contributor Author

jufab commented Jul 22, 2021

@vmarchaud yes, but not sure what am I suppose to do... 😊

@jufab jufab requested a review from rauno56 as a code owner July 26, 2021 22:11
@vmarchaud vmarchaud added the enhancement New feature or request label Aug 7, 2021
@dyladan dyladan requested a review from a team August 9, 2021 15:29
@dyladan dyladan changed the title use Blob in sendBeacon to add application/json type feat: use Blob in sendBeacon to add application/json type Aug 9, 2021
@dyladan
Copy link
Member

dyladan commented Aug 9, 2021

Please mark conversations as resolved when they are resolved. It helps us to know when a PR is mergeable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Traces send by http protocol fail in new Otel collector receiver
6 participants