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.

Sign up for GitHub

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(instrumentation-user-interaction): support clicks in React apps #537

Merged
merged 4 commits into from
Jun 21, 2021
Merged

fix(instrumentation-user-interaction): support clicks in React apps #537

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69a1c7b
fix(instrumentation-user-interaction): support clicks in React apps
kkruk-sumo Jun 14, 2021
6209b38
Merge branch 'main' into support_react_clicks
kkruk-sumo Jun 14, 2021
7ee1d07
chore: code maintenance
kkruk-sumo Jun 16, 2021
b14db18
Merge branch 'main' into support_react_clicks
kkruk-sumo Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions plugins/web/opentelemetry-instrumentation-user-interaction/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ export class UserInteractionInstrumentation extends InstrumentationBase<unknown>
* auto instrument the click events
* This is done when zone is not available
*/
private _patchElement() {
private _patchAddEventListener() {
const plugin = this;
return (original: Function) => {
return function addEventListenerPatched(
Expand All @@ -265,16 +265,19 @@ export class UserInteractionInstrumentation extends InstrumentationBase<unknown>
) {
const once = useCapture && useCapture.once;
const patchedListener = (...args: any[]) => {
const target = this;
let parentSpan: api.Span | undefined;
const event: Event | undefined = args[0];
const target = event?.target;
if (event) {
parentSpan = plugin._eventsSpanMap.get(event);
}
if (once) {
plugin.removePatchedListener(this, type, listener);
}
const span = plugin._createSpan(target, type, parentSpan);
const span =
target instanceof HTMLElement
Copy link
Member

Choose a reason for hiding this comment

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

_createSpan already has a number of "does this have enough information to produce a good span? -> if not, undefined" checks, and I think this one belongs inside _createSpan rather than outside it, to keep said logic centralized.

? plugin._createSpan(target, type, parentSpan)
: undefined;
if (span) {
if (event) {
plugin._eventsSpanMap.set(event, span);
Expand Down Expand Up @@ -439,10 +442,14 @@ export class UserInteractionInstrumentation extends InstrumentationBase<unknown>
applyThis?: any,
applyArgs?: any
): Zone {
const target: HTMLElement | undefined = task.target;
const event =
Array.isArray(applyArgs) && applyArgs[0] instanceof Event
? applyArgs[0]
: undefined;
const target = event?.target;
let span: api.Span | undefined;
const activeZone = this;
if (target) {
if (target instanceof HTMLElement) {
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment - if _createSpan does this check, it doesn't need to be repeated elsewhere.

span = plugin._createSpan(target, task.eventName);
if (span) {
plugin._incrementTask(span);
Expand Down Expand Up @@ -564,23 +571,23 @@ export class UserInteractionInstrumentation extends InstrumentationBase<unknown>
);
} else {
this._zonePatched = false;
if (isWrapped(HTMLElement.prototype.addEventListener)) {
this._unwrap(HTMLElement.prototype, 'addEventListener');
if (isWrapped(EventTarget.prototype.addEventListener)) {
this._unwrap(EventTarget.prototype, 'addEventListener');
api.diag.debug('removing previous patch from method addEventListener');
}
if (isWrapped(HTMLElement.prototype.removeEventListener)) {
this._unwrap(HTMLElement.prototype, 'removeEventListener');
if (isWrapped(EventTarget.prototype.removeEventListener)) {
this._unwrap(EventTarget.prototype, 'removeEventListener');
api.diag.debug(
'removing previous patch from method removeEventListener'
);
}
this._wrap(
HTMLElement.prototype,
EventTarget.prototype,
'addEventListener',
this._patchElement()
this._patchAddEventListener()
);
this._wrap(
HTMLElement.prototype,
EventTarget.prototype,
'removeEventListener',
this._patchRemoveEventListener()
);
Expand Down
2 changes: 1 addition & 1 deletion plugins/web/opentelemetry-instrumentation-user-interaction/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as types from '@opentelemetry/api';
*/
export type AsyncTask = Task & {
eventName: string;
target: HTMLElement;
target: EventTarget;
// Allows access to the private `_zone` property of a Zone.js Task.
_zone: Zone;
};
Expand Down
4 changes: 1 addition & 3 deletions plugins/web/opentelemetry-instrumentation-user-interaction/test/helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,8 @@ export function createButton(disabled?: boolean): HTMLElement {

export function fakeInteraction(
callback: Function = function () {},
elem?: HTMLElement
element: HTMLElement = createButton()
) {
const element: HTMLElement = elem || createButton();

element.addEventListener('click', () => {
callback();
});
Expand Down
129 changes: 125 additions & 4 deletions ...ns/web/opentelemetry-instrumentation-user-interaction/test/userInteraction.nozone.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,10 +337,15 @@ describe('UserInteractionInstrumentation', () => {
callCount++;
};
document.body.addEventListener('click', listener1);
document.body.firstElementChild?.addEventListener('click', listener2);
document.body.firstElementChild?.dispatchEvent(
new MouseEvent('click', { bubbles: true })
);
try {
document.body.firstElementChild?.addEventListener('click', listener2);
document.body.firstElementChild?.dispatchEvent(
new MouseEvent('click', { bubbles: true })
);
} finally {
// remove added listener so we don't pollute other tests
document.body.removeEventListener('click', listener1);
}
assert.strictEqual(callCount, 2);
assert.strictEqual(exportSpy.args.length, 2);
assert.strictEqual(
Expand Down Expand Up @@ -410,6 +415,122 @@ describe('UserInteractionInstrumentation', () => {
});
});

it('should handle interactions listened on document - react < 17', done => {
const btn1 = document.createElement('button');
btn1.setAttribute('id', 'btn1');
document.body.appendChild(btn1);
const btn2 = document.createElement('button');
btn2.setAttribute('id', 'btn2');
document.body.appendChild(btn2);

const listener = (event: MouseEvent) => {
switch (event.target) {
case btn1:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
case btn2:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
}
};

document.addEventListener('click', listener);

try {
btn1.click();
btn2.click();
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate this test (having two targets and ensuring that the span names properly line up)!

} finally {
// remove added listener so we don't pollute other tests
document.removeEventListener('click', listener);
}

sandbox.clock.tick(1000);
originalSetTimeout(() => {
assert.equal(exportSpy.args.length, 4, 'should export 4 spans');

const span1: tracing.ReadableSpan = exportSpy.args[0][0][0];
const span2: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span3: tracing.ReadableSpan = exportSpy.args[2][0][0];
const span4: tracing.ReadableSpan = exportSpy.args[3][0][0];

assertClickSpan(span1, 'btn1');
assertClickSpan(span2, 'btn2');

assert.strictEqual(
span1.spanContext().spanId,
span3.parentSpanId,
'span3 has wrong parent'
);
assert.strictEqual(
span2.spanContext().spanId,
span4.parentSpanId,
'span4 has wrong parent'
);

done();
});
});

it('should handle interactions listened on a parent element (bubbled events) - react >= 17', done => {
const root = document.createElement('div');
document.body.appendChild(root);

const btn1 = document.createElement('button');
btn1.setAttribute('id', 'btn1');
root.appendChild(btn1);
const btn2 = document.createElement('button');
btn2.setAttribute('id', 'btn2');
root.appendChild(btn2);

root.addEventListener('click', event => {
switch (event.target) {
case btn1:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
case btn2:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
}
});

btn1.click();
btn2.click();

sandbox.clock.tick(1000);
originalSetTimeout(() => {
assert.equal(exportSpy.args.length, 4, 'should export 4 spans');

const span1: tracing.ReadableSpan = exportSpy.args[0][0][0];
const span2: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span3: tracing.ReadableSpan = exportSpy.args[2][0][0];
const span4: tracing.ReadableSpan = exportSpy.args[3][0][0];

assertClickSpan(span1, 'btn1');
assertClickSpan(span2, 'btn2');

assert.strictEqual(
span1.spanContext().spanId,
span3.parentSpanId,
'span3 has wrong parent'
);
assert.strictEqual(
span2.spanContext().spanId,
span4.parentSpanId,
'span4 has wrong parent'
);

done();
});
});

it('should handle disable', () => {
assert.strictEqual(
isWrapped(HTMLElement.prototype.addEventListener),
Expand Down
122 changes: 122 additions & 0 deletions plugins/web/opentelemetry-instrumentation-user-interaction/test/userInteraction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ const FILE_URL =
'https://raw.githubusercontent.com/open-telemetry/opentelemetry-js/main/package.json';

describe('UserInteractionInstrumentation', () => {
afterEach(() => {
while (document.body.lastChild) {
document.body.removeChild(document.body.lastChild);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is necessary? Perhaps a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added.

}
});

describe('when zone.js is available', () => {
let contextManager: ZoneContextManager;
let userInteractionInstrumentation: UserInteractionInstrumentation;
Expand Down Expand Up @@ -313,6 +319,122 @@ describe('UserInteractionInstrumentation', () => {
});
});

it('should handle interactions listened on document - react < 17', done => {
const btn1 = document.createElement('button');
btn1.setAttribute('id', 'btn1');
document.body.appendChild(btn1);
const btn2 = document.createElement('button');
btn2.setAttribute('id', 'btn2');
document.body.appendChild(btn2);

const listener = (event: MouseEvent) => {
switch (event.target) {
case btn1:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
case btn2:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
}
};

document.addEventListener('click', listener);

try {
btn1.click();
btn2.click();
} finally {
// remove added listener so we don't pollute other tests
document.removeEventListener('click', listener);
}

sandbox.clock.tick(1000);
originalSetTimeout(() => {
assert.equal(exportSpy.args.length, 4, 'should export 4 spans');

const span1: tracing.ReadableSpan = exportSpy.args[0][0][0];
const span2: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span3: tracing.ReadableSpan = exportSpy.args[2][0][0];
const span4: tracing.ReadableSpan = exportSpy.args[3][0][0];

assertClickSpan(span1, 'btn1');
assertClickSpan(span2, 'btn2');

assert.strictEqual(
span1.spanContext().spanId,
span3.parentSpanId,
'span3 has wrong parent'
);
assert.strictEqual(
span2.spanContext().spanId,
span4.parentSpanId,
'span4 has wrong parent'
);

done();
});
});

it('should handle interactions listened on a parent element (bubbled events) - react >= 17', done => {
const root = document.createElement('div');
document.body.appendChild(root);

const btn1 = document.createElement('button');
btn1.setAttribute('id', 'btn1');
root.appendChild(btn1);
const btn2 = document.createElement('button');
btn2.setAttribute('id', 'btn2');
root.appendChild(btn2);

root.addEventListener('click', event => {
switch (event.target) {
case btn1:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
case btn2:
getData(FILE_URL, () => {
sandbox.clock.tick(10);
}).then(() => {});
break;
}
});

btn1.click();
btn2.click();

sandbox.clock.tick(1000);
originalSetTimeout(() => {
assert.equal(exportSpy.args.length, 4, 'should export 4 spans');

const span1: tracing.ReadableSpan = exportSpy.args[0][0][0];
const span2: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span3: tracing.ReadableSpan = exportSpy.args[2][0][0];
const span4: tracing.ReadableSpan = exportSpy.args[3][0][0];

assertClickSpan(span1, 'btn1');
assertClickSpan(span2, 'btn2');

assert.strictEqual(
span1.spanContext().spanId,
span3.parentSpanId,
'span3 has wrong parent'
);
assert.strictEqual(
span2.spanContext().spanId,
span4.parentSpanId,
'span4 has wrong parent'
);

done();
});
});

it('should handle unpatch', () => {
const _window: WindowWithZone = window as unknown as WindowWithZone;
const ZoneWithPrototype = _window.Zone;
Expand Down