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

Allow for alternative context propagation headers #4

Open
felixbarny opened this issue Jul 9, 2017 · 13 comments
Open

Allow for alternative context propagation headers #4

felixbarny opened this issue Jul 9, 2017 · 13 comments

Comments

@felixbarny
Copy link

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
Copy link
Contributor

bripkens commented Jul 9, 2017

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
Copy link
Author

How can vars.js be overridden?

@bripkens
Copy link
Contributor

You replace it at build time.

@felixbarny
Copy link
Author

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

@bripkens
Copy link
Contributor

Do you want to contribute this?

@felixbarny
Copy link
Author

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
Copy link
Author

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

@trampi
Copy link

trampi commented Jul 11, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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

trampi commented Jul 11, 2017

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

trampi commented Jul 11, 2017

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

@bripkens
Copy link
Contributor

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.

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

No branches or pull requests

3 participants