[taskManager plugin status] Do not emit empty status to status Observables
#171012
Conversation
gsoldevila
added
bug
| @@ -16,12 +16,12 @@ export function calculateHealthStatus( | |||
| config: TaskManagerConfig, | |||
| shouldRunTasks: boolean, | |||
| logger: Logger | |||
| ): { status: HealthStatus; reason?: string } { | |||
| ): { status: HealthStatus; reason?: string; isEmpty?: boolean } { | |||
There was a problem hiding this comment.
maybe instead of isEmpty, we could just have a new HealthStatus.NOT_INITIALIZED and filter on that instead?
There was a problem hiding this comment.
That sounds like a great idea!
| @@ -114,7 +115,8 @@ export function healthRoute(params: HealthRouteParams): { | |||
| }), | |||
| // Only calculate the summarized stats (calculates all running averages and evaluates state) | |||
| // when needed by throttling down to the requiredHotStatsFreshness | |||
| map((stats) => withServiceStatus(getHealthStatus(stats))) | |||
| map((stats) => withServiceStatus(getHealthStatus(stats))), | |||
| filter(([monitoredHealth]) => !monitoredHealth.isEmpty) | |||
There was a problem hiding this comment.
@ymao1 I haven't checked the impact of filtering at this level, i.e. whether we want the monitoredHealth$ observable to have the "NOT_INITIALIZED" events.
There's also a call to logHealthMetrics, but I imagine logging the "NOT_INITIALIZED" does not bring much value.
There was a problem hiding this comment.
Looking at the code, all the subscribers to the observable are trying to pull various stats from the monitoredHealth$.stats which would be empty anyway in this case, so I believe the impact is small. I'm unable to run it locally right now (poor internet connection) but will give it a go first thing next week.
|
Pinging @elastic/response-ops (Team:ResponseOps) |
gsoldevila
added
the
release_note:skip
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
## Summary Address #171164 The tests were assuming that the first event emitted is always `"degraded"`. After merging #171012 this might no longer be the case, aka the first event might already have an `"available"` status and we might have no more events in that case. This PR takes this enhancement into account and makes the test more robust: * Checking that an `"initializing"` event comes through first. * Checking that we eventually get an `"available"` event. --- Flaky Test Runner Pipeline - 100x 🟢 https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3981
Summary
Improvement over #169447, see related comment.