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(plugin-xml-http-request): support sync requests #1133

Merged
merged 7 commits into from
Jun 4, 2020
Merged

fix(plugin-xml-http-request): support sync requests #1133

merged 7 commits into from
Jun 4, 2020

Conversation

johnbley
Copy link
Member

@johnbley johnbley commented Jun 2, 2020

Which problem is this PR solving?

XHR with async=false are currently unsupported with the instrumentation in plugin-xml-http-request, producing a warning message.

Short description of the changes

The spec promises that our onLoad/onError/etc. callbacks will be called as part of the synchronous send(). Therefore, the existing logic of hooking open+send and using these callbacks to determine span end works fine for the synchronous case as well. I've manually verified this in an Angular app using synchronous-mode xhr (on several browsers) and added a simple unit test showing this as well.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #1133 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   92.28%   92.30%   +0.02%     
==========================================
  Files         116      116              
  Lines        3396     3393       -3     
  Branches      686      685       -1     
==========================================
- Hits         3134     3132       -2     
+ Misses        262      261       -1     
Impacted Files Coverage Δ
...s/opentelemetry-plugin-xml-http-request/src/xhr.ts 79.31% <100.00%> (+0.18%) ⬆️

@dyladan
Copy link
Member

dyladan commented Jun 2, 2020

Sorry for the failing action. It seems there is a permissions issue, so I filed a PR to strip it out #1134

@johnbley
Copy link
Member Author

johnbley commented Jun 2, 2020

Sorry for the failing action. It seems there is a permissions issue, so I filed a PR to strip it out #1134

No worries! I'm already a member of otel and have contributed to other projects here, so it wasn't super important for me anyway. Your contribution guidelines are clear and easy to follow and I appreciate the PR template!

@mayurkale22
Copy link
Member

@obecny as you wrote plugin-xml-http-request, would love to get your eyes on this.

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, one comment, thx

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.

It seems hard to believe that such a minor change would add this functionality when it was explicitly unsupported. There must have been a reason?

@obecny
Copy link
Member

obecny commented Jun 3, 2020

It seems hard to believe that such a minor change would add this functionality when it was explicitly unsupported. There must have been a reason?

reading from spec:

Note: Starting with Gecko 30.0 (Firefox 30.0 / Thunderbird 30.0 / SeaMonkey 2.27), Blink 39.0, and Edge 13, synchronous requests on the main thread have been deprecated due to their negative impact on the user experience.
Synchronous XHR requests often cause hangs on the web. But developers typically don't notice the problem because the hang only manifests with poor network conditions or when the remote server is slow to respond. Synchronous XHR is now in deprecation state. The recommendation is that developers move away from the synchronous API and instead use asynchronous requests.

All new XHR features such as timeout or abort are not allowed for synchronous XHR. Doing so will raise an InvalidAccessError.

And in spec it is also mentioned that sync will return true or false and events are fired for async - nothing mentioned about sync.

But I checked the chrome with sync and yes it seems like the event is also fired and it works fine.
So if someone really wants to use sync callback (even though it is deprecated and bad practice) but then on the other hand it seems it can work I don't see a problem why. If one day it won't be working - we can't do much, but for today if it works why not then. Although I don't guarantee it is working fine in all known browsers - haven't checked the IE

@mayurkale22 mayurkale22 added the enhancement New feature or request label Jun 4, 2020
@mayurkale22 mayurkale22 merged commit dadcad7 into open-telemetry:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants