feat: adding user telemetry config and service

[anandtiwary]

Description

Please include a summary of the change, motivation and context.
feat: adding user telemetry config and service

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]

Codecov Report

Merging #1126 (69962ee) into main (8a5ddbb) will decrease coverage by 0.02%.
The diff coverage is 78.18%.

@@            Coverage Diff             @@
##             main    #1126      +/-   ##
==========================================
- Coverage   85.10%   85.08%   -0.03%     
==========================================
  Files         822      825       +3     
  Lines       17011    17066      +55     
  Branches     2212     2216       +4     
==========================================
+ Hits        14478    14521      +43     
- Misses       2502     2514      +12     
  Partials       31       31              

Impacted Files	Coverage Δ
...ects/common/src/telemetry/user-telemetry.module.ts	0.00% <0.00%> (ø)
...mon/src/telemetry/user-telemetry-helper.service.ts	89.47% <89.47%> (ø)
...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 8a5ddbb...69962ee. Read the comment docs.

[aaron-steinfeld]

I thought we had gotten away from the concept of an Internal and external version of the service? Is the purpose to discourage programmatic usage by anyone but the directives (if yes - why? why prevent programmatic usage if the directive is not sufficient - that should be a shortcut rather than the only way this can be used).

[anandtiwary]

So, I want devs to use directives wherever they want to capture events. There is no reason why we need to expose it via service as well. In some cases, this could lead to duplicate events.

With custom events, we are trying to track user actions. Can you describe a use case where we would want to do it programmatically (Apart from page view and error handling that we have already covered)?

[aaron-steinfeld]

For example on a handled error - like an error server call that we caught and put a toast for. We're currently only capturing uncaught errors, right? Alternatively, some page event like an auto-refresh. User going idle perhaps? I feel like there's almost infinite possibilities and just don't see a huge benefit of funneling all usages into a single directive. IMO, that's provided as the 90% case, but the programmatic api should always be exposed by the lib (think angular's router-link but direct router navigate api).

[anandtiwary]

Yeah, so those programmatic things are not interesting metrics for "users". We are trying to capture user actions and that are more relevant for Product/maketing and ux.

The use cases you are describing are more relevant to UI (imo) and I have looked into this aspect in detail. My recommendation for UI and UX use cases were to use an "autotracking" based telemetry tool like fullstory, logRocket or Zipy. These tools would automatically capture much more info, like cpu/memory usage, network perf and the rest. I don't think we want to replicate all that for this framework, since they are not useful data for a tool like GA and mixpanel.

For any explicit trackEvent call, it will have to be thought through since some platforms charge on a per event storage basis.

So... let's wait for an actual use case to come. I am okay with exposing those methods. But let's do it when we have a use case.

[aaron-steinfeld]

Agree to disagree. Anyway, my main thing was about having an "internal" and "external" service. We could get around that with this pattern: https://angular.io/guide/dependency-injection-in-action#class-interface

[anandtiwary]

WIll add tests when we are 90ish % done

[aaron-steinfeld]

could you follow up on the comment about avoiding the two services

[anandtiwary]

could you follow up on the comment about avoiding the two services

So for this one, I will have to create an abstract class and export it. I am not sure if it looks any better than exporting the public service directly.

[aaron-steinfeld]

could you follow up on the comment about avoiding the two services

So for this one, I will have to create an abstract class and export it. I am not sure if it looks any better than exporting the public service directly.

It saves from having to create two implementations though - it just allows users to access the main implementation by a different token (in other words, provides an interface)

[anandtiwary]

My preference would be to roll with what I have latest. We can make improvements later

[anandtiwary]

This token approach would also allow any consumer to provide their own impl for this service. what are your thoughts on that?

[aaron-steinfeld]

This token approach would also allow any consumer to provide their own impl for this service. what are your thoughts on that?

You could rebind it with another, but only in the same way you could override an existing injection token always - same as in this implementation. (i.e. I could provide UserTelemetryService with a different implementation here, too - exactly the same)

In normal use, it just works how dependency injection is meant to work, I inject a class and don't care how it got there, what the specific impl is, or who put it there.

[anandtiwary]

With the token approach, I will have to provide this token inside forRoot method. That means I will have to expose the register method as well. I don't want that.

Let's stay with the two-service approach for now. We can change it later.

[aaron-steinfeld]

With the token approach, I will have to provide this token inside forRoot method. That means I will have to expose the register method as well. I don't want that.

Let's stay with the two-service approach for now. We can change it later.

We can change it later if you really want, that's fine - but providing the token inside forRoot doesn't expose register any more than it currently is. The diff basically looks like this as I understand it:

diff --git a/projects/common/src/public-api.ts b/projects/common/src/public-api.ts
index aae6ba44..c4477d64 100644
--- a/projects/common/src/public-api.ts
+++ b/projects/common/src/public-api.ts
@@ -112,3 +112,6 @@ export * from './time/time';
 
 // Validators
 export * from './utilities/validators/email-validator';
+
+export * from './telemetry/user-telemetry.module';
+export * from './telemetry/user-telemetry.service';
diff --git a/projects/common/src/telemetry/user-telemetry-helper.service.ts b/projects/common/src/telemetry/user-telemetry-helper.service.ts
index 6c0d6fd1..dd684e08 100644
--- a/projects/common/src/telemetry/user-telemetry-helper.service.ts
+++ b/projects/common/src/telemetry/user-telemetry-helper.service.ts
@@ -3,12 +3,14 @@ import { NavigationEnd, Router } from '@angular/router';
 import { filter } from 'rxjs/operators';
 import { Dictionary } from '../utilities/types/types';
 import { UserTelemetryProvider, UserTelemetryRegistrationConfig, UserTraits } from './telemetry';
+import { UserTelemetryService } from './user-telemetry.service';
 
 @Injectable({ providedIn: 'root' })
-export class UserTelemetryHelperService {
+export class UserTelemetryHelperService extends UserTelemetryService {
   private telemetryProviders: UserTelemetryInternalConfig[] = [];
 
   public constructor(private readonly injector: Injector, private readonly router: Router) {
+    super();
     this.setupAutomaticPageTracking();
   }
 
diff --git a/projects/common/src/telemetry/user-telemetry.module.ts b/projects/common/src/telemetry/user-telemetry.module.ts
index f35d2094..e0e8ca18 100644
--- a/projects/common/src/telemetry/user-telemetry.module.ts
+++ b/projects/common/src/telemetry/user-telemetry.module.ts
@@ -1,6 +1,7 @@
-import { Inject, ModuleWithProviders, NgModule, InjectionToken } from '@angular/core';
+import { Inject, InjectionToken, ModuleWithProviders, NgModule } from '@angular/core';
 import { UserTelemetryRegistrationConfig } from './telemetry';
 import { UserTelemetryHelperService } from './user-telemetry-helper.service';
+import { UserTelemetryService } from './user-telemetry.service';
 
 @NgModule()
 export class UserTelemetryModule {
@@ -20,6 +21,10 @@ export class UserTelemetryModule {
         {
           provide: USER_TELEMETRY_PROVIDER_TOKENS,
           useValue: providerConfigs
+        },
+        {
+          provide: UserTelemetryService,
+          useExisting: UserTelemetryHelperService
         }
       ]
     };
diff --git a/projects/common/src/telemetry/user-telemetry.service.ts b/projects/common/src/telemetry/user-telemetry.service.ts
index d5adf7aa..4ae7cb1a 100644
--- a/projects/common/src/telemetry/user-telemetry.service.ts
+++ b/projects/common/src/telemetry/user-telemetry.service.ts
@@ -1,17 +1,7 @@
-import { Injectable } from '@angular/core';
 import { UserTraits } from './telemetry';
-import { UserTelemetryHelperService } from './user-telemetry-helper.service';
 
-@Injectable({ providedIn: 'root' })
-export class UserTelemetryService {
-  public constructor(private readonly userTelemetryHelperService: UserTelemetryHelperService) {}
+export abstract class UserTelemetryService {
+  public abstract identify(userTraits: UserTraits): void;
 
-  public initialize(userTraits: UserTraits): void {
-    this.userTelemetryHelperService.initialize();
-    this.userTelemetryHelperService.identify(userTraits);
-  }
-
-  public shutdown(): void {
-    this.userTelemetryHelperService.shutdown();
-  }
+  public abstract shutdown(): void;
 }

• I didn't do any renames, but don't love the name helper. It's independent of the discussion and not exported, so can always be changed later (but I'd probably call it InternalUserTelemetryService or something)
• the public api changes in my diff are needed either way to use this change
• there's a slight api change to make the external api align with the internal one. These should match, and if the old external api was how we want it to be used we should change the internal api to match.

So I'll go ahead and approve since I know there are more PRs following where we can address these things.

[anandtiwary]

hmm.... so useExisting would inject it from the root. That makes sense. I was using useClass and for that I had to expose register as an abstract method too.

[anandtiwary]

I will take care of this in the follow up.

[github-actions]

Unit Test Results

    4 files  ±0  267 suites  +2   16m 34s ⏱️ -26s
962 tests +5  962 ✔️ +5  0 💤 ±0  0 ❌ ±0 
969 runs  +5  969 ✔️ +5  0 💤 ±0  0 ❌ ±0 

Results for commit 594a8db. ± Comparison against base commit 8a5ddbb.