feat: plugin user interaction for web #658

obecny · 2020-01-02T23:24:06Z

Which problem is this PR solving?

fixes Automatic tracing of user interactions in the browser (e.g. clicks) #492

Short description of the changes

It adds auto instrumentation for user click events. Together with plugin for xhr it will give user better overview and tracking for such actions

obecny · 2020-01-02T23:24:25Z

^^ @draffensperger

codecov-io · 2020-01-02T23:33:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b079de4). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #658   +/-   ##
=========================================
  Coverage          ?   89.79%           
=========================================
  Files             ?      217           
  Lines             ?    10322           
  Branches          ?      990           
=========================================
  Hits              ?     9269           
  Misses            ?     1053           
  Partials          ?        0

Impacted Files	Coverage Δ
.../opentelemetry-plugin-pg-pool/test/pg-pool.test.ts	`34.92% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b079de4...a392585. Read the comment docs.

markwolff · 2020-01-03T00:44:52Z

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

+      shimmer.wrap(
+        HTMLElement.prototype,
+        'addEventListener',
+        this._patchElement()


Does this patch get used only on elements which use addEventListener (textbox, etc)? Would inlines like onclick="some fn" use this as well?

inline onclick is not supported

That is correct, see this Stack Overflow answer that says the same thing.

A potential future work item could be to iterate over the DOM and see if there are any onclick event handlers set and then substitute them out for a wrapped version that would run them in the root zone.

That said, for the first version of this, I think we should just document it as a limitation and create a follow up issue to consider doing at some point in the future.

I expect that many people will use web frameworks like React, Vue, Angular, all of which I'm pretty sure use addEventListener rather than setting the onclick property.

That's an unfortunate limitation but seems like we can't really do much about it for now. Iterating over the DOM is potentially expensive and I would want it to be configurable at least.

I wonder if there would be a way to do it with a query selector, worth looking into anyway. But seems like a follow-up / future work item. We could also just see how often people actually request it, particularly since onclick is consider bad practice.

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

markwolff · 2020-01-03T00:50:59Z

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

+    if (!element.getAttribute) {
+      return undefined;
+    }
+    if (element.hasAttribute('disabled')) {


Do you think it would be useful to include an additional element attribute that this plugin will look for to ignore tracking?

Hmm don't have really opinion on that. Is there already anything like "common practice" or some standard how certain element is being excluded from auto instrumentation ?. Do you think it should be configurable in config ?

I like the idea of having a default attribute to opt-out of instrumentation for clicks but I think having it configurable is also good.

I think ideally (not for initial PR), we would also have some more intelligence on what types of interactions actually become spans. For example, a fair number of clicks might not result in any network calls, but maybe just change UI state (e.g. open a sidebar, etc.). Those aren't so meaningful to track whereas something like a "Save" button interaction that has multiple network calls and UI rendering would be valuable. A proxy for that could be that an interaction takes a certain amount of time or that it results in a network call. Seems like a potential area of future work but I don't think it's necessary for the first pass on this.

draffensperger

Great work @obecny this is awesome! I did a first pass with a number of comments, but this is generally good and moves the library forward in a really cool way.

packages/opentelemetry-plugin-user-interaction/src/types.ts

draffensperger · 2020-01-03T14:48:52Z

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

+    if (!element.getAttribute) {
+      return undefined;
+    }
+    if (element.hasAttribute('disabled')) {


I like the idea of having a default attribute to opt-out of instrumentation for clicks but I think having it configurable is also good.

I think ideally (not for initial PR), we would also have some more intelligence on what types of interactions actually become spans. For example, a fair number of clicks might not result in any network calls, but maybe just change UI state (e.g. open a sidebar, etc.). Those aren't so meaningful to track whereas something like a "Save" button interaction that has multiple network calls and UI rendering would be valuable. A proxy for that could be that an interaction takes a certain amount of time or that it results in a network call. Seems like a potential area of future work but I don't think it's necessary for the first pass on this.

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

packages/opentelemetry-plugin-user-interaction/test/userInteraction.test.ts

packages/opentelemetry-web/src/utils.ts

packages/opentelemetry-plugin-user-interaction/test/helper.test.ts

packages/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts

draffensperger · 2020-01-07T13:50:21Z

packages/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts

+      );
+
+      // this is needed as window is treated as scope and karma is adding
+      // context which is then detected as spanContext


Given that context is a fairly generic thing, would it make sense for the scope manager to use something like _ot_context or similar to not collide with any production application that might be using context on the window for something application specific?

We might fix this differently by removing window from scope managers when active span doesn't exist I think this might be probably a proper fix, I need to check it but the fix will come in different PR anyway, for now I would say it will stay as it is

packages/opentelemetry-plugin-user-interaction/test/userInteraction.test.ts

packages/opentelemetry-web/src/utils.ts

packages/opentelemetry-plugin-user-interaction/test/userInteraction.test.ts

packages/opentelemetry-plugin-user-interaction/package.json

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

mayurkale22 · 2020-01-09T00:53:19Z

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

+        task: AsyncTask
+      ) {
+        const currentZone = Zone.current;
+        const currentSpan: types.Span = currentZone.get(ZONE_SCOPE_KEY);


types.Span is optional, right?

what you mean ?

I think he is asking if currentZone.get always returns a Span or can it also return null or undefined?

draffensperger

Left a couple more comments, mostly stylistic / type system stuff and requesting that we don't test private methods. We can also iterate in the future on detecting span ends without Zone.js and on what exact events to trace.

But I think this is getting in a generally good overall shape. Great work @obecny . I'm going to give my approval though I gave a few more suggested changes.

draffensperger · 2020-01-13T15:27:01Z

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

+      shimmer.wrap(
+        HTMLElement.prototype,
+        'addEventListener',
+        this._patchElement()


I wonder if there would be a way to do it with a query selector, worth looking into anyway. But seems like a follow-up / future work item. We could also just see how often people actually request it, particularly since onclick is consider bad practice.

draffensperger · 2020-01-13T15:27:50Z

packages/opentelemetry-plugin-user-interaction/test/userInteraction.test.ts

+    let sandbox: sinon.SinonSandbox;
+    let webTracer: WebTracer;
+    let dummySpanExporter: DummySpanExporter;
+    let exportSpy: any;


@obecny friendly ping on this - is it possible to use the jasmine.Spy type here?

draffensperger · 2020-01-13T15:28:06Z

packages/opentelemetry-plugin-user-interaction/test/userInteraction.test.ts

+    let webTracer: WebTracer;
+    let dummySpanExporter: DummySpanExporter;
+    let exportSpy: any;
+    let requests: any[] = [];


@obecny friendly ping here, is there any more specific type we can use?

packages/opentelemetry-web/src/utils.ts

…etry-js into plugin_user_interaction

draffensperger

Nice, thanks for these changes!

obecny · 2020-01-13T22:16:29Z

lint is failing due to david-dm.org, fix is here #660

mayurkale22 · 2020-01-21T17:54:56Z

@open-telemetry/javascript-approvers We need more reviews for this one!

mayurkale22

Overall lgtm

dyladan

Overall this LGTM. Just have the one question about the attribute name.

packages/opentelemetry-web/src/utils.ts

dyladan · 2020-01-24T17:20:20Z

packages/opentelemetry-plugin-user-interaction/src/userInteraction.ts

+          [AttributeNames.EVENT_TYPE]: eventName,
+          [AttributeNames.TARGET_ELEMENT]: element.tagName,
+          [AttributeNames.TARGET_XPATH]: xpath,
+          [AttributeNames.HTTP_URL]: window.location.href,


Is this meant to be the page you're currently viewing when you perform an interaction? If yes, I think we should use a different attribute name. http is a wire protocol and may not accurately represent the intended purpose. In addition, http.url already has a meaning in the spec and I don't want to confuse people. Can we change this to be something like view.url or page.url or similar?

@mayurkale22 @open-telemetry/javascript-approvers WDYT?

One of the complexities here is that the user did in fact use the HTTP protocol to request the page initially. In that sense, it's sort of tied back to the original request. It also may be what people are most familiar with searching for if they were to think of it.

That said, it is true that view.url or page.url is more accurate. If we were to do with that, we should also update the Document load plugin to reference it in its root spans (see

opentelemetry-js/packages/opentelemetry-plugin-document-load/src/documentLoad.ts

Line 117 in d3af8c4

PTN.FETCH_START,

)

@obecny @mayurkale22 what are your thoughts here?

I don't have any strong opinion on that, for me all 3 options are fine but having the same name for url if this is either web or node makes more sense for me so that's why I added here the same name as in other plugins. But this is really a minor thing and can be changed at anytime now or later.

* chore: bumped aws components to 1.0 * chore: fixed link in docs to be consistent Release-As: 1.0.0 * chore: cleaned up idgenerator readme Release-As: 1.0.0 * chore: cleaned up propagator readme Release-As: 1.0.0 * chore: updated sdk to 1.0.0 * updated readme for trace id generator Co-authored-by: Rauno Viskus <[email protected]> Co-authored-by: Bartlomiej Obecny <[email protected]>

feat: plugin user interaction for web

d639e06

obecny requested review from bg451, dyladan, markwolff, mayurkale22, OlivierAlbertini, rochdev and vmarchaud as code owners January 2, 2020 23:24

markwolff reviewed Jan 3, 2020

View reviewed changes

mayurkale22 added Awaiting reviewer feedback labels Jan 3, 2020

chore: unnecessary check

d1a6509

draffensperger reviewed Jan 3, 2020

View reviewed changes

dyladan added the enhancement New feature or request label Jan 3, 2020

obecny added 5 commits January 6, 2020 18:42

chore: reviews

6c9de55

chore: merge with masters

6d2259f

chore: adding version

dab40b1

chore: adding version

bc96552

chore: reviews

eb698de

mayurkale22 added the size/XXL label Jan 6, 2020

draffensperger reviewed Jan 7, 2020

View reviewed changes

chore: reviews

be21bac

mayurkale22 reviewed Jan 9, 2020

View reviewed changes

obecny added 3 commits January 10, 2020 12:31

chore: reviews

fdb4019

Merge branch 'master' into plugin_user_interaction

85745d1

chore: fixes after merge

26eaffe

draffensperger approved these changes Jan 13, 2020

View reviewed changes

Merge branch 'master' into plugin_user_interaction

324a705

obecny added 2 commits January 13, 2020 23:06

chore: reviews

199db32

Merge branch 'plugin_user_interaction' of github.com:obecny/opentelem…

de0c4d1

…etry-js into plugin_user_interaction

draffensperger reviewed Jan 13, 2020

View reviewed changes

obecny added 3 commits January 15, 2020 23:32

Merge branch 'master' into plugin_user_interaction

be41208

chore: updating version

4e79efe

Merge branch 'master' into plugin_user_interaction

b079de4

obecny self-assigned this Jan 16, 2020

Merge branch 'master' into plugin_user_interaction

a392585

obecny requested review from markwolff and mayurkale22 January 16, 2020 20:59

mayurkale22 approved these changes Jan 23, 2020

View reviewed changes

dyladan reviewed Jan 24, 2020

View reviewed changes

chore: merge branch 'master' into plugin_user_interaction

55e3a9e

dyladan approved these changes Feb 4, 2020

View reviewed changes

mayurkale22 and others added 3 commits February 4, 2020 17:11

Merge branch 'master' into plugin_user_interaction

9fd862a

chore: lint

75eb5af

chore: merge branch 'master' into plugin_user_interaction

66b1dbd

mayurkale22 removed the Awaiting reviewer feedback label Feb 5, 2020

mayurkale22 merged commit e13dd57 into open-telemetry:master Feb 5, 2020

obecny deleted the plugin_user_interaction branch July 8, 2020 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: plugin user interaction for web #658

feat: plugin user interaction for web #658

obecny commented Jan 2, 2020

obecny commented Jan 2, 2020

codecov-io commented Jan 2, 2020 •

edited

Loading

markwolff Jan 3, 2020

obecny Jan 3, 2020

draffensperger Jan 3, 2020

dyladan Jan 6, 2020

draffensperger Jan 13, 2020

markwolff Jan 3, 2020

obecny Jan 3, 2020 •

edited

Loading

draffensperger Jan 3, 2020

draffensperger left a comment

draffensperger Jan 3, 2020

draffensperger Jan 7, 2020

obecny Jan 13, 2020

mayurkale22 Jan 9, 2020

obecny Jan 10, 2020

dyladan Jan 13, 2020

draffensperger left a comment

draffensperger Jan 13, 2020

draffensperger Jan 13, 2020

draffensperger Jan 13, 2020

draffensperger left a comment

obecny commented Jan 13, 2020

mayurkale22 commented Jan 21, 2020

mayurkale22 left a comment

dyladan left a comment

dyladan Jan 24, 2020

draffensperger Jan 24, 2020

dyladan Feb 4, 2020

obecny Feb 4, 2020 •

edited

Loading

feat: plugin user interaction for web #658

feat: plugin user interaction for web #658

Conversation

obecny commented Jan 2, 2020

Which problem is this PR solving?

Short description of the changes

obecny commented Jan 2, 2020

codecov-io commented Jan 2, 2020 • edited Loading

Codecov Report

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 Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

draffensperger left a comment

Choose a reason for hiding this comment

obecny commented Jan 13, 2020

mayurkale22 commented Jan 21, 2020

mayurkale22 left a 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 Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Jan 2, 2020 •

edited

Loading

obecny Jan 3, 2020 •

edited

Loading

obecny Feb 4, 2020 •

edited

Loading