-
Notifications
You must be signed in to change notification settings - Fork 11
fix: running telemetry methods on initialized list only #1163
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.
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
Conversation
@@ -29,31 +30,32 @@ export class UserTelemetryImplService extends UserTelemetryService { | |||
} | |||
|
|||
public initialize(): void { | |||
this.telemetryProviders.forEach(provider => provider.telemetryProvider.initialize(provider.initConfig)); | |||
this.allTelemetryProviders.forEach(provider => provider.telemetryProvider.initialize(provider.initConfig)); | |||
this.initializedTelemetryProviders = [...this.allTelemetryProviders]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are we trying to solve here? were we calling before initializing? That's the only behavior that I see that would change here. Wouldn't that imply a misconfig though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we don't 'initialize' telemetry when FF is disabled or when user is an internal user. But the track directive still pushes click events on the service. Our E2E tests fails when it sees any console error. In this case, the third party lib throws an error since we tried to send an 'event' without initializing the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other alternative is to put an init check in every method that will run every time the track event methods are called. We can avoid that by keeping a separate initialized list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, a perf hit. But an ideal solution would be to load the entire module only if the feature flag is enabled.
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #1163 +/- ##
==========================================
- Coverage 85.04% 85.04% -0.01%
==========================================
Files 831 831
Lines 17207 17210 +3
Branches 2235 2235
==========================================
+ Hits 14634 14636 +2
- Misses 2541 2542 +1
Partials 32 32
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. Will take this approach for now, in the future we should investigate splitting behavior so that we can differentiate between all of:
- enabled but not ready (error any calls)
- disabled (drop calls)
- enabled (forward calls)
Description
Please include a summary of the change, motivation and context.
fix: running telemetry methods on initialized list only
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.