XMLHttpRequest #595

obecny · 2019-12-05T21:39:20Z

Which problem is this PR solving?

Short description of the changes

It adds auto instrumentation for XMLHttpRequest including trace headers

…ttp-request

…e for web

codecov-io · 2019-12-05T21:58:24Z

Codecov Report

Merging #595 into master will decrease coverage by 0.24%.
The diff coverage is 83.04%.

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
- Coverage   90.48%   90.23%   -0.25%     
==========================================
  Files         180      189       +9     
  Lines        9109     9608     +499     
  Branches      814      877      +63     
==========================================
+ Hits         8242     8670     +428     
- Misses        867      938      +71

Impacted Files	Coverage Δ
...ntelemetry-web/src/enums/PerformanceTimingNames.ts	`100% <ø> (ø)`
packages/opentelemetry-core/src/common/types.ts	`100% <ø> (ø)`	⬆️
packages/opentelemetry-web/src/WebTracer.ts	`100% <100%> (ø)`	⬆️
...telemetry-plugin-document-load/src/documentLoad.ts	`97.93% <100%> (-0.2%)`	⬇️
...lugin-xml-http-request/src/enums/AttributeNames.ts	`100% <100%> (ø)`
...ry-plugin-xml-http-request/src/enums/EventNames.ts	`100% <100%> (ø)`
packages/opentelemetry-core/src/utils/wrap.ts	`100% <100%> (ø)`
...emetry-plugin-xml-http-request/src/enums/Format.ts	`100% <100%> (ø)`
packages/opentelemetry-core/src/utils/url.ts	`100% <100%> (ø)`
packages/opentelemetry-core/test/utils/url.test.ts	`50% <50%> (ø)`
... and 27 more

mayurkale22 · 2019-12-05T21:59:38Z

/cc @draffensperger

dyladan

Some first pass comments

packages/opentelemetry-core/src/common/time.ts

dyladan · 2019-12-06T10:48:22Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+  protected patch() {
+    this._logger.debug('applying patch to', this.moduleName, this.version);
+
+    if (isWrapped(XMLHttpRequest.prototype.open)) {


Why would this already be patched? Shouldn't we just no-op if it is already wrapped?

with web there is always some probability that something can be destroyed and recreated again. This way it will always work correctly

Since this is patching a global, would it not patch if multiple components on the same page are instrumented with separate tracers/configs?

copy paste here my reply from below
We are using shimmer which doesn't support multiple wrap of the same function so if you even register multiple tracers with the same plugins, and try to unpatch just one, it will unpatch the global successfully anyway. So if the decision to use shimmer was done my assumption is that we are able to instrument using the plugins once only. Otherwise the code will be broken anyway.

packages/opentelemetry-core/src/trace/instrumentation/basePluginUtils.ts

dyladan · 2019-12-06T11:01:22Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+      ): OpenFunction | any {
+        plugin._createSpan(this as XMLHttpRequestWrapped, url, method);
+
+        if (async) {


it is always safer to use original.apply(this, arguments) because we can't account for users passing incorrect values or the incorrect number of values. It also removes the need for this check.

It is the same here.
You don't have arguments word in typescript , so you would still declare array of arguments and define type for it, which would be quite weird anyway.

Besides that check also the whole code to see more context. The async will always be set to true and there are reasons for that check here. It is also worth to mention that it was ported from open census.

You do have the arguments keyword in typescript.

It is possible to call open with async: false and user not-null. In this case you are changing the arguments the user has called the function with.

We cannot assume that users will use the API properly as documented and it is much easier and safer to directly pass their arguments. If the user incorrectly calls open with async: true I am suggesting something like this (tests pass with this version):

protected _patchOpen() { return (original: Function): OpenFunction => { const plugin = this; return function patchOpen( this: XMLHttpRequest, method: string, url: string, async?: boolean, user?: string | null, pass?: string | null ): ReturnType<OpenFunction> { plugin._createSpan(this as XMLHttpRequestWrapped, url, method); return original.apply(this, arguments); /* If you would rather use `call` this can be done easily return original.call(this, ...arguments); */ }; }; }

Regarding the performance difference between apply and call, it may be the case that call is faster than apply, but in this case the if statement would provide more overhead than the difference.

still cannot use apply anyway, as I mentioned previously please read the spec, and why it needs to force the async

If true, notification of a completed transaction is provided using event listeners.

What about that means it must be true? If it is false (synchronous), then you just finish the span after calling the original function. If user sets async false in a context that it is not allowed (some browsers only allow sync requests in workers), then

The way your code is currently, it changes the behavior of user code behind the scenes. If I am a user and I make a synchronous xhr request, I expect it to be made synchronously not transparently rewritten to be async behind the scenes by my telemetry provider.

https://github.com/census-instrumentation/opencensus-web/blob/68aa988732e03f2de49ebc0213c3b2525aa5b68b/packages/opencensus-web-instrumentation-zone-peer-dep/src/monkey-patching.ts#L31
and
https://github.com/census-instrumentation/opencensus-web/blob/68aa988732e03f2de49ebc0213c3b2525aa5b68b/packages/opencensus-web-instrumentation-zone-peer-dep/src/monkey-patching.ts#L54
and
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests

Can you guys @draffensperger @mayurkale22 have your input here too, thx :).

and one more Synchronous XHR is now in deprecation state.

I understand that it's deprecated, but that doesn't mean users can't or won't use it and the telemetry library shouldn't silently change the behavior of the program.

should we tallk about this in the SIG tomorrow?

dyladan · 2019-12-06T11:02:29Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+          };
+          plugin._addHeaders(this, currentSpan);
+        }
+        return original.call(this, body);


Suggested change

return original.call(this, body);

return original.apply(this, arguments);

call is faster

we already know there is just one para, we know it's type so we can use it and typescript will also validate the type of this is correcty

You don't have arguments word in typescript , so you would still declare array of aguments and define type for it, which would be quite weird anyway

and the same here, thx
@draffensperger @mayurkale22 ^^

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

examples/tracer-web/examples/xml-http-request/index.js

packages/opentelemetry-core/src/common/time.ts

packages/opentelemetry-core/src/trace/instrumentation/basePluginUtils.ts

packages/opentelemetry-plugin-xml-http-request/src/enums/AttributeNames.ts

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

packages/opentelemetry-web/src/utils.ts

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

OlivierAlbertini

Nice work!

examples/tracer-web/README.md

packages/opentelemetry-plugin-xml-http-request/src/enums/AttributeNames.ts

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

obecny · 2019-12-07T02:48:40Z

marking temporary as WIP - working on better detection for cors

dyladan · 2019-12-09T07:37:52Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+ */
+export class XMLHttpRequestPlugin extends BasePlugin<XMLHttpRequest> {
+  readonly component: string = 'xml-http-request';
+  readonly version: string = '0.3.0';


That would assume that the user has the same version of the plugin and of core installed which may not always be the case. @obecny and I had talked about adding the autogenerated VERSION file to all lerna packages, but nothing has been done yet.

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

dyladan · 2019-12-09T10:08:11Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+      ): OpenFunction | any {
+        plugin._createSpan(this as XMLHttpRequestWrapped, url, method);
+
+        if (async) {


You do have the arguments keyword in typescript.

It is possible to call open with async: false and user not-null. In this case you are changing the arguments the user has called the function with.

We cannot assume that users will use the API properly as documented and it is much easier and safer to directly pass their arguments. If the user incorrectly calls open with async: true I am suggesting something like this (tests pass with this version):

protected _patchOpen() { return (original: Function): OpenFunction => { const plugin = this; return function patchOpen( this: XMLHttpRequest, method: string, url: string, async?: boolean, user?: string | null, pass?: string | null ): ReturnType<OpenFunction> { plugin._createSpan(this as XMLHttpRequestWrapped, url, method); return original.apply(this, arguments); /* If you would rather use `call` this can be done easily return original.call(this, ...arguments); */ }; }; }

Regarding the performance difference between apply and call, it may be the case that call is faster than apply, but in this case the if statement would provide more overhead than the difference.

dyladan · 2019-12-10T20:42:55Z

You may also want to do the same with the other maps that have possibility of leaks:

_callbackToRemoveEvents
_resourcesCreatedInTheMiddle
_usedResources

which then removes the need for taskCount

obecny · 2019-12-10T21:33:36Z

@dyladan not all can be done and taskCount is still needed, but all things that depends on xhr has been moved.

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

draffensperger

💯 👍 Great job on this.

I'm giving approval since to me this semantically captures what we should be doing in this plugin. I left a couple more minor comments/suggestions and in particular I think the issue around the cast vs. use of the span API is relevant to fix if we can. But this is great!!

As an aside, I saw the comment thread about this assuming there is a trace context set up, which to me is a relevant comment, and highlights the need for this plugin to work in conjunction with other plugins that set up a "root" span of sorts, e.g. a user interaction tracing plugin, a root span from document load, etc.

examples/tracer-web/examples/xml-http-request/index.js

packages/opentelemetry-plugin-xml-http-request/README.md

draffensperger · 2019-12-11T14:40:45Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+        }
+      }
+      if (!addHeaderOnDifferentOrigin) {
+        this._logger.warn('Cannot set headers on different origin');


I think this will be a pretty routine case that the user sends an XHR to a different origin that they don't want to pass along trace headers for (that is, at least until the browser spec can give an exception for CORS for the W3C trace headers)

Given that, would it make sense to remove this warning log and corresponding if block?

I might be wrong but my understanding is that we might break something with the request if we force to add headers. The headers are only added when origin is the same. If not then I check if user allows to add the headers based on config. If not then the headers are dropped and warning is being show. I cannot remove the if block (to prevent adding headers) but only warning if that is something you want me to do ?

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

…ixed

dyladan

This looks real good!

From my perspective I really only have one outstanding question about forcing all calls to open to be async even when the user passes false. I'd like to get @draffensperger and @mayurkale22 opinion on this.

It looks like the dependency on web in this version is limited to only functions that don't require casts to our sdk types so they should be compatible even with an sdk other than ours being used. 👍

dyladan · 2019-12-12T00:24:31Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

+      ): void {
+        plugin._createSpan(this, url, method);
+
+        return original.call(this, method, url, true, user, pass);


Creating a new thread on this line as the old one is deep in the history and on a line that has been outdated for a long time.

@draffensperger @mayurkale22

Yeah, I think I missed this in my review, but I definitely agree that we should preserve the original behavior of the XHR function including the async parameter.

draffensperger · 2019-12-12T18:11:59Z

examples/tracer-web/examples/xml-http-request/index.js

+  const onClick = () => {
+    for (let i = 0, j = 5; i < j; i++) {
+      const span1 = webTracerWithZone.startSpan(`files-series-info-${i}`, {
+        parent: webTracerWithZone.getCurrentSpan()


I believe if there is no current span, this span will just be a root span, which is the correct behavior. I think my comment about having a nice root span for XHRs is more like a future feature and not so much an issue with this code.

obecny · 2019-12-16T18:34:08Z

@draffensperger @mayurkale22 @dyladan changed the code with regards to our meeting

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts

examples/tracer-web/examples/xml-http-request/index.js

lerna.json

mayurkale22 · 2019-12-19T22:52:43Z

@obecny just wanted to confirm, do you have any outstanding work here or PR is ready to merge?

obecny · 2019-12-19T23:29:33Z

@obecny just wanted to confirm, do you have any outstanding work here or PR is ready to merge?

Nothing left from my side

obecny added 3 commits December 5, 2019 22:12

feat(xml-http-request): new plugin for auto instrumentation for xml-h…

8fce91a

…ttp-request

chore: new example for xml-http-request and updated examples structur…

0875f3f

…e for web

chore: updating readme

f1c6013

obecny requested review from bg451, danielkhan, markwolff, mayurkale22, OlivierAlbertini, rochdev and vmarchaud as code owners December 5, 2019 21:39

obecny added 3 commits December 5, 2019 22:45

chore: linting

6038bcc

chore: fixing origin for tests

ac68ab9

chore: linting

87dcdc6

obecny added plugin-request labels Dec 5, 2019

mayurkale22 added the Awaiting reviewer feedback label Dec 5, 2019

chore: updating to use b3 format from core

6e10df7

dyladan reviewed Dec 6, 2019

View reviewed changes

draffensperger reviewed Dec 6, 2019

View reviewed changes

OlivierAlbertini reviewed Dec 6, 2019

View reviewed changes

examples/tracer-web/README.md Show resolved Hide resolved

packages/opentelemetry-plugin-xml-http-request/src/enums/AttributeNames.ts Show resolved Hide resolved

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts Outdated Show resolved Hide resolved

obecny added 4 commits December 6, 2019 21:24

chore: updates after reviews

e91d8f9

chore: wrong function call

8e01e96

chore: updating attribute names

898c190

chore: linting

54d0ba0

obecny added the WIP label Dec 7, 2019

dyladan reviewed Dec 9, 2019

View reviewed changes

obecny added 2 commits December 10, 2019 22:26

chore: adding weakmap for keeping information connected with xhr

f73aacb

chore: renaming config param

88e122a

chore: not needed cast

c3dac11

dyladan reviewed Dec 10, 2019

View reviewed changes

chore: updating title

524da7a

draffensperger approved these changes Dec 11, 2019

View reviewed changes

chore: refactored xhr, removed tracing dependency, few other issues f…

50e151e

…ixed

dyladan reviewed Dec 12, 2019

View reviewed changes

draffensperger reviewed Dec 12, 2019

View reviewed changes

obecny added 4 commits December 13, 2019 22:04

chore: reviews

6c143e7

chore: refactored for collecting timing resources

3d7b32e

chore: merge branch 'master' into xml-http-request

91bad5d

chore: fixes after merging

5aa23ca

obecny requested a review from dyladan December 17, 2019 20:56

dyladan requested changes Dec 17, 2019

View reviewed changes

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts Outdated Show resolved Hide resolved

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts Outdated Show resolved Hide resolved

chore: reviews

ab9795a

dyladan approved these changes Dec 18, 2019

View reviewed changes

mayurkale22 added Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) and removed Awaiting reviewer feedback labels Dec 18, 2019

mayurkale22 reviewed Dec 19, 2019

View reviewed changes

examples/tracer-web/examples/xml-http-request/index.js Show resolved Hide resolved

lerna.json Outdated Show resolved Hide resolved

chore: reviews

4d4e65f

mayurkale22 approved these changes Dec 19, 2019

View reviewed changes

Merge branch 'master' into xml-http-request

ca96cb1

mayurkale22 merged commit bc583b8 into open-telemetry:master Dec 19, 2019

obecny deleted the xml-http-request branch July 8, 2020 12:13

	return original.call(this, body);
	return original.apply(this, arguments);

XMLHttpRequest #595

XMLHttpRequest #595

Conversation

obecny commented Dec 5, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov-io commented Dec 5, 2019 • edited Loading

Codecov Report

mayurkale22 commented Dec 5, 2019

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini left a comment

Choose a reason for hiding this comment

obecny commented Dec 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Dec 10, 2019

obecny commented Dec 10, 2019

draffensperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny commented Dec 16, 2019 • edited Loading

mayurkale22 commented Dec 19, 2019

obecny commented Dec 19, 2019

obecny commented Dec 5, 2019 •

edited

Loading

codecov-io commented Dec 5, 2019 •

edited

Loading

obecny Dec 10, 2019 •

edited

Loading

obecny commented Dec 16, 2019 •

edited

Loading