-
Notifications
You must be signed in to change notification settings - Fork 61
app/vmui: vmalert ui for logs #522
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
Conversation
3ed0814 to
c8dd6d1
Compare
Loori-R
left a comment
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.
Overall looks good! I left a few comments, mostly around code style and structure.
app/vmui/packages/vmui/src/components/ExploreAlerts/Notifier/style.scss
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/components/ExploreAlerts/Rule/index.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/components/ExploreAlerts/RuleHeader/style.scss
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/components/Main/Autocomplete/Autocomplete.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreNotifiers.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreNotifiers.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
6162b2b to
9fed494
Compare
Loori-R
left a comment
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.
LGTM. Thanks!
6720eeb to
bdbe818
Compare
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreNotifiers.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
app/vmui/packages/vmui/src/pages/ExploreAlerts/ExploreRules.tsx
Outdated
Show resolved
Hide resolved
dfa790d to
d9ae79d
Compare
5360ab6 to
3287c6f
Compare
arturminchukov
left a comment
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, LGTM!
144b38f to
f22b9f1
Compare
f22b9f1 to
68e8074
Compare
Reason for revert: this functionality is non-trivial, so it needs additional maintenance efforts in the future. This functionality is non-core for VictoriaLogs web UI (aka 'nice to have feature'). It is already provided by vmalert web UI ( https://docs.victoriametrics.com/victoriametrics/vmalert/#web ). I doubt we'll have enough capacity for providing high-quiality maintenance for this functionality. It is better to revert it instead of providing half-baked functionality for our users. Updates #522 Updates VictoriaMetrics/VictoriaMetrics#8989 Updates #5
|
@hagen1778 , @AndrewChubatiuk , this pull request has been reverted in the commit 88e9e3c . See the commit message description for details on why this commit has been reverted. TL;DR: this pull request implements non-essential functionality, which is already implemented partially at vmalert web UI. This functionality is non-trivial and requires a lot of code changes. These code changes may negatively impact code maintenance in the future and take non-trivial amounts of additional time for the maintenance. I doubt we'll be able to maintain this functionality at the usable level in the long run. @hagen1778 , @makasim please consider reverting the similar functionality at VictoriaMetrics web UI (see VictoriaMetrics/VictoriaMetrics#8989 ) because of the same reasons. |
|
It is very important that all the pull requests satisfy our development goals at https://docs.victoriametrics.com/victoriametrics/goals/ |
Describe Your Changes
Implements VMAlert UI tab for logs, depends on API from #5 and similar to VictoriaMetrics/VictoriaMetrics#8989
Checklist
The following checks are mandatory: