-
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
[Synthetics 1.0] Monitor Details -> Monitor Summary -> Last test run panels #137059
[Synthetics 1.0] Monitor Details -> Monitor Summary -> Last test run panels #137059
Conversation
4d2550b
to
2126a46
Compare
Pinging @elastic/uptime (Team:uptime) |
2126a46
to
1d43936
Compare
1d43936
to
1b56356
Compare
@elasticmachine merge upstream |
</strong> | ||
</EuiText> | ||
), | ||
header: 'Step', |
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.
i18n
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 for catching.
import { EuiProgress, EuiText } from '@elastic/eui'; | ||
import { FormattedMessage } from '@kbn/i18n-react'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { euiStyled } from '@kbn/kibana-react-plugin/common'; |
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 we're transitioning to using emotion.
}; | ||
|
||
const BorderedTextLoading = euiStyled(EuiText)` | ||
display: flex; |
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.
Seems to be the same styles as above, so could be in a reusable variable.
border: 1px solid ${(props) => props.theme.eui.euiColorLightShade}; | ||
`; | ||
|
||
export const LoadingImageState = () => { |
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'd check with @hbharding about loading states. I think I introduced this loading state in the original Uptime app, but design wasn't happy with it.
import { StepImageCaption } from './step_image_caption'; | ||
import { StepImagePopover } from './step_image_popover'; | ||
|
||
const StepDiv = euiStyled.div` |
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.
use emotion if possible.
const POPOVER_IMG_HEIGHT = 360; | ||
const POPOVER_IMG_WIDTH = 640; | ||
|
||
const StepImage = euiStyled(EuiImage)` |
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.
use emotion if possible.
const { syntheticsMonitor, syntheticsMonitorLoading } = useSelector(selectorMonitorDetailsState); | ||
const dispatch = useDispatch(); | ||
|
||
const availableMonitor = monitorId |
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 nested ternary is hard to rease about. Can you refactor it as a switch or if else statement or a helper function of some sort.
<> | ||
<MonitorDetailsTabs /> | ||
</> |
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.
<> | |
<MonitorDetailsTabs /> | |
</> | |
<MonitorDetailsTabs /> |
} | ||
|
||
// TODO: Remove the following along with the rest route if not needed | ||
export const fetchMonitorStatus = async (params: QueryParams): Promise<Ping> => { |
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.
Looks like this isn't being used after all. Let's delete it.
import { | ||
ScreenshotBlockDoc, | ||
ScreenshotBlockCache, | ||
isPendingBlock, |
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.
Why is a helper function being imported from runtime_types?
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.
Considering isPendingBlock
is a type guard, runtime_types seems a suitable common place. (Also, isScreenshotBlockDoc
, another related type guard, was already exported in there)
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.
Lightweight Pings
Lightweight pings contain a hyperlink on the monitor time execution time. This links to the waterfall chart. Hyperlinks should be excluded for lightweight monitors as no further data is available for lightweight monitors in the waterfall chart.
Browser pings
The hyperlink for the waterfall chart in the last test run panel is broken. Notably, the check group appears to be wrong. The check group for the last test run seems to be correct, while the check group for the last test run reported in the last 10 panel is different.
Also, the screenshot in the table is missing. For reference, here is a screenshot from Figma.
Project monitors
Last 10 test runs doesn't work with project monitors4617f75
to
45c5423
Compare
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 great and works as expected.
Code is very clean and well organised.
Great work !!
Please do a follow up to add some e2e test journey for these panels .
I tested this with Browser monitors and an HTTP Monitors.
I can see that it's display a detail panel for ping monitors.
And display step lists for browser monitors.
f0eb58a
to
5b9eee7
Compare
…itor Summary page.
Improve loading spinner styles. Fix step's carousel on step image popover.
5b9eee7
to
5902b5f
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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
In Post-FF testing I've observed that the last test run info block at the top of the page isn't showing up for project monitors. There aren't any pending network requests in my browser either, so it's likely isolated to the client, or we're getting null data back and not handling it in the loading logic: It's working properly for the monitors I'm managing via the synthetics UI, and a private location: Aside from these project monitor issues, I'm seeing the appropriate data show up for my monitors as specified in the testing instructions. |
Raised #143059 as per @justinkambic's comment |
Summary
Tracked in #134204.
Adds the Last test run and Last 10 Test Runs panels on the Monitor Details -> Summary page. Both of the sections are added in one PR as the state is shared between the two UI sections.
How to test