feat: adding error handler and track directive #1127

anandtiwary · 2021-09-15T20:09:58Z

Description

Please include a summary of the change, motivation and context.
feat: adding error handler and track directive

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

codecov · 2021-09-15T20:17:37Z

Codecov Report

Merging #1127 (1296635) into main (be9edf4) will increase coverage by 0.02%.
The diff coverage is 88.67%.

@@            Coverage Diff             @@
##             main    #1127      +/-   ##
==========================================
+ Coverage   85.18%   85.20%   +0.02%     
==========================================
  Files         825      827       +2     
  Lines       17082    17121      +39     
  Branches     2216     2222       +6     
==========================================
+ Hits        14551    14588      +37     
- Misses       2500     2502       +2     
  Partials       31       31

Impacted Files	Coverage Δ
...ects/common/src/telemetry/user-telemetry.module.ts	`0.00% <0.00%> (ø)`
...ry/error-handler/telemetry-global-error-handler.ts	`100.00% <100.00%> (ø)`
...ects/common/src/telemetry/track/track.directive.ts	`100.00% <100.00%> (ø)`
...ommon/src/telemetry/user-telemetry-impl.service.ts	`90.00% <100.00%> (ø)`
...cts/common/src/telemetry/user-telemetry.service.ts	`100.00% <100.00%> (ø)`

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 be9edf4...1296635. Read the comment docs.

aaron-steinfeld · 2021-09-22T15:56:13Z

projects/common/src/telemetry/telemetry.ts

@@ -28,3 +28,8 @@ export interface UserTraits extends Dictionary<unknown> {
  name?: string;
  displayName?: string;
 }
+
+export const enum TrackUserEventsType {
+  Click = 'click',


Any reason to limit to these two events?

It makes sense to keep it flexible (string), but I want to expand this list after I test their behavior, how many events they generate and any perf impact. From Product/Marketing standpoint, the 4 event types we discussed previously are important. Events like MouseOver, MouseOut are not useful.

aaron-steinfeld · 2021-09-22T15:57:31Z

projects/common/src/telemetry/error-handler/telemetry-global-error-handler.ts

+import { ErrorHandler, Injectable, Injector } from '@angular/core';
+import { UserTelemetryImplService } from '../user-telemetry-impl.service';
+
+@Injectable()


This should be registered as part of the telemetry module, right? or intentionally keeping it as a separate registration?

No, I need to provide it in the module. I missed it in this PR

projects/common/src/telemetry/track/track.directive.ts

aaron-steinfeld · 2021-09-22T16:01:36Z

projects/common/src/telemetry/track/track.directive.ts

+    }
+
+    if (changes.label) {
+      this.trackedEventLabel = this.label ?? (this.host.nativeElement as HTMLElement)?.tagName;


tag name doesnt seem very useful, maybe we should require label? Could make that the main arg and the event name the addtl arg since that's easier to default.

So, the autotrack based telemetry tools extract certain fields from the element. I am yet to figure out the relevant ones. After I do that, I will add those fields here.

Imo htTrack= "events" is more readable. You can always miss providing a directive input and angular won't complain. So let's keep the default logic. I will use a better field than just with a later PR.

aaron-steinfeld · 2021-09-27T03:05:43Z

projects/common/src/telemetry/track/track.directive.ts

    this.userTelemetryImplService.trackEvent(`${userEvent}: ${this.trackedEventLabel}`, {
-      target: eventObj.target,
+      ...(eventObj.target as HTMLElement),


why are you spreading the whole element onto the event?

I wasn't sure what all fields would be interesting. Anyway, I have added few properties only now.

aaron-steinfeld · 2021-09-27T13:52:46Z

projects/common/src/telemetry/track/track.directive.ts

    this.userTelemetryImplService.trackEvent(`${userEvent}: ${this.trackedEventLabel}`, {
-      ...(eventObj.target as HTMLElement),
+      tagName: targetElement.tagName,
+      innerHTML: targetElement.innerHTML,


This still feels like over-reporting (and potentially can report PII data). Why do we need the innerHTML of a clicked (for example) element? Can we start minimal and add stuff as use cases come up?

I can remove innerHtml. But btw, you need to put controls in your telemetry tool to protect against storing pii. It shouldn't be completely dependent on the code

github-actions · 2021-09-27T19:26:33Z

Unit Test Results

    4 files 270 suites 17m 35s ⏱️
968 tests 968 ✔️ 0 💤 0 ❌
975 runs 975 ✔️ 0 💤 0 ❌

Results for commit 6ed8f86.

anandtiwary added 2 commits September 15, 2021 13:06

feat: adding user telemetry config and service

99388e6

feat: adding error handler and a track directive

76ef6f3

This comment has been minimized.

Sign in to view

Base automatically changed from user-telemetry-1 to main September 21, 2021 21:24

anandtiwary added 6 commits September 21, 2021 15:04

Merge branch 'main' into user-telemetry-2

aa91d22

refactor: using abstract service approach

6161655

refactor: updating api for htTrack directive

a8b308d

revert: package-lock.json

9ce27b3

revert: revert package.json

1358760

revert: revert package.json peer deps

8395555

anandtiwary marked this pull request as ready for review September 22, 2021 00:44

anandtiwary requested a review from a team as a code owner September 22, 2021 00:44

This comment has been minimized.

Sign in to view

anandtiwary requested a review from aaron-steinfeld September 22, 2021 00:45

anandtiwary changed the title ~~feat: adding error handler and a track directive~~ feat: adding error handler and track directive Sep 22, 2021

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Sep 22, 2021

View reviewed changes

refactor: adding tests and addressing commments

4db25a3

anandtiwary requested a review from a team September 25, 2021 00:06

This comment has been minimized.

Sign in to view

refactor: fix lint errors

8abcc0a

anandtiwary requested a review from aaron-steinfeld September 26, 2021 18:20

aaron-steinfeld reviewed Sep 27, 2021

View reviewed changes

refactor: adding only few properties

adf577b

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Sep 27, 2021

View reviewed changes

refactor: addressing review comments

42bddca

This comment has been minimized.

Sign in to view

Merge branch 'main' into user-telemetry-2

1296635

anandtiwary requested review from aaron-steinfeld and a team and removed request for aaron-steinfeld September 27, 2021 16:56

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes Sep 27, 2021

View reviewed changes

anandtiwary merged commit 6ed8f86 into main Sep 27, 2021

anandtiwary deleted the user-telemetry-2 branch September 27, 2021 19:21

feat: adding error handler and track directive #1127

feat: adding error handler and track directive #1127

Uh oh!

Conversation

anandtiwary commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Documentation

Uh oh!

codecov bot commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Sep 27, 2021

Unit Test Results

Uh oh!

Uh oh!

anandtiwary commented Sep 15, 2021 •

edited

Loading

codecov bot commented Sep 15, 2021 •

edited

Loading