feat: using telemetry tracker in few atomic components #1129
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anandtiwary
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -9,7 +9,7 @@ import { UserTelemetryService } from './user-telemetry.service'; | |||
| export class UserTelemetryImplService extends UserTelemetryService { | |||
| private telemetryProviders: UserTelemetryInternalConfig[] = []; | |||
|
|
|||
| public constructor(private readonly injector: Injector, private readonly router: Router) { | |||
| public constructor(private readonly injector: Injector, @Optional() private readonly router?: Router) { | |||
There was a problem hiding this comment.
So, if I don't make Router optional then I get test errors wherever the atomic component (that uses htTrack) is used. Alternatively, I will have to provide Router Mock service everywhere.
There was a problem hiding this comment.
But shouldn't we be mocking the component in those tests? Why are they use the real track component?
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #1129 +/- ##
==========================================
+ Coverage 85.02% 85.04% +0.02%
==========================================
Files 829 831 +2
Lines 17175 17208 +33
Branches 2233 2235 +2
==========================================
+ Hits 14603 14635 +32
- Misses 2540 2541 +1
Partials 32 32
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| @@ -9,7 +9,7 @@ import { UserTelemetryService } from './user-telemetry.service'; | |||
| export class UserTelemetryImplService extends UserTelemetryService { | |||
| private telemetryProviders: UserTelemetryInternalConfig[] = []; | |||
|
|
|||
| public constructor(private readonly injector: Injector, private readonly router: Router) { | |||
| public constructor(private readonly injector: Injector, @Optional() private readonly router?: Router) { | |||
There was a problem hiding this comment.
But shouldn't we be mocking the component in those tests? Why are they use the real track component?
| @@ -13,7 +15,8 @@ describe('BreadcrumbsComponent', () => { | |||
| const createHost = createHostFactory({ | |||
| declareComponent: false, | |||
| component: BreadcrumbsComponent, | |||
| imports: [BreadcrumbsModule, HttpClientTestingModule, IconLibraryTestingModule], | |||
| imports: [BreadcrumbsModule, HttpClientTestingModule, IconLibraryTestingModule, RouterTestingModule], | |||
| declarations: [MockDirective(TrackDirective)], | |||
There was a problem hiding this comment.
So here's an example. The mocked track component should be sufficient, we shouldn't need the router testing module too (and in general, this test needs to be converted be shallow)
There was a problem hiding this comment.
The MockDirective still tries to inject Router since it is in the constructor.
There was a problem hiding this comment.
really? that seems off...
There was a problem hiding this comment.
Yep, they try to inject the services. I have seen it at few other places.
| @@ -10,7 +10,7 @@ import { ButtonRole, ButtonSize, ButtonStyle } from './button'; | |||
| changeDetection: ChangeDetectionStrategy.OnPush, | |||
| template: ` | |||
| <ht-event-blocker event="click" class="button-container" [enabled]="this.disabled"> | |||
| <button class="button" [ngClass]="this.getStyleClasses()"> | |||
| <button class="button" [ngClass]="this.getStyleClasses()" [htTrack] [htTrackLabel]="this.label"> | |||
There was a problem hiding this comment.
do we capture something like the route path or url here? Seems like it's worth not just tracking "Submit" for example, but where that submit is.
There was a problem hiding this comment.
Yes. URL change is tracked via router events and it can be enabled via enablePageTracking option
There was a problem hiding this comment.
but those are separate events. If I'm looking at a click event for a button labeled "submit" how do I know what page it's on? Or do the tools provide that by tracking the event timeline?
There was a problem hiding this comment.
These are the things we can capture and add to the "eventData" object from inside the telemetryImpl service. But AFAIU, the telemetry tools will timestamp these anyway, so we can connect the dots there. Let's iterate over this.
Description
Please include a summary of the change, motivation and context.
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:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.