-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Alerting: Loki-based alert state history modal #66595
Conversation
Co-Authored-By: Konrad Lalik <konrad.lalik@grafana.com>
Co-Authored-By: Konrad Lalik <konrad.lalik@grafana.com>
…-state-history-modal
…na/grafana into alerting/alert-state-history-modal
public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx
Outdated
Show resolved
Hide resolved
> | ||
<StateHistory alertId={alertId} /> | ||
<Suspense fallback={'Loading...'}> |
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.
👏
public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx
Outdated
Show resolved
Hide resolved
builder.setSync(); | ||
const interpolator = builder.getTooltipInterpolator(); | ||
|
||
// I found this in TooltipPlugin.tsx |
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.
This is awesome!! 🚀 ☄️ Left a few suggestions/comments.
public/app/features/alerting/unified/hooks/useStateHistoryModal.tsx
Outdated
Show resolved
Hide resolved
public/app/features/alerting/unified/components/rules/state-history/common.ts
Outdated
Show resolved
Hide resolved
public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx
Outdated
Show resolved
Hide resolved
instancesFilter | ||
); | ||
|
||
const frameSubset = useMemo(() => take(dataFrames, MAX_TIMELINE_SERIES), [dataFrames]); |
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.
any reason why you're using lodash's take
instead of just slice
?
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 think some of my personal bias towards using Lodash / more readable code entered in to this (which is basically executing slice(0, n)
.
There's definitely a trade-off here since some engineers might be more familiar with the std library than lodash's functions but hopefully the function names are descriptive enough.
public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx
Outdated
Show resolved
Hide resolved
public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx
Show resolved
Hide resolved
…-state-history-modal
…story/common.ts Co-authored-by: Virginia Cepeda <virginia.cepeda@grafana.com>
…na/grafana into alerting/alert-state-history-modal
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.
Incredible work! 🎉🎉🎉🎉🎉🎉🚀🚀🚀🚀🚀🚀
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.
My review here is a bit biased since I co-authored some parts; but have raised some (non-blocking) points that I think we could address from a maintenance perspective.
public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx
Outdated
Show resolved
Hide resolved
public/app/features/alerting/unified/components/rules/state-history/LogTimelineViewer.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.
1 test still needs fixing or might be flakey but code LGTM
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.
🚀 🚀 🚀
What is this feature?
This PR adds a new Alert State History modal which taps into the new Loki-based history API
Minor improvements:
AlertStateTag
componentWhy do we need this feature?
To provide better insights into changes in alert instances over time
Fixes #63656
Special notes for your reviewer:
The new modal is used when the state history backend engine is set to
Loki
. Otherwise, the old one will be usedOLD ASH MODAL
![image](https://user-images.githubusercontent.com/6293563/232069695-c7679322-4c16-44bb-b58e-9d9c7cf35b62.png)
NEW ASH MODAL
![image](https://user-images.githubusercontent.com/6293563/232069772-b22ac7ea-65b2-4ae4-989e-f8b3b4366f6f.png)
Please check that: