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

[johnbley]

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]

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]

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

[johnbley]

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]

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

[obecny]

lgtm, one comment, thx

[dyladan]

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]

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