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

Pick better defaults for tracePropagationTargets #9764

Closed
lforst opened this issue Dec 6, 2023 · 4 comments · Fixed by #10621
Closed

Pick better defaults for tracePropagationTargets #9764

lforst opened this issue Dec 6, 2023 · 4 comments · Fixed by #10621
Milestone

Comments

@lforst
Copy link
Member

lforst commented Dec 6, 2023

The current defaults for tracePropagationTargets (['localhost', /^\/(?!\/)/]) run into a few problems:

  • If you do a fetch request to the same origin domain without using a short version (i.e. fetch("https://mysameorigin.com/api/some-route") instead of fetch("/api/some-route")) we will currently not attach tracing headers by default.
  • If you use localhost with subdomains (e.g. http://cdn.localhost/resource) we will add tracing headers which may trigger CORS even for localhost.
  • Currently, we don't attach tracing headers to requests like fetch("//sameorigin.com/") while it would be valid to do so if sameorigin.com is actually the same origin.

Proposal

  • We need to be stricter on the localhost validation, e.g. /^(https?|ws):\/\/localhost//
  • We need to add a matcher for when the origin is provided and it is the same origin as the current page, e.g. new RegExp(`^${window.location.origin}\/`)
  • (Requirement: No longer support IE11) When a URL starts with a /, e.g. //external-origin.com/, we throw it into the URL constructor as follows and use the resulting origin as the base for matching against items in tracePropagationTargets: new URL(target, window.location.origin). That way, we are matching the actual resolved (including the protocol) against our defined tracePropagationTargets and it will work well with our predefined defaults. Maybe there is a case to be made to generally throw the target in the URL constructor before comparing against anything.
@mydea
Copy link
Member

mydea commented Dec 7, 2023

WDYT about doing 1. and 2. in v7? You could consider it breaking, but I'd say it's more of a "fix" and probably what people would expect anyhow..?

@lforst
Copy link
Member Author

lforst commented Dec 7, 2023

@mydea There is definitely an argument to doing it already. I wasn't sure if it is technically breaking though 🤔 Touching tPT is kinda scary because the errors that could result from it are very hight impact.

@lforst
Copy link
Member Author

lforst commented Dec 7, 2023

Especially adding a new matcher is very scary because we will attach headers to more requests.

@Baffour
Copy link

Baffour commented Mar 25, 2024

@lforst amazing thanks for this! I was about to raise that last point as a bug/feature request, before I found this. Our code has been inconsistently using full URLs in some places, and just the path in others, and it took me a long time to realise this is why distributed tracing wasn't working consistently when tracePropagationTargets was defined

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

Successfully merging a pull request may close this issue.

3 participants