Skip to content
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

Enable alerts definitions using web sessions #16113

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

josejulio
Copy link
Member

@josejulio josejulio commented Oct 3, 2017

  • Aggregated (active/expired/rejected) web sessions

https://issues.jboss.org/browse/HAWKULAR-1255
screenshot from 2017-10-05 16-51-40
screenshot from 2017-10-05 16-51-15

@josejulio
Copy link
Member Author

@miq-bot add-label providers/middleware

@josejulio
Copy link
Member Author

@lucasponce Can you please take a look?

@lucasponce
Copy link
Contributor

@josejulio It looks good to me.
I have one doubt if web sessions metrics should have two conditions or just a simple threshold is enough.
I personally like to have a single condition (choosing the operator), to define a condition when expired matches the operator instead to chain two.
i.e.
An alert when web sessions > 40 or web session < 10 is confusing to me, as you would expect two different alerts on this case (I think).

But, I guess this should be confirmed by @abonas or John Doyle.

If there is not a clear requeriment, I would go for a single condition, it's easier and the same semantic can be implemented with two alerts (and I think as a user it will be more meaningful).

@lucasponce
Copy link
Contributor

Also, alerting offers more possibilities like range or rates conditions,

http://www.hawkular.org/docs/rest/rest-alerts-v1.html#Condition

But for a first introduction in MiQ of this feature I think a threshold can be enough.

@josejulio
Copy link
Member Author

@lucasponce updated the PR with your suggestions.
Also it nows depends on:
ManageIQ/manageiq-ui-classic#2309 and ManageIQ/manageiq-providers-hawkular#68

@miq-bot
Copy link
Member

miq-bot commented Oct 6, 2017

This pull request is not mergeable. Please rebase and repush.

@josejulio josejulio force-pushed the add_alerts_mw_web_sessions branch from cbe13b1 to 8a4cccb Compare October 11, 2017 16:37
@josejulio josejulio force-pushed the add_alerts_mw_web_sessions branch from 8a4cccb to 166116a Compare October 11, 2017 16:39
@josejulio
Copy link
Member Author

@aljesusg please review

@miq-bot
Copy link
Member

miq-bot commented Oct 11, 2017

Checked commit josejulio@166116a with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@aljesusg
Copy link
Member

LGFM @josejulio

@josejulio
Copy link
Member Author

Thanks, @chessbyte, can you review? thanks!

@chessbyte chessbyte requested a review from abonas October 14, 2017 15:16
@chessbyte
Copy link
Member

@abonas will review after your 👍

@abonas
Copy link
Member

abonas commented Oct 16, 2017

@miq-bot add_label alerts

@chessbyte chessbyte added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 16, 2017
@chessbyte chessbyte merged commit 7393bba into ManageIQ:master Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants