Reset the metrics after each emission#59551
Conversation
pgayvallet
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
| private readonly requests: OpsServerMetrics['requests'] = { | ||
| private requests: OpsServerMetrics['requests'] = { | ||
| disconnects: 0, | ||
| total: 0, | ||
| statusCodes: {}, | ||
| }; |
There was a problem hiding this comment.
dont want optional properties, and as calling reset in the constructor is not sufficient to have TS consider the variable as initialized, I'm forced to duplicate that in the reset function.
| const waitForNextEmission = () => | ||
| getOpsMetrics$() | ||
| .pipe(take(1)) | ||
| .toPromise(); | ||
|
|
||
| expect(mockOpsCollector.collect).toHaveBeenCalledTimes(1); | ||
| expect(mockOpsCollector.reset).toHaveBeenCalledTimes(1); | ||
|
|
||
| let nextEmission = waitForNextEmission(); | ||
| jest.advanceTimersByTime(testInterval); | ||
| await nextEmission; |
There was a problem hiding this comment.
private async refreshMetrics() {
this.logger.debug('Refreshing metrics');
const metrics = await this.metricsCollector!.collect();
this.metricsCollector!.reset();
this.metrics$.next(metrics);
}this.metricsCollector!.collect() is the first thing called in the interval handler, so waiting for advanceTimersByTime is sufficient, however as the reset call is performed after an async operation, the reset was not called on time for the test, so I had to actually wait for the observable emission
There was a problem hiding this comment.
does it make sense to add a comment in test?
There was a problem hiding this comment.
Probably. Will do
| private metricsCollector?: OpsMetricsCollector; | ||
| private collectInterval?: NodeJS.Timeout; | ||
| private metrics$ = new ReplaySubject<OpsMetrics>(1); | ||
| private metrics$ = new Subject<OpsMetrics>(); |
There was a problem hiding this comment.
With the metrics being reset after every emission, we probably don't want a replay effect anymore, so I switched back to a plain Subject. This also go closer to the legacy/oppsy implementation that was using events (therefor have no replay capabilities)
There was a problem hiding this comment.
It means that a subscriber doesn't receive the last emitted value. But okay, if it works in this way in LP
chrisronline
left a comment
chrisronline
left a comment
There was a problem hiding this comment.
I brought over the commit to my NP branch and the numbers match the old system!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* reset the metrics after each emission * add test comment
5 participants
Summary
Follow-up of #58623 after the discussion starting here #58623 (comment)
Resets the collectors state after each emission, to get back to the consumers' expected behavior
Checklist