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

CORS requirements on web exporters with navigator.sendBeacon #3062

Closed
1 task done
legendecas opened this issue Jun 27, 2022 · 16 comments
Closed
1 task done

CORS requirements on web exporters with navigator.sendBeacon #3062

legendecas opened this issue Jun 27, 2022 · 16 comments
Assignees
Labels

Comments

@legendecas
Copy link
Member

legendecas commented Jun 27, 2022

  • This only affects the JavaScript OpenTelemetry library

navigator.sendBeacon with non-simple content-type header like "application/json" requires CORS checks like Access-Control-Allow-Headers, Access-Control-Allow-Methods, Access-Control-Allow-Origin, and Access-Control-Allow-Credentials to be set with appropriate values on preflight requests.

Notably, sendBeacon initialize the request with credential mode set to true with a non-simple content-type, which means the server has to respond to preflight requests with Access-Control-Allow-Credentials true too. This is not configurable nor an option exposed with the sendBeacon API. That is to say, credentials like cookies, authorization headers or TLS client certificates are sent with sendBeacon in this case. However, the credential mode is configurable with fetch and XHR APIs.

We have the following use cases on sendBeacon:

  • OTLP exporter: Sent with application/json content type, not tested if OTLP server responds with proper CORS headers.
  • Zipkin exporter: actually, the Zipkin server accepts JSON data sent over content-type as text/plain, so the problem does not apply to Zipkin backends.
  • Zipkin exporter sending to Jaeger backend: Not supported without a reverse proxy, Jaeger server rejects text/plain content-type, and doesn't respond with Access-Control-Allow-Credentials CORS header. (issue: exporter-zipkin does not send Content-Type in default config #1727)

Currently, we don't provide options on how those data are sent, like sent with fetch (environments that don't support XHR or beacon), XHR or beacon, and the CORS credential modes like omit or include. I think it may be worthwhile to add a package that shares all the necessary setups on the web platform.

@legendecas
Copy link
Member Author

legendecas commented Jun 27, 2022

For the Zipkin compatible mode of the Jaeger server, I'm if it is behaving in an expected way. Maybe @yurishkuro can chime in here?

@yurishkuro
Copy link
Member

Zipkin receiver in Jaeger has a config option for cors.

@dyladan
Copy link
Member

dyladan commented Jun 27, 2022

In https://www.jaegertracing.io/docs/1.35/cli/ I see the allowed headers and allowed origins, but I don't see the allow credentials response header. Is there somewhere else I should look for cors configs?

@legendecas
Copy link
Member Author

legendecas commented Jun 27, 2022

Zipkin receiver in Jaeger has a config option for cors.

Yes, but as stated in the OP it is missing Access-Control-Allow-Credentials CORS header. This behavior is as same as Zipkin's. Yet, Zipkin supports interpreting simple content types like text/plain so navigator.sendBeacon (the API Zipkin exporter uses by default) can successfully send data to Zipkin servers. However, Jaeger (tested with 1.35) does not support simple content types and rejects requests that are not typed as MIME application/json.

With MIME types like application/json, navigator.sendBeacon requires the server to respond with CORS headers, and specifically Access-Control-Allow-Credentials need to be true.

@yurishkuro
Copy link
Member

but I don't see the allow credentials response header

Seems like easily added if needed. The full Options struct has a lot more fields than what Jaeger CLI flags currently support - https://github.com/rs/cors/blob/da52b0701de54d82f71776d634d4183b69c3a915/cors.go#L32

@yurishkuro
Copy link
Member

Having said that, maybe it's better to implement this in OTLP receiver, then Jaeger could use that as well. I prefer to spend less effort on supporting legacy formats like Zipkin.

@iShawnWang
Copy link

This works fine for me for now ~

import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http';
const exporter = new OTLPTraceExporter({
  headers: {}, // magic here !!!
  url: '<your ot receiver host>/v1/traces',
});

After digging into the source code with debugger i found this, with the header: {} option, ot sender will use xhr instead of sendBeacon !

image

so without credentials: 'include',
this config on ot receiver will work

receivers: 
  otlp:
    protocols:
      grpc:
      http:
        cors:
          allowed_origins: ["*"]
          allowed_headers: ["*"]

@dyladan
Copy link
Member

dyladan commented Jul 20, 2022

Maybe we should include a way to force xhr or sendbeacon by config

@legendecas
Copy link
Member Author

Maybe we should include a way to force xhr or sendbeacon by config

Yeah, this is what I have in my mind too. I'm thinking about introducing a new @opentelemetry/core-web package for web utilities that need to be shared across various web packages. Currently, those utils are located in the @opentelemetry/sdk-trace-web (e.g. https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-web/src/utils.ts#L201).

@t2t2
Copy link
Contributor

t2t2 commented Jul 21, 2022

Maybe consider having xhr (or fetch considering how it's available in node v18+, deno v1 and usually polyfilled in IE) as the default method in general and navigator.sendBeacon as backup when in browser document is being unloaded?

  • sendBeacon has a 64KB limit. In firefox this is per sendBeacon call (eg can do 64KB + 64KB + 64KB), but in chrome this limit also applies for requests queue so 64 KB + won't send + won't send

image

  • It's common to send RUM data directly to a vendor endpoint (eg. https://bam.vendor-data.net/). There is also a rule in EasyPrivacy list used by content blockers like adblock, ublock origin and such:
! Block ping
$ping,third-party

What this rule means is all sendBeacon calls to third-party domains should be blocked, meaning all data from users with this adblock list enabled would be missing

  • More of a future looking but should any format introduce support for retrying sending data, it would be necessary to read response from server

@iShawnWang
Copy link

In my opinion, xhr or fetch is more predictable ~

@github-actions
Copy link

This issue 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 Sep 26, 2022
@github-actions
Copy link

This issue was closed because it has been stale for 14 days with no activity.

@legendecas legendecas reopened this Nov 4, 2022
@legendecas legendecas self-assigned this Nov 4, 2022
@legendecas legendecas removed the stale label Nov 4, 2022
@github-actions
Copy link

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

github-actions bot commented Apr 3, 2023

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

This issue was closed because it has been stale for 14 days with no activity.

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

No branches or pull requests

5 participants