-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Uptime] Show troubleshoot guide on missing monitors #128527
Conversation
ebe72e1
to
dbc3e57
Compare
i added one more condition in the query, in case last date range is now in that case instead of converting timespan range to absolute value, it will keep it as now and now-5m, this means, even if kibana clock is out of sync, queries should work fine, since now will be calculated at elasticsearch, though it will not solve heartbeat clock issue, for that user will be show that trouble shoot guide |
<div style={{ width: '300px' }}> | ||
<EuiText size="s"> | ||
<p> | ||
<FormattedMessage |
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.
See note on improving text
Pinging @elastic/uptime (Team:uptime) |
</EuiText> | ||
</div> | ||
<EuiPopoverFooter> | ||
<EuiButton |
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.
Add the "calendar" icon to the button so its associated with the date time picker
return ( | ||
<EuiPopover | ||
button={ | ||
<EuiButtonEmpty iconType="arrowDown" iconSide="right" onClick={onButtonClick}> |
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.
remove icon from button. Could be confused as an accordion since it appears in the context of a table.
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 @shahzad31,
I wasn't able to test this locally, but I have a few suggestions. I recall seeing you demo this, so I'm picturing this in my head and recreated in Figma from memory. Apologies if I missed something.
We might not be able to address everything I'm proposing before FF, but I suggest we at least improve the popover text. Overall, great job!
Your last comment about a new condition
i added one more condition in the query, in case last date range is now in that case instead of converting timespan range to absolute value, it will keep it as now and now-5m, this means, even if kibana clock is out of sync, queries should work fine, since now will be calculated at elasticsearch, though it will not solve heartbeat clock issue, for that user will be show that trouble shoot guide
I'm not sure if I understand fully, but I want to confirm if this new condition impacts the text that we provide to the user. I'm assuming no.
Improve popover text
There are a few grammar and sentence structure mistakes I think we should improve in this PR. Overall, I also think we can shorten the text a bit. Copying @ollyhowell and @colleenmcginnis in case they have any input. But given the time and unless there are concerns, I propose we go with:
Element | Currently | Change to |
---|---|---|
Popover button | Troubleshoot | Where are my monitors? |
Popover title | Troubleshoot out of sync clock | System clock may be out of sync |
Popover text | If there is data in the pings over time chart and you dont see any monitors in monitor list. There might be an issue with your system clock, either on the system heartbeat is running or where kibana is running. You can try the absolute date range, if that shows missing monitors, then make sure to check out of sync system clock. |
Try using an absolute date range. If monitors still do not appear afterwards, there may be an issue with the system clock where Heartbeat or Kibana is installed. |
Popover action | Try absolute date range | Apply absolute date range |
Figma screenshot:
- For the popover action button, add the 'calendar' icon so that the action is associated with the date time picker
- I don't think we need the "arrowDown" icon for the popover button. Since this appears in the context of a table, it could be confused as an accordion.
- Ideally we could point the user in the right direction if it is indeed a system clock issue.
- If there is a troubleshooting guide that exists, we should point them there by adding: "For more information, read our (link)troubleshooting guide(/link)." at the end.
- If no documentation exists, we could say something more generic like: "Please consult your system administrator."
Consider not using a popover
The popover makes the "quick fix" button and information unnecessarily difficult to find and access. I haven't considered any reason why we should "hide" this, so please speak up if you had something in mind. Instead, we could simply use an EuiEmptyPrompt
inside the table when the condition is met. Something like:
This seems more consistent with how we handle empty prompts in other places in Kibana. If this is too much to do before FF or there is disagreement, we can address this later.
I'm a little wary of making the content so definite. There are a few reasons why this behavior can happen, one of which is time clock, but it may also not be time clock. So I'd prefer having a title that was closer to |
@dominiqueclarke sounds good to me. I updated the copy in the table from my previous comment to reflect that. |
I have updated the UI, i think using an empty state might be too strong here, Since this will only happen as an edge case, i feel like there might be other scenerio where there will be no monitors, so making it a definitive statement might be not correct here. I think we can have a more complete trouble shoot guide after this iteration , where we can cover some other things. |
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
Code review only
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @shahzad31 |
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 @shahzad31. LGTM!
(cherry picked from commit e05b216)
💔 Some backports could not be created
Manual backportTo create the backport manually run:
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit e05b216) Co-authored-by: Shahzad <[email protected]>
Summary
Fixes: elastic/uptime#465
It shows the troubleshoot guide in case user has data in pings over time chart and none in monitor list.
Testing