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
refactor(ironfish): Clean up existing Telemetry into single service #966
Conversation
|
||
if (points.length < this.MAX_QUEUE_SIZE) { | ||
this.logger.debug('Retrying telemetry submission') | ||
this.points = points |
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.
This needs to be concatted or else you'll wipe out queued points while the request is pending
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.
Later, we should dump points if we fail after a certain amount of times.
export async function submitTelemetry({ | ||
points, | ||
}: SubmitTelemetryRequest): Promise<SubmitTelemetryResponse> { | ||
const api = new WebApi() |
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.
I just noticed we don't consistency configure this from somewhere to let us test against staging, or let other users point at their own endpoint.
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.
This is a good PR, we can continue working on it after.
03dce6d
to
83eace0
Compare
Summary
The existing telemetry implementation wasn't designed very intuitively. There was an existing
Telemetry
interface which was implemented byNodeTelemetry
andDisabledTelemetry
, each of which had a lot of side effects and returned new instances.This code change:
Metric
andField
from types to interfaces (inspired by TS docs)Testing Plan
Added unit tests and verified manually in InfluxDB for my
node_id
.Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.