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

[APM] Service overview: Instances table metadata foldout #96467

Merged
merged 24 commits into from
Apr 20, 2021

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Apr 7, 2021

closes #94412

Screen Shot 2021-04-07 at 13 38 07

Screen Shot 2021-04-07 at 14 36 39

Screen Shot 2021-04-07 at 13 37 59

@cauemarcondes cauemarcondes added release_note:enhancement v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 7, 2021
@cauemarcondes cauemarcondes marked this pull request as ready for review April 13, 2021 17:48
@cauemarcondes cauemarcondes requested a review from a team as a code owner April 13, 2021 17:48
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Apr 13, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Copy link
Contributor

@smith smith 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!

@formgeist
Copy link
Contributor

@cauemarcondes Looks great! Really some great new capabilities we're gaining with this. I noticed some minor visual nits around the table that I wanted to share...

Screenshot 2021-04-14 at 13 20 27

Looks like the Actions button width is a bit off, so the icon appears off-center. Only noticed this when clicking the icon and popover shows and the focus state is enabled.

Screenshot 2021-04-14 at 13 20 48

Screenshot 2021-04-14 at 13 21 06

The height of the table rows is inconsistent, not sure why this is - compared to the Transactions table on the same page (which also has sparklines). I'd prefer this to follow the existing tables on the row height if that's possible.

@cauemarcondes
Copy link
Contributor Author

Fixes:
Screen Shot 2021-04-14 at 16 02 33
Screen Shot 2021-04-14 at 16 31 01

@cauemarcondes cauemarcondes requested a review from formgeist April 14, 2021 20:33
Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

@cauemarcondes Thanks for making those changes! I found that the actions button icon still looks like a different size and the focus state is around the icon itself.

Screenshot 2021-04-15 at 09 28 09

Screenshot 2021-04-15 at 09 51 42

vs. the accordion button icon

Screenshot 2021-04-15 at 09 28 13

Screenshot 2021-04-15 at 09 52 12

Layout changes

I think one of the enhancements that was missing from the original issue was changing the layout so the two panels would be using column flex group setting to ensure that the instances table wouldn't just expand down aside the other viz panel, but you'd be able to keep the visualization on top instead. I've attempted to mock this in the UI;

Screenshot 2021-04-15 at 09 49 53

This requires us to also set a fixed height (like the latency chart in the top) to 200 to avoid it taking up too much space. The new layout for this section will also help when you have a lot of instances mapped in the visualization especially when there's comparisons in there as well. I just simply changed the EuiFlexGroup direction to column.

Lastly, there's a missing tooltip on the filter action in the service, cloud, and orchestration info tables. The copy could just be "Filter by value".

Screenshot 2021-04-15 at 09 47 04

@formgeist
Copy link
Contributor

One more thing; we should show which column it's sorted by default (traffic).

Screenshot 2021-04-15 at 10 47 27

@cauemarcondes
Copy link
Contributor Author

Fixes:
Action button:
Screen Shot 2021-04-15 at 11 29 29

Table below chart:
Screen Shot 2021-04-15 at 11 30 03

Sort direction:
Screen Shot 2021-04-15 at 11 36 26

@sorenlouv
Copy link
Member

retest

@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

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

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@cauemarcondes
Copy link
Contributor Author

jenkins, retest this please

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@cauemarcondes
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
apm 1577 1583 +6

Async chunks

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

id before after diff
apm 4.2MB 4.2MB +17.6KB

History

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

@cauemarcondes cauemarcondes merged commit 6b70784 into elastic:master Apr 20, 2021
@cauemarcondes cauemarcondes deleted the apm-instances-table branch April 20, 2021 22:27
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 20, 2021
* shows instance details

* shows instance details

* shows instance details

* shows instance details

* shows instance details

* adding api test

* addressing PR comments

* addressing PR comments

* addressing PR comments

* addressing PR comments

* fixing ts issues

* fixing ci

* fixing api tests

* fixing api test

* fixing api test

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 21, 2021
…7763)

* shows instance details

* shows instance details

* shows instance details

* shows instance details

* shows instance details

* adding api test

* addressing PR comments

* addressing PR comments

* addressing PR comments

* addressing PR comments

* fixing ts issues

* fixing ci

* fixing api tests

* fixing api test

* fixing api test

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Cauê Marcondes <[email protected]>
@cauemarcondes
Copy link
Contributor Author

e2e added here #99098

@ogupte ogupte self-assigned this May 10, 2021
@ogupte ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label May 24, 2021
.filter((section) => !isEmpty(section.actions))
)
.filter((sections) => !isEmpty(sections));
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you are right, I refactored it here #104338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-7.13.0 apm:test-plan-done Pull request that was successfully tested during the test plan auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Service overview: Instances table metadata foldout
7 participants