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

[Uptime] Synthetic check steps list view #90978

Merged
merged 46 commits into from
Mar 16, 2021

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Feb 10, 2021

Summary

Fixes: #80156

Synthetic check steps list view

i still need to do last successful check in case of error steps and probably few other polishing to align with design

image

@shahzad31 shahzad31 marked this pull request as ready for review February 10, 2021 17:05
@shahzad31 shahzad31 requested a review from a team as a code owner February 10, 2021 17:05
@shahzad31 shahzad31 self-assigned this Feb 10, 2021
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Feb 10, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@paulb-elastic
Copy link
Contributor

I know it’s an early PR, but here’s some things I’ve noticed:

  1. Having clicked through, before the page has loaded, there is an error message suggesting there is no data:
    image

  2. You can click on the row to expand/collapse the step details, but the right hand expander only seems to open, not close (we did discuss and agree to remove this for Synthetic tests only - see comment):
    Expander

  3. Headline differs from the mock up (@katrin-freihofner, thoughts?)

  • Steps should probably be lowercase steps
  • It shows the number succeeded rather than failed (I think this is correct, as there can only ever be 1 step that fails)
    image
  1. We don’t show the last good image for a failed test yet (when added, there’s also a footer/label under each image with the date/time stamp (@katrin-freihofner do you only want to show these timestamps when we are displaying two images, as the time of the test itself is shown in the breadcrumb, or add it for all?)

  2. The click to see full size image has a problem with the labelling, but it doesn’t seem specific to this branch, I’ll raise a separate issue for it
    Left-RightImage

@shahzad31
Copy link
Contributor Author

@paulb-elastic Thank you so much for early feedback,

i have resolved your 1, 2, 4 points.

In case of 3, i preserved the already working logic from ping list where we were earlier displaying steps in expanded row.

For 5, i think you already created separate issue.

@shahzad31
Copy link
Contributor Author

After slack convo, accordions will collapsed by default for successful checks and expanded for failed.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! A few questions

@paulb-elastic
Copy link
Contributor

Thanks for the updates @shahzad31

A few other items to mention/discuss:

  1. The last good screenshot doesn’t seem to be shown (I can see the screenshot if I look at the older test result, so know there is one in ES):
    image

  2. There is no date/time stamp in the footer (as shown on the mock up):
    image

  3. Now that the whole row is clickable, but as discussed previously we still want to be able to separately click the thumbnail to get the full size screenshot, should we make the hover area for the thumbnail be the same area as the clickable region (as they are different)?
    image

This makes it a bit confusing in that when clicking with the larger screenshot hover, it clicks through to the step view (which is correct), even through the screenshot was being shown (it’s a bit difficult to explain). @katrin-freihofner what do you think, should we make these regions the same?

@shahzad31
Copy link
Contributor Author

@paulb-elastic added the timestamps for screenshots, those were tricky to spot 🗡️

image

@shahzad31
Copy link
Contributor Author

@paulb-elastic not sure about 3, about separating hover region and click. I think it's not a big deal.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this failed step has a thumbnail, but no images when expanded.
download (1)

Also, I find this layout a bit weird how it has the label step name above the thumbnail.
Screen Shot 2021-02-11 at 1 26 20 PM

},
{
term: {
'synthetics.payload.status': 'succeeded',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't work because we use enabled: false in our mapping for synthetics.payload. If it is working on your box I'd assume it's a broken mapping.

If we need to filter in this way we should add synthetics.step.status as a field. I've opened elastic/beats#24141 to track this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, our mapping was broken, only @paulb-elastic had it right all along.

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@shahzad31 shahzad31 closed this Mar 15, 2021
@shahzad31 shahzad31 reopened this Mar 15, 2021
@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
uptime 603 607 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
uptime 939.5KB 952.0KB +12.5KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @shahzad31

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Two quick things

The console output is open by default, even when nothing is populated. Is that intentional?

Screen Shot 2021-03-15 at 9 41 30 AM

Also, the icons in the script step are a bit cut off.
Screen Shot 2021-03-15 at 12 44 17 PM

@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 16, 2021
@shahzad31 shahzad31 merged commit 09e949e into elastic:master Mar 16, 2021
@shahzad31 shahzad31 deleted the synthetics-checks-view branch March 16, 2021 15:41
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Mar 16, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #94716

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 17, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime UI] Create new check details view
9 participants