-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Transforms: uses health information for alerting rule #152561
[ML] Transforms: uses health information for alerting rule #152561
Conversation
Pinging @elastic/ml-ui (:ml) |
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.
UI text LGTM. (Left one note on Pete's comment.)
@elasticmachine merge upstream |
import { PLUGIN, TRANSFORM_RULE_TYPE } from '../../../../common/constants'; | ||
import { transformHealthRuleParams, TransformHealthRuleParams } from './schema'; | ||
import { transformHealthServiceProvider } from './transform_health_service'; | ||
|
||
export interface BaseResponse { | ||
export interface TransformHealth { | ||
status: 'green' | 'unknown' | 'yellow' | 'red'; |
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.
In x-pack/plugins/transform/common/constants.ts
we have export type TransformHealth
which you could reuse here. To avoid a naming clash we could consolidate this a bit further. In x-pack/plugins/transform/common/types/transform_stats.ts
the full transform stats are defined where health is a part of. You could your interface here move to that file and make it part of TransformStats
where it's defined inline now.
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.
Updated in d1b6e8b
/** | ||
* TODO update types in the es client | ||
*/ | ||
type TransformGetTransformStats = TransformGetTransformStatsTransformStats & { |
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.
Maybe this can also be combined/cleaned up with what we have in x-pack/plugins/transform/common/types/transform_stats.ts
.
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.
Updated in d1b6e8b
await esClient.transform.getTransformStats({ | ||
transform_id: transformIds.join(','), | ||
}) | ||
).transforms as TransformGetTransformStats[]; |
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.
getTransformStats
itself has a try/catch block and might also return IHttpFetchError
so I'm not sure if we can assume there will also be .transforms
available.
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.
Sorry, I misread the code, it's wrapped the other way around so the try/catch is on the outer level when the stats are requested via API call. It looks like in this case there's then no try/catch at all, should we add one?
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.
If we are unable to fetch transform stats, the rule can't be executed. It's ok to throw an error because it'll end up in the Rules UI.
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.
Sounds good, thanks for clarifying!
…api-update' into ml-144158-transform-health-rule-api-update
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.
Tested and looks good, apart from a question I have around the behavior of mapping the enabled state of the previous error message check to the new unhealthy transform check.
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.
Tested and latest changes LGTM (pending Pete's question).
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.
Latest changes for the enabled
state of rules created with the previous config LGTM.
description: i18n.translate( | ||
'xpack.transform.alertTypes.transformHealth.healthCheckDescription', | ||
{ | ||
defaultMessage: 'Get alerts if a transform health status is not green.', |
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 was wondering if this should be consistent with the messaging in the transforms list, using Healthy
in place of green
, but Get alerts if a transform health status is not healthy.
reads a bit odd.
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @darnautov |
Summary
Resolves #144158
Replaces the "Errors in transform messages" check with "Unhealthy transform" that relies on the health info from the transform stats.
Alerting context has been extended with
health_status
andissues
properties for both checks.The "Errors in transform messages" check will be still working for existing rules if it's been set explicitly.
Example of the alert message:
Checklist