-
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
[OBS-UX-MNGMT] - Add P95, P99, and RATE Aggs to the Custom Threshold rule #173728
[OBS-UX-MNGMT] - Add P95, P99, and RATE Aggs to the Custom Threshold rule #173728
Conversation
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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 tried to create a rule and I got this error:
Looking at the code, I don't see adding these new aggregations to the executor.
@simianhacker I remember you were talking about some minor differences between these aggregations and the rest. Would you please share those differences to ensure we check them during the PR review?
It would be good to add an API integration test for one of these integration (Maybe the one that has more differences based on Chris's input)
x-pack/plugins/observability/public/components/custom_threshold/components/expression_row.tsx
Outdated
Show resolved
Hide resolved
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.
Checking the commits, it seems there is a divergence. Woking on fixing it
@elasticmachine merge upstream |
@maryam-saeidi, the allowed Aggs were hard coded. Fixed here https://github.com/elastic/kibana/pull/173728/files#diff-2c2365096901ec4d3f525f2de7671eee30ae3e61da3925c6c94d257b006ea71cR88 |
@elasticmachine merge upstream |
.../observability/public/components/custom_threshold/components/preview_chart/preview_chart.tsx
Outdated
Show resolved
Hide resolved
.../observability/public/components/custom_threshold/components/rule_condition_chart/helpers.ts
Outdated
Show resolved
Hide resolved
export const lensFieldFormatter = ( | ||
metrics: CustomThresholdExpressionMetric[] | ||
): typeof LensFieldFormat[keyof typeof LensFieldFormat] => { | ||
if (metrics.length < 1 || !metrics[0].field) return LensFieldFormat.NUMBER; |
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.
Does it mean we will use the first metric's unit for the whole equation? Does it work for all the scenarios? (Not sure how we can check this.)
The safer option would be to only format when we have one metric but I cannot think of a scenario that this would create an issue. What do you think @simianhacker @maciejforcone @paulb-elastic?
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.
@maryam-saeidi I agree. We can go further. Even if we have more than one metric, we check if ALL have the same field type and then apply the format; otherwise, there is no format.
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 agree, if all the fields have the same formatter, then we format, otherwise leave it raw.
x-pack/plugins/observability/server/lib/rules/custom_threshold/lib/format_alert_result.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/observability/custom_threshold_rule/rate_bytes_fired.ts
Outdated
Show resolved
Hide resolved
start: moment(timeframe.end).subtract(metricParams.timeSize, metricParams.timeUnit).valueOf(), | ||
}); | ||
) => { | ||
const isRateAgg = metricParams.metrics.some((metric) => metric.aggType === Aggregators.RATE); |
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.
Difference in seconds is related to lastPeriod
(more info)
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @fkanout |
…rule (elastic#173728) ## Summary Fixes elastic#172945 by adding P95, P99, and RATE Aggs to the Custom Threshold rule <img width="575" alt="Screenshot 2023-12-20 at 12 31 50" src="https://github.com/elastic/kibana/assets/6838659/fd9f33fc-97c5-4cf8-b15e-3134048c861a"> <img width="565" alt="Screenshot 2023-12-20 at 12 31 57" src="https://github.com/elastic/kibana/assets/6838659/56047c84-a6e5-4a8a-a05f-b6c1358a7297"> <img width="601" alt="Screenshot 2023-12-20 at 12 36 16" src="https://github.com/elastic/kibana/assets/6838659/af7c291d-320f-4cb1-b62f-46e29f596bcd">
…rule (elastic#173728) ## Summary Fixes elastic#172945 by adding P95, P99, and RATE Aggs to the Custom Threshold rule <img width="575" alt="Screenshot 2023-12-20 at 12 31 50" src="https://github.com/elastic/kibana/assets/6838659/fd9f33fc-97c5-4cf8-b15e-3134048c861a"> <img width="565" alt="Screenshot 2023-12-20 at 12 31 57" src="https://github.com/elastic/kibana/assets/6838659/56047c84-a6e5-4a8a-a05f-b6c1358a7297"> <img width="601" alt="Screenshot 2023-12-20 at 12 36 16" src="https://github.com/elastic/kibana/assets/6838659/af7c291d-320f-4cb1-b62f-46e29f596bcd">
…rule (elastic#173728) ## Summary Fixes elastic#172945 by adding P95, P99, and RATE Aggs to the Custom Threshold rule <img width="575" alt="Screenshot 2023-12-20 at 12 31 50" src="https://github.com/elastic/kibana/assets/6838659/fd9f33fc-97c5-4cf8-b15e-3134048c861a"> <img width="565" alt="Screenshot 2023-12-20 at 12 31 57" src="https://github.com/elastic/kibana/assets/6838659/56047c84-a6e5-4a8a-a05f-b6c1358a7297"> <img width="601" alt="Screenshot 2023-12-20 at 12 36 16" src="https://github.com/elastic/kibana/assets/6838659/af7c291d-320f-4cb1-b62f-46e29f596bcd">
Summary
Fixes #172945 by adding P95, P99, and RATE Aggs to the Custom Threshold rule