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

fix(sdk-trace-web): make parseUrl respect document.baseURI #3670

Merged
merged 6 commits into from
Mar 17, 2023

Conversation

domasx2
Copy link
Contributor

@domasx2 domasx2 commented Mar 13, 2023

Which problem is this PR solving?

Noticed that fetch instrumentation breaks web apps that provide relative urls to window.fetch and have URL base explicitly set via a base element. The root cause seems to be parseUrl, which resolves absolute URL using location.href as URL base when it should use document.baseURI.

Short description of the changes

Makes parseUrl use document.baseURI when available and fall back to location.href when not (web workers).

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I'm attempting to instrument Grafana frontend with tracing. If fetch instrumentation is enabled, dashboards do not load due to mangled API request URLs. Applying this fix resolves the issue.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@domasx2 domasx2 requested a review from a team as a code owner March 13, 2023 15:17
@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #3670 (a34f668) into main (52facfa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a34f668 differs from pull request most recent head 814af56. Consider uploading reports for the commit 814af56 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3670   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files         277      277           
  Lines        8454     8455    +1     
  Branches     1761     1762    +1     
=======================================
+ Hits         7923     7924    +1     
  Misses        531      531           
Impacted Files Coverage Δ
packages/opentelemetry-sdk-trace-web/src/utils.ts 95.09% <100.00%> (+0.03%) ⬆️

@dyladan
Copy link
Member

dyladan commented Mar 13, 2023

Looks like you have a lint failure. Please also make a changelog entry to CHANGELOG.md

@domasx2 domasx2 changed the title fix(web-sdk): make parseUrl respect document.baseURI fix(sdk-trace-web): make parseUrl respect document.baseURI Mar 14, 2023
@domasx2 domasx2 force-pushed the domas-fix-parsing-relative-urls branch from 703e628 to f813b6c Compare March 14, 2023 10:07
@domasx2
Copy link
Contributor Author

domasx2 commented Mar 14, 2023

Looks like you have a lint failure. Please also make a changelog entry to CHANGELOG.md

Thanks @dyladan ! Updated changelog. Lint failure seems to be 503 to npmjs.com when linting docs, but the same link works now and lint passes locally. Likely a temporary issue at the time

@domasx2 domasx2 requested a review from dyladan March 14, 2023 14:44
@dyladan
Copy link
Member

dyladan commented Mar 17, 2023

Please do not force push during the review process. It makes it more difficult to see what has changed since the last review, breaks comments on previous commits, and breaks links to previous commits.

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

Successfully merging this pull request may close these issues.

None yet

2 participants