feat: use Blob in sendBeacon to add application/json type

[jufab]

Which problem is this PR solving?

• Fix issue Traces send by http protocol fail in new Otel collector receiver #2321

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]

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]

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]

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]

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

• 5. 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]

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]

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]

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]

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

[MSNev]

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

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]

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

[dyladan]

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