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.

Sign up for GitHub

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

Jump to bottom

[APM] Updated header icons #84760

Merged
merged 24 commits into from
Dec 16, 2020
Merged

[APM] Updated header icons #84760

merged 24 commits into from
Dec 16, 2020

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Dec 2, 2020 •
edited
Loading

closes #81717

Screenshot 2020-12-02 at 17 40 49

Screenshot 2020-12-02 at 17 40 56

Screenshot 2020-12-02 at 17 41 14

Screenshot 2020-12-02 at 17 41 38

Screenshot 2020-12-02 at 17 41 44

When should we should the container icon?
the only two possible container orchestration will be Docker or Kubernetes

  • To define if the orchestration is Kubernetes : I’ll check if there’s any kubernetes information in the document
  • To define if the orchestration is Docker : If there’s no kubernetes information and there is container in the document
  • if there’s no kubernetes neither container in the document, I won’t show Container section in the header.

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@cauemarcondes cauemarcondes force-pushed the apm-serv-overview-header-icons branch from 359bfab to 47dd1dd Compare December 10, 2020 09:58
@cauemarcondes cauemarcondes marked this pull request as ready for review December 10, 2020 13:34
@cauemarcondes cauemarcondes requested a review from a team as a code owner December 10, 2020 13:34
@formgeist
Copy link
Contributor

formgeist commented Dec 10, 2020 •
edited
Loading

@cauemarcondes Maybe I misunderstood something, but if I have multiple versions through a selected time range, I imagine those versions would show in the service version fields. And the same goes for instances.

Screenshot 2020-12-10 at 14 48 08

Screenshot 2020-12-10 at 14 47 38

@cauemarcondes
Copy link
Contributor Author

@cauemarcondes Maybe I misunderstood something, but if I have multiple versions through a selected time range, I imagine those versions would show in the service version fields. And the same goes for instances.

Screenshot 2020-12-10 at 14 48 08 Screenshot 2020-12-10 at 14 47 38

Alright, I didn't know it. How should we show it if multiple versions are returned?

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 10, 2020
@elasticmachine
Copy link
Contributor

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

@formgeist
Copy link
Contributor

@cauemarcondes We should just list them out, and since they're time-based perhaps in chronological order, so the most recent first.

Screenshot 2020-12-10 at 15 12 13

Additionally, I think you've implemented it using EuiStat but I think EuiDescriptionList (https://elastic.github.io/eui/#/display/description-list#reverse-style) actually provides possibly a better UI component for something like this, at least that's the reference I used in the design. I see now that I didn't clarify the components in the design issue, something I should do more often.

@cauemarcondes
Copy link
Contributor Author

And the same goes for instances

@formgeist Which field are you referring to when you say instances?

@dgieselaar
Copy link
Member

Have we considered doing:

  • for the immediately displayed information, only get the last document
  • for the data in the popup, only fetch it when the popup has opened

I am worried about the amount of open connections we have to ES when loading the overview page. We can't set a priority on searches and we might need more search threads than are available, leading to other searches being queued.

@cauemarcondes
Copy link
Contributor Author

Have we considered doing:

  • for the immediately displayed information, only get the last document
  • for the data in the popup, only fetch it when the popup has opened

I am worried about the amount of open connections we have to ES when loading the overview page. We can't set a priority on searches and we might need more search threads than are available, leading to other searches being queued.

I have considered that, but the first version of the API was pretty simple, now I see that I'm changing it to have more aggregated values, I'll probably do like you suggested.

@formgeist
Copy link
Contributor

@formgeist Which field are you referring to when you say instances?

I believe it's the unique number of service.node.name within the selected time range. @alex-fedotyev can you confirm that you want to show the number of service instances, not just the number of container.id's?

@cauemarcondes cauemarcondes requested a review from a team December 14, 2020 12:33
smith
smith requested changes Dec 15, 2020
View reviewed changes
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.

The comments I have are non-blocking nits.

Could we make it so when we wrap at small screen sizes the icons all stay together on one line?

image

x-pack/plugins/apm/public/components/app/service_details/service_icons/cloud_details.tsx Outdated
listItems.push({
title: i18n.translate('xpack.apm.serviceNameHeader.cloud.zone', {
defaultMessage:
'{zones, plural, =0 {Availability zone} one {Availability zone} other {Availability zones}} ',
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is happening here if there are no availability zones? It looks like it would have an "Availability zone" title with an empty ul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no availability zones this section won't show.

This prevents it:

  if (!!cloud.availabilityZones?.length) {

x-pack/plugins/apm/public/components/app/service_details/service_icons/cloud_details.tsx Outdated
listItems.push({
title: i18n.translate('xpack.apm.serviceNameHeader.cloud.machine', {
defaultMessage:
'{machineTypes, plural, =0{Machine type} one {Machine type} other {Machine types}} ',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here as with AZs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing here, if there are no machine types this section won't show.

x-pack/plugins/apm/public/components/app/service_details/service_icons/container_details.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/components/app/service_details/service_icons/icon_popover.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/components/app/service_details/service_icons/index.test.tsx Outdated Show resolved Hide resolved
x-pack/test/apm_api_integration/basic/tests/services/service_details.ts Outdated Show resolved Hide resolved
sorenlouv
sorenlouv reviewed Dec 16, 2020
View reviewed changes
x-pack/plugins/apm/public/components/app/service_details/service_icons/index.tsx
@@ -78,12 +90,9 @@ export function ServiceIcons({ serviceName }: Props) {
});
}
},
[isPopoverOpen, serviceName, start, end, uiFilters]
[selectedIconPopover, serviceName, start, end, uiFilters]
Copy link
Member

Choose a reason for hiding this comment

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

Is the request made every time the popover is opened/closed or only the first time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The request is going to be cached, so no, the details are going to be fetched once and used until the parameters are changed. Take a look:

ezgif com-gif-maker (3)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1730 1735 +5

Async chunks

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

id before after diff
apm 5.4MB 5.4MB +12.1KB

Distributable file count

id before after diff
default 47253 48017 +764

History

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

@cauemarcondes cauemarcondes merged commit 53da425 into elastic:master Dec 16, 2020
@cauemarcondes cauemarcondes deleted the apm-serv-overview-header-icons branch December 16, 2020 22:08
@cauemarcondes cauemarcondes mentioned this pull request Dec 16, 2020
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Dec 16, 2020
@cauemarcondes @kibanamachine
[APM] Updated header icons (elastic#84760)
c57b5ee 
* creating service name header

* fixing icons

* removing unused api import

* fixing some stuff

* adding API tests

* refactoring some stuff

* fixing tests

* refactoring some stuff

* fixing i18n

* reverting

* renaming

* applying min width

* addressing PR comments and adding test

* sorting service version

* changing sort type to desc

* addressing pr comments

* changing to show total and not avg

* addressing pr comments

* addressing pr comments

* addressing pr comments

Co-authored-by: Kibana Machine <[email protected]>
cauemarcondes added a commit that referenced this pull request Dec 17, 2020
@cauemarcondes @kibanamachine
[APM] Updated header icons (#84760) (#86201)
9f8b7b2 
* creating service name header

* fixing icons

* removing unused api import

* fixing some stuff

* adding API tests

* refactoring some stuff

* fixing tests

* refactoring some stuff

* fixing i18n

* reverting

* renaming

* applying min width

* addressing PR comments and adding test

* sorting service version

* changing sort type to desc

* addressing pr comments

* changing to show total and not avg

* addressing pr comments

* addressing pr comments

* addressing pr comments

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

Co-authored-by: Kibana Machine <[email protected]>
@cauemarcondes cauemarcondes mentioned this pull request Dec 17, 2020
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Dec 17, 2020
@cauemarcondes @kibanamachine
[APM] Updated header icons (elastic#84760)
8fb70b4 
* creating service name header

* fixing icons

* removing unused api import

* fixing some stuff

* adding API tests

* refactoring some stuff

* fixing tests

* refactoring some stuff

* fixing i18n

* reverting

* renaming

* applying min width

* addressing PR comments and adding test

* sorting service version

* changing sort type to desc

* addressing pr comments

* changing to show total and not avg

* addressing pr comments

* addressing pr comments

* addressing pr comments

Co-authored-by: Kibana Machine <[email protected]>
cauemarcondes added a commit that referenced this pull request Dec 18, 2020
@cauemarcondes @kibanamachine
[APM] Updated header icons (#84760) (#86310)
1d97b8a 
* creating service name header

* fixing icons

* removing unused api import

* fixing some stuff

* adding API tests

* refactoring some stuff

* fixing tests

* refactoring some stuff

* fixing i18n

* reverting

* renaming

* applying min width

* addressing PR comments and adding test

* sorting service version

* changing sort type to desc

* addressing pr comments

* changing to show total and not avg

* addressing pr comments

* addressing pr comments

* addressing pr comments

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

Co-authored-by: Kibana Machine <[email protected]>
@cauemarcondes cauemarcondes mentioned this pull request Jan 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@formgeist formgeist formgeist left review comments

@sorenlouv sorenlouv sorenlouv approved these changes

@smith smith smith approved these changes

Assignees
No one assigned
Labels
release_note:enhancement Team:APM All issues that need APM UI Team support v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Updated header icons
8 participants
@cauemarcondes @formgeist @elasticmachine @dgieselaar @alex-fedotyev @kibanamachine @smith @sorenlouv