Skip to content
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.

Sign up for GitHub

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

[RAC] [Metrics UI] Include group name in the reason message #115171

Merged
merged 12 commits into from
Oct 19, 2021
Merged

[RAC] [Metrics UI] Include group name in the reason message #115171

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b91716d
[RAC] [Metrics UI] Include group name in the reason message
mgiota Oct 15, 2021
2d3c2c9
remove console log
mgiota Oct 15, 2021
8cd19ce
fix i18n errors
mgiota Oct 15, 2021
88fc6ee
fix more i18n errors
mgiota Oct 18, 2021
065de0a
fix i18n & check errors and move group to the end of the reason text
mgiota Oct 18, 2021
36f599b
Merge branch 'master' of github.com:elastic/kibana into 109610_group_…
mgiota Oct 18, 2021
3556efe
add empty lines at the end of translation files
mgiota Oct 18, 2021
aaee1c3
fix more i18n tests
mgiota Oct 18, 2021
6949af2
try to remove manually added translations
mgiota Oct 18, 2021
ee85ac3
Revert "try to remove manually added translations"
mgiota Oct 18, 2021
8989f6b
apply i18n_check fix and reorder values in the formatted reason
mgiota Oct 18, 2021
7f002c3
log threshold reformat reason message and move group info at the end
mgiota Oct 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions x-pack/plugins/infra/server/lib/alerting/common/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,17 @@ const thresholdToI18n = ([a, b]: Array<number | string>) => {
};

export const buildFiredAlertReason: (alertResult: {
groupName: string;
metric: string;
comparator: Comparator;
threshold: Array<number | string>;
currentValue: number | string;
}) => string = ({ metric, comparator, threshold, currentValue }) =>
}) => string = ({ groupName, metric, comparator, threshold, currentValue }) =>
i18n.translate('xpack.infra.metrics.alerting.threshold.firedAlertReason', {
defaultMessage:
'{metric} is {comparator} a threshold of {threshold} (current value is {currentValue})',
'{groupName}: {metric} is {comparator} a threshold of {threshold} (current value is {currentValue})',
values: {
groupName,
metric,
comparator: comparatorToI18n(comparator, threshold.map(toNumber), toNumber(currentValue)),
threshold: thresholdToI18n(threshold),
Expand All @@ -126,15 +128,17 @@ export const buildFiredAlertReason: (alertResult: {
});

export const buildRecoveredAlertReason: (alertResult: {
groupName: string;
metric: string;
comparator: Comparator;
threshold: Array<number | string>;
currentValue: number | string;
}) => string = ({ metric, comparator, threshold, currentValue }) =>
}) => string = ({ groupName, metric, comparator, threshold, currentValue }) =>
i18n.translate('xpack.infra.metrics.alerting.threshold.recoveredAlertReason', {
defaultMessage:
'{metric} is now {comparator} a threshold of {threshold} (current value is {currentValue})',
'{groupName}: {metric} is now {comparator} a threshold of {threshold} (current value is {currentValue})',
values: {
groupName,
metric,
comparator: recoveredComparatorToI18n(
comparator,
Expand All @@ -147,13 +151,15 @@ export const buildRecoveredAlertReason: (alertResult: {
});

export const buildNoDataAlertReason: (alertResult: {
groupName: string;
metric: string;
timeSize: number;
timeUnit: string;
}) => string = ({ metric, timeSize, timeUnit }) =>
}) => string = ({ groupName, metric, timeSize, timeUnit }) =>
i18n.translate('xpack.infra.metrics.alerting.threshold.noDataAlertReason', {
defaultMessage: '{metric} has reported no data over the past {interval}',
defaultMessage: '{groupName}: {metric} has reported no data over the past {interval}',
values: {
groupName,
metric,
interval: `${timeSize}${timeUnit}`,
},
Expand Down
13 changes: 10 additions & 3 deletions ...fra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) =
reason = results
.map((result) =>
buildReasonWithVerboseMetricName(
item,
result[item],
buildFiredAlertReason,
nextState === AlertStates.WARNING
Expand All @@ -151,19 +152,23 @@ export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) =
*/
// } else if (nextState === AlertStates.OK && prevState?.alertState === AlertStates.ALERT) {
// reason = results
// .map((result) => buildReasonWithVerboseMetricName(result[item], buildRecoveredAlertReason))
// .map((result) => buildReasonWithVerboseMetricName(item, result[item], buildRecoveredAlertReason))
// .join('\n');
}
if (alertOnNoData) {
if (nextState === AlertStates.NO_DATA) {
reason = results
.filter((result) => result[item].isNoData)
.map((result) => buildReasonWithVerboseMetricName(result[item], buildNoDataAlertReason))
.map((result) =>
buildReasonWithVerboseMetricName(item, result[item], buildNoDataAlertReason)
)
.join('\n');
} else if (nextState === AlertStates.ERROR) {
reason = results
.filter((result) => result[item].isError)
.map((result) => buildReasonWithVerboseMetricName(result[item], buildErrorAlertReason))
.map((result) =>
buildReasonWithVerboseMetricName(item, result[item], buildErrorAlertReason)
)
.join('\n');
}
}
Expand Down Expand Up @@ -199,13 +204,15 @@ export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) =
});

const buildReasonWithVerboseMetricName = (
groupName: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning to use groupName instead of item here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was influenced by log threshold where is called groupName. But you might be right. Since here in metrics we call it item, I could rename

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But item sounds very generic. Do you have a suggestion of how we could describe it better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know to be honest (naming is hard :D). I'm not sure either what item or resultItem refers to. My comment came more from the disparity between the variable used when calling the function (item) versus the argument name.

Regarding how to name it, I see two approaches:

  • Name it after the field used.
  • Name it based on its purpose in the buildReasonViewVerboseMetricName function.

For the first one, is this related to the kibana.alert.instance.id field used to indentify the alert? If that's the case we can call it instance or instanceId.

For the second one, I would do something on the line of reasonPrefix, reasonHeader or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afgomez Yep I got you. Well naming, it's tough.

Regarding your question, no it is not related to kibana.alert.instance.id. I would say it is the node name judging from this screenshot. In this case item is the host name which is Panagiotas-MBP
Screenshot 2021-10-18 at 09 31 50

In the code https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts#L114 item is basically inventoryItem.

I see 2 options:

  • I rename all occurrences of item to either nodeName or inventoryItem (I would say nodeName)
  • or I go with your 2nd suggestion and define a new variable const reasonPrefix = item; and pass it to the buildReasonViewVerboseMetricName function. But others would still wonder what is item in other places.

I think I'd better go with option 1. Does it sound good?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like metric_threshold_executor shares same code and they use group there https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts#L120, so I will use group for both files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will use group for both files

Perfect!

resultItem: any,
buildReason: (r: any) => string,
useWarningThreshold?: boolean
) => {
if (!resultItem) return '';
const resultWithVerboseMetricName = {
...resultItem,
groupName,
metric:
toMetricOpt(resultItem.metric)?.text ||
(resultItem.metric === 'custom'
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/translations/translations/zh-CN.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13383,15 +13383,15 @@
"xpack.infra.metrics.alerting.threshold.errorAlertReason": "Elasticsearch 尝试查询 {metric} 的数据时出现故障",
"xpack.infra.metrics.alerting.threshold.errorState": "错误",
"xpack.infra.metrics.alerting.threshold.fired": "告警",
"xpack.infra.metrics.alerting.threshold.firedAlertReason": "{metric} {comparator}阈值 {threshold}（当前值为 {currentValue}）",
"xpack.infra.metrics.alerting.threshold.firedAlertReason": "{groupName}: {metric} {comparator}阈值 {threshold}（当前值为 {currentValue}）",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also change the x-pack/plugins/translations/translations/ja-JP.json translation file, which is causing the i18n check failure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claudiopro Yep indeed these tests were failing. Ideally I should first fix these and then open my PR for review. Thanks for the review.

"xpack.infra.metrics.alerting.threshold.gtComparator": "大于",
"xpack.infra.metrics.alerting.threshold.ltComparator": "小于",
"xpack.infra.metrics.alerting.threshold.noDataAlertReason": "{metric} 在过去 {interval}中未报告数据",
"xpack.infra.metrics.alerting.threshold.noDataAlertReason": "{groupName}: {metric} 在过去 {interval}中未报告数据",
"xpack.infra.metrics.alerting.threshold.noDataFormattedValue": "[无数据]",
"xpack.infra.metrics.alerting.threshold.noDataState": "无数据",
"xpack.infra.metrics.alerting.threshold.okState": "正常 [已恢复]",
"xpack.infra.metrics.alerting.threshold.outsideRangeComparator": "不介于",
"xpack.infra.metrics.alerting.threshold.recoveredAlertReason": "{metric} 现在{comparator}阈值 {threshold}（当前值为 {currentValue}）",
"xpack.infra.metrics.alerting.threshold.recoveredAlertReason": "{groupName}: {metric} 现在{comparator}阈值 {threshold}（当前值为 {currentValue}）",
"xpack.infra.metrics.alerting.threshold.thresholdRange": "{a} 和 {b}",
"xpack.infra.metrics.alerting.threshold.warning": "警告",
"xpack.infra.metrics.alerting.threshold.warningState": "警告",
Expand Down