-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Custom threshold rule] Use lens chart with annotations in alert details page #175513
[Custom threshold rule] Use lens chart with annotations in alert details page #175513
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/ci |
/ci |
/ci |
/ci |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
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.
Config builder changes are looking good! However, Lens really supports two distinct types of annotation layer: by-value and by-reference. You're currently using by-value.
Could we make that a bit more clear (e.g. rename xy_annotation_layer
to xy_by_value_annotation_layer
, have the class implement ChartLayer<XYByValueAnnotationLayer>
, etc)?
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.
Code looks good, added some comments. Will check it locally.
@@ -47,13 +58,15 @@ const getOperationTypeFromRuleAggType = (aggType: AggType): OperationType => { | |||
export const getBufferThreshold = (threshold?: number): string => | |||
(Math.ceil((threshold || 0) * 1.1 * 100) / 100).toFixed(2).toString(); | |||
|
|||
export function PreviewChart({ |
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.
PreviewChart
was a more clear name as we refer to this component in our discussion as the preview chart instead of RuleConditionChart
. Was the previous name confusing?
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.
The PreviewChart
works well in context of rule configuration, when the purpose is to show user preview of data flow so that can define appropriate threshold. In context of alert details page, we are showing data at the time alert triggered and some data before and after alert to analyse. I think RuleConditionChart
is a common name that would work for both purposes. We can rename it to something else to cover both use cases, so if you have any other suggestions, let me know.
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 see your point, no I don't have a better suggestion.
Would you mind renaming this component to PreviewChart in here to make it easier to find in the code?
import { RuleConditionChart as PreviewChart } from './components/rule_condition_chart/rule_condition_chart';
.../public/components/custom_threshold/components/rule_condition_chart/rule_condition_chart.tsx
Outdated
Show resolved
Hide resolved
.../public/components/custom_threshold/components/rule_condition_chart/rule_condition_chart.tsx
Outdated
Show resolved
Hide resolved
.../public/components/custom_threshold/components/rule_condition_chart/rule_condition_chart.tsx
Outdated
Show resolved
Hide resolved
...mponents/custom_threshold/components/alert_details_app_section/alert_details_app_section.tsx
Outdated
Show resolved
Hide resolved
...mponents/custom_threshold/components/alert_details_app_section/alert_details_app_section.tsx
Outdated
Show resolved
Hide resolved
...mponents/custom_threshold/components/alert_details_app_section/alert_details_app_section.tsx
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.
@maryam-saeidi For recovered alerts, I have only added range annotation, do you think we should add alert start annotation as well? Let me check regarding active alert annotation, why there is a gap. |
I remember we had it and I think it is a useful indication of when the alert started even if it is already recovered. |
@@ -0,0 +1,66 @@ | |||
/* |
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.
The attribute_builder
will be deprecated in the future in favor of the config_builder
created recently by the visualizations team.
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.
@crespocarlos thanks for the heads up. I will create a separate issue to replace attribute_builder
usage with config_builder
in observability plugin.
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.
created #176146
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.
thanks @benakansara !
@maryam-saeidi I have added alert start annotation for recovered alerts as well. For the issue with gap between two annotations, I have applied workaround to change series type to |
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.
key: { | ||
type: 'range', | ||
timestamp: alertStart!, | ||
endTimestamp: alertEnd ?? '', |
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.
Thinking out loud: If we pass endTimestamp
as undefined, would it be the same as the activeAlertRangeAnnotation
annotation? So we don't need to define two range annotations and one would be enough.
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.
yes, good idea. I updated the code in fa3df0d.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: cc @benakansara |
…ils page (elastic#175513) Resolves elastic#174075, elastic#155691 - Using same chart in alert details page as in rule flyout for preview - Group by info is applied as filter on chart data - Added annotations - For active alerts, added point annotation with alert start time and range annotation with range from alert start time till current time - For recovered alerts, added range annotation for alert duration ### Active alert <img width="1257" alt="Screenshot 2024-01-26 at 15 05 26" src="https://github.com/elastic/kibana/assets/69037875/d548b529-7811-4e3e-996d-aa4d259327b9"> <img width="1265" alt="Screenshot 2024-01-26 at 15 09 44" src="https://github.com/elastic/kibana/assets/69037875/a01d0cdf-5254-484d-91b3-c45a846790a9"> ### Recovered alert <img width="1250" alt="Screenshot 2024-01-26 at 14 53 08" src="https://github.com/elastic/kibana/assets/69037875/521a69a2-6a4a-4b9e-a886-3477c92d63ac"> --------- Co-authored-by: kibanamachine <[email protected]>
…ils page (elastic#175513) Resolves elastic#174075, elastic#155691 - Using same chart in alert details page as in rule flyout for preview - Group by info is applied as filter on chart data - Added annotations - For active alerts, added point annotation with alert start time and range annotation with range from alert start time till current time - For recovered alerts, added range annotation for alert duration ### Active alert <img width="1257" alt="Screenshot 2024-01-26 at 15 05 26" src="https://github.com/elastic/kibana/assets/69037875/d548b529-7811-4e3e-996d-aa4d259327b9"> <img width="1265" alt="Screenshot 2024-01-26 at 15 09 44" src="https://github.com/elastic/kibana/assets/69037875/a01d0cdf-5254-484d-91b3-c45a846790a9"> ### Recovered alert <img width="1250" alt="Screenshot 2024-01-26 at 14 53 08" src="https://github.com/elastic/kibana/assets/69037875/521a69a2-6a4a-4b9e-a886-3477c92d63ac"> --------- Co-authored-by: kibanamachine <[email protected]>
…ils page (elastic#175513) Resolves elastic#174075, elastic#155691 - Using same chart in alert details page as in rule flyout for preview - Group by info is applied as filter on chart data - Added annotations - For active alerts, added point annotation with alert start time and range annotation with range from alert start time till current time - For recovered alerts, added range annotation for alert duration ### Active alert <img width="1257" alt="Screenshot 2024-01-26 at 15 05 26" src="https://github.com/elastic/kibana/assets/69037875/d548b529-7811-4e3e-996d-aa4d259327b9"> <img width="1265" alt="Screenshot 2024-01-26 at 15 09 44" src="https://github.com/elastic/kibana/assets/69037875/a01d0cdf-5254-484d-91b3-c45a846790a9"> ### Recovered alert <img width="1250" alt="Screenshot 2024-01-26 at 14 53 08" src="https://github.com/elastic/kibana/assets/69037875/521a69a2-6a4a-4b9e-a886-3477c92d63ac"> --------- Co-authored-by: kibanamachine <[email protected]>
Resolves #174075, #155691
Active alert
Recovered alert