-
Notifications
You must be signed in to change notification settings - Fork 20
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
TNT-41046 Encapsulate telemetry reporting to make it easy to switch storage layer #50
Conversation
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.
Looks great! Just a couple minor comments
Added the edits! |
@@ -28,22 +27,12 @@ describe("TelemetryProvider", () => { | |||
} | |||
}) | |||
); | |||
expect(mockExecute.mock.calls[0][1][0].execution).toBe(1); |
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.
Minor nitpick for sake of consistency - let's remove this line and add it into the previous assertion:
expect.objectContaining({........, execution: 1 })
Description
Telemetry related logic was moved out of the target-decisioning-engine notificationProvider and into target-tools telemetryProvider.
The TelemetryProvider stores telemetry entries, and can add them to a DeliveryRequest.
Currently the implementation was kept relatively the same. The NotificationProvider has a TelemetryProvider, using it to store entries, and to add telemetries before sending notifications.
Related Issue
Jira Ticket
Motivation and Context
Description from Jira Ticket:
Currently telemetry reporting is done through Delivery API and makes its way into Prometheus; reporting and storage are tightly coupled in the SDKs. We should decouple them and create an abstract storage service in case we switch to a different storage layer in the future.
How Has This Been Tested?
Tests were added for the new Telemetry Provider, just ensuring it stores entries and adds them to a request.
Tests were run with
npm run test
and written using jest.This implementation will impact how the notification provider stores telemetry entries by encapsulating telemetry related logic in a separate class.
Types of changes
Checklist: