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(xhr): check for resource timing support #1720

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

bradfrosty
Copy link
Contributor

@bradfrosty bradfrosty commented Dec 4, 2020

Which problem is this PR solving?

Short description of the changes

  • Check for PerformanceResourceTiming support in @opentelemetry/instrumentation-xml-http-request

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #1720 (19aee1b) into master (91612c4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1720   +/-   ##
=======================================
  Coverage   91.35%   91.36%           
=======================================
  Files         165      165           
  Lines        5138     5141    +3     
  Branches     1056     1059    +3     
=======================================
+ Hits         4694     4697    +3     
  Misses        444      444           
Impacted Files Coverage Δ
...emetry-instrumentation-xml-http-request/src/xhr.ts 96.77% <100.00%> (+0.05%) ⬆️

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM but according to caniuse the PerformanceResourceTiming API is only unsupported in very old browsers that we likely don't work with anyways. Are you trying to target support for some specific browser version?

@bradfrosty
Copy link
Contributor Author

@dyladan I was confused for a bit, because I was referring to that caniuse link as well. However, that page is not entirely accurate.

In the issue description I link to a more accurate caniuse for resource timing support.

So it shows that it's supported in Safari 11, but only beyond macOS 10.12+. So in a sense, I guess we're trying to target a specific browser version AND OS version. We have a significant number of users still using Safari 11 on macOS 10.11 (57k errors reported in last 24 hours). The reported browser and OS for these errors is 100% on this specific browser/OS combination.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@obecny obecny added the bug Something isn't working label Dec 4, 2020
@bradfrosty bradfrosty force-pushed the observer-check-entrytype branch from d3e6fbf to 27c381e Compare December 7, 2020 18:19
@obecny obecny merged commit e941f55 into open-telemetry:master Dec 9, 2020
@bradfrosty bradfrosty deleted the observer-check-entrytype branch December 9, 2020 15:47
naseemkullah pushed a commit to naseemkullah/opentelemetry-js that referenced this pull request Dec 23, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XHR instrumentation doesn't check browser support for PerformanceObserver entryTypes
4 participants