search

instana / weasel

Gather end-user browser performance data
https://www.instana.com
MIT License
40 stars 5 forks source link

Allow for alternative context propagation headers #4

Open felixbarny opened 7 years ago

felixbarny commented 7 years ago

In https://github.com/instana/weasel/blob/master/lib/hooks/XMLHttpRequest.js#L172, Instana specific context propagation headers are used. It would be nice if it was configurable which headers should be used. Maybe even by just specifying the "propagation style" like instana or b3.

Where would be the appropriate place to do that?

As a workaround, I'm currently replacing the Instana headers with B3 headers in the build.

bripkens commented 7 years ago

Hey @felixbarny,

there is currently no appropriate way/file to do this. How about we add an option to vars.js like setCorrelationHeaders? Users of Weasel need to change the content of vars.js anyway in order set an appropriate nameOfLongGlobal and a default reportingUrl.

WDYT?

Maybe even by just specifying the "propagation style" like instana or b3.

The compilation result of weasel must be as small as possible. I'd prefer not to ship optional code paths for runtime.

felixbarny commented 7 years ago

How can vars.js be overridden?

bripkens commented 7 years ago

You replace it at build time.

felixbarny commented 7 years ago

I see. Then yes, it's probably the best place to specify the header names.

bripkens commented 7 years ago

Do you want to contribute this?

felixbarny commented 7 years ago

I really want but currently I'm quite involved in a lot of stuff. But I'll ask my colleague @trampi who implemented the eum stuff in stagemonitor.

felixbarny commented 7 years ago

@trampi: Do you think this approach is feasible or should we rather stick to directly modifying the headers in processResources?

trampi commented 7 years ago

It seems reasonable to me to override them via vars.js. By the way, we should definitely have a look at using CORS (see https://github.com/instana/weasel/blob/master/lib/hooks/XMLHttpRequest.js#L208), or else the headers will not be set.

@bripkens: you are not using CORS because you want to support all browsers with minimum overhead, right?

bripkens commented 7 years ago

The thing is, that you cannot set tracing headers by default. Only for the same origin you can be sure that this is possible. All other origins need to explicitly allow these custom headers. By always setting these headers, we could break a lot of monitored websites which access third parties (which is basically every large website).

To make this work within weasel, I thought about adding a user-configurable domain whitelist. Users could add domains for which they know that servers send correct Access-Control-Allow-Headers headers.

bripkens commented 7 years ago

Suppose in the not too distant future we (as in the tracing community) come up with a common distributed trace correlation header, then we could talk to browser vendors to allow this header even for cross-origin requests. That would be great to have.

trampi commented 7 years ago

To give a little more background: we're currently implementing end user monitoring support in stagemonitor and use weasel to report the end user timings to a custom stagemonitor endpoint. The endpoint is either same origin or cross origin, in the latter case stagemonitor will send the correct CORS-headers.

By always setting these headers, we could break a lot of monitored websites which access third parties (which is basically every large website).

As far as I understand, only the server specified in reportingUrl has to set the correct CORS headers, correct? The worst-case is that weasel will not be able to report, because of incorrect CORS headers, right?

To make this work within weasel, I thought about adding a user-configurable domain whitelist. Users could add domains for which they know that servers send correct Access-Control-Allow-Headers headers.

Weasel could try to send a XHR with the correlation headers and fall back to sending an XHR without them, if the first XHR fails because of CORS being configured incorrectly.

Suppose in the not too distant future we (as in the tracing community) come up with a common distributed trace correlation header, then we could talk to browser vendors to allow this header even for cross-origin requests.

Yes, that would be awesome!

trampi commented 7 years ago

Sorry, was thinking about beacons, not XHR-Requests to other hosts for propagation. I now understand the problem.

bripkens commented 7 years ago

Weasel could try to send a XHR with the correlation headers and fall back to sending an XHR without them, if the first XHR fails because of CORS being configured incorrectly.

As far as I know, the client side is not informed about the concrete XHR rejection reason. If it is, this might be one way to do it. Though I'd prefer to give users control over this as it is incredibly unlikely that APIs send the correct Access-Control-Allow-Headers header.