You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⚠️ Please note there is some outstanding work here to be carried out before merge, detailed below in Pending Work, these points don't require comment ⚠️
This PR closes#47201, and adapts the log rate page to the new sets of data visualisations.
Checklist
Use strikethroughs to remove checklist items you don't feel are applicable to this PR.
These screenshots and GIFs document the full functionality.
Pending work
Format all values in the y-axis to 3-digit numbers to try and keep anomaly chart widths as similar as possible
Remove "Overall anomaly score" from tooltip and change wording to "Max Anomaly Scores:
dataset 1 [max-score]" etc
Use "pretty" numbers (i.e. 35,000 would be 35k) (User facing)
The toolbar text needs adding ("Analyzed x log entries from...") (User facing)
"Analyze in ML" buttons need adding (User facing) (Will be deferred to when we stich the two big log rate PRs together)
Results should be sorted in the overall annotations chart tooltip (User facing)
Amend API integration test (Developer facing)
Code for acquiring derivations of data from the results (functions in data_formatters) could potentially be DRY'd up and in some places hoisted. Components like the expanded row will only calculate and memoize their data when actually opened and mounted, and all derivations are memoized, so this isn't major-major. (Developer facing) (Can be deferred post-FF).
(All the user facing work here is small, and will be completed on Monday).
Caveats / issues
Below are some caveats / issues, I don't think any of them are deal breakers. And some are just here to gain a majority opinion on how to proceed.
The anomalies charts render annotations for anomalies with a severity score of warning, minor, major and critical. But not low (anything below 3). However, all anomaly scores do contribute to the "Top anomaly score" stat. This means you could see a stat of 2 for "Top anomaly score" but no anomaly annotations on the chart. Removing it from the "Top anomaly score" stat seems misleading. Should we add low severity score annotations to the chart? (These were omitted to avoid too much noise). Should we discount low scores from the "Top anomaly score"? - Result: we will keep all severity gradings (warning, minor, major, critical)
The widths of all the charts don't align 100%. This seems quite hard to achieve due to there not being a way to give an explicit width to the chart legend - technically we can hack some surrounding CSS in, but I'm not confident this won't break chart rendering in certain situations. Furthermore, the width of the Y axis can differ slightly based on the values. Even if we were to use a set scale, and "pretty" / formatted numbers, we'd likely want - for example - 10 and 100 to be graphed in the same scale. But if one anomaly chart has a highest value of 8, and another is 10, the pixel width will still be slightly out. - Result: values will all be rendered using 3-digits
The loading states are pretty jarring when auto reload is turned on. @jasonrhodes had mentioned other teams were potentially looking into a way to display a placeholder on top of graphs for this sort of situation.
@weltenwort Thank you for the thorough review, especially for catching things like the missing translations 😬
most of the comments just refer to cosmetics, so feel free to disregard or defer in the interest of getting things done.
I've responded to almost all of the feedback, the bits I've left are the parts that had questions around them and open comments. As they're not blockers I feel we can defer those changes.
Regarding the data formatters: I know you left it out intentionally in the interest of implementation speed. If we wanted to improve performance and readability it would probably make sense to offer a data structure that groups the results by partition first and histogram second in addition to the current structure from the results hook. These filters and transformations should become a lot simpler after that. We can do that in a follow-up, of course. 😉
Similarly, all the empty-string checks and 'unknown' transformations suggest to me that we should probably do that earlier or factor it out into a normalization function at some later point.
Yep, totally agree with all of this 👍
I'll move these improvements, and all other improvements, to a ticket so nothing gets lost.
Builds seem to have been quite flaky here - some were caused by influx work, but the vast majority are being caused by a Java exception in CI (or that's what it looks like).
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR closes #47201, and adapts the log rate page to the new sets of data visualisations.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Detailed overview
These screenshots and GIFs document the full functionality.
Pending work
dataset 1 [max-score]" etc
"Analyze in ML" buttons need adding(User facing) (Will be deferred to when we stich the two big log rate PRs together)data_formatters) could potentially be DRY'd up and in some places hoisted. Components like the expanded row will only calculate and memoize their data when actually opened and mounted, and all derivations are memoized, so this isn't major-major. (Developer facing) (Can be deferred post-FF).(All the user facing work here is small, and will be completed on Monday).
Caveats / issues
Below are some caveats / issues, I don't think any of them are deal breakers. And some are just here to gain a majority opinion on how to proceed.
The anomalies charts render annotations for anomalies with a severity score of- Result: we will keep all severity gradings (warning, minor, major, critical)warning,minor,majorandcritical. But notlow(anything below 3). However, all anomaly scores do contribute to the "Top anomaly score" stat. This means you could see a stat of 2 for "Top anomaly score" but no anomaly annotations on the chart. Removing it from the "Top anomaly score" stat seems misleading. Should we addlowseverity score annotations to the chart? (These were omitted to avoid too much noise). Should we discountlowscores from the "Top anomaly score"?The widths of all the charts don't align 100%. This seems quite hard to achieve due to there not being a way to give an explicit width to the chart legend - technically we can hack some surrounding CSS in, but I'm not confident this won't break chart rendering in certain situations. Furthermore, the width of the Y axis can differ slightly based on the values. Even if we were to use a set scale, and "pretty" / formatted numbers, we'd likely want - for example - 10 and 100 to be graphed in the same scale. But if one anomaly chart has a highest value of 8, and another is 10, the pixel width will still be slightly out.- Result: values will all be rendered using 3-digitsThe loading states are pretty jarring when auto reload is turned on. @jasonrhodes had mentioned other teams were potentially looking into a way to display a placeholder on top of graphs for this sort of situation.
I think that's everything!