-
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
[ML] Add annotation markers to the Anomaly Swimlane axis #97202
Conversation
Noticed that with just a few annotations and without further labeling it might not be that obvious for users what they are looking at: Here's a quick&dirty suggestion how to improve:
|
@@ -214,6 +220,7 @@ const loadExplorerDataProvider = ( | |||
tap(explorerService.setChartsDataLoading), | |||
mergeMap( | |||
({ | |||
allAnnotations, |
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.
Wondering if we should rename everything related to allAnnotations
to overallAnnotations
. "all" might be a bit misleading since ANNOTATIONS_TABLE_DEFAULT_QUERY_SIZE = 500
limits how many annotations we fetch and it's not really "all".
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 here bc00e98
(#97202)
Agree with @walterra that the annotations would benefit from a label, and I like his proposed styling with border (or single line). I guess that the legend is part of the swim lane chart, so it may be more practical to stick with your current layout where the annotations are below the Overall swim lane. The Overall timeline is probably more important information that the annotations, so again it makes sense to make that the top element in the view. |
@walterra @peteharverson I have updated the styling to include the border and label on the y axis here. The result is definitely much more pleasing! |
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.
Tested and overall looks good. Just found a few styling / range issues.
chartElement.selectAll('*').remove(); | ||
|
||
const startingXPos = Y_AXIS_LABEL_WIDTH + 2 * Y_AXIS_LABEL_PADDING; | ||
const endingXPos = startingXPos + chartWidth - Y_AXIS_LABEL_PADDING; |
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.
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.
After discussing with Dima, we think the best way to fix this issue is through some modifications with the elastic-charts component itself. We can provide a callback function that exposes the positions of the heatmap cells whenever the swimlane is resized. Added this to the meta issue for Annotations.
x-pack/plugins/ml/public/application/explorer/swimlane_annotation_container.tsx
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/explorer/swimlane_annotation_container.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/explorer/swimlane_annotation_container.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.
Tested latest edits and LGTM. Would be good to align the ends of the annotation bar and the swim lanes, but it is only off by a few pixels so can be addressed separately.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @qn895 |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…7708) Co-authored-by: Quynh Nguyen <[email protected]>
Summary
This PR addresses #93463 and adds annotation markers to the Anomaly swimlane within the Anomaly Explorer page.
Checklist
Delete any items that are not applicable to this PR.