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

[Unified Observability] Add Page load distribution chart to overview page #132258

Merged
merged 19 commits into from
May 23, 2022
Merged

[Unified Observability] Add Page load distribution chart to overview page #132258

merged 19 commits into from
May 23, 2022

Conversation

estermv
Copy link
Contributor

@estermv estermv commented May 16, 2022 •
edited
Loading

Closes #129472

It adds a page load distribution chart inside the UX section using the exploratory view embeddable.
Screenshot 2022-05-20 at 17 23 08

NOTE FOR REVIEWERS: the loading of the chart is not ideal, but since this visualization doesn't appear in the viewport when the user loads the page I think we can do it in further iterations.

@estermv
Copy link
Contributor Author

estermv commented May 18, 2022

@elasticmachine merge upstream

@estermv estermv requested a review from formgeist May 20, 2022 15:27
@estermv
Copy link
Contributor Author

estermv commented May 20, 2022

@elasticmachine merge upstream

formgeist
formgeist reviewed May 23, 2022
View reviewed changes
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.

@estermv Looks good having the chart there. I've added some small design nits to the review.

x-pack/plugins/observability/public/components/app/section/ux/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/components/app/section/ux/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/components/app/section/ux/index.tsx
const PAGE_LOAD_DISTRIBUTION_TITLE = i18n.translate(
'xpack.observability.overview.ux.pageLoadDistribution.title',
{
defaultMessage: 'Page load distribution',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Page load distribution',
defaultMessage: 'Page load distribution: Top 5 services',

Not sure if we're scoping it to the top 5 services instead of all services available? If so, we can add that to the title of the chart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I totally forgot about it 😅. I've been searching for how to do it in the Exploratory view but I didn't know how, so maybe we can't do that.

@shahzad31 is it possible to show the top 5 services only in the exploratory view?

Copy link
Contributor

Choose a reason for hiding this comment

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

@estermv right now, it's hardcode, so we will have to make it configurable, if we want that.
https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/components/shared/exploratory_view/configurations/lens_attributes.ts#L174

Copy link
Contributor

Choose a reason for hiding this comment

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

i will suggest creating an enhancement request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@formgeist I opened an issue to make it configurable #132720. Since the number of services is already limited and the chart won't break, are you fine with merging this as it is?

estermv and others added 2 commits May 23, 2022 10:49
@estermv
remove memoization
d34bf22
@estermv @formgeist
Update chart height
fcdc0a3 
Co-authored-by: Casper Hübertz <casper@formgeist.com>
@estermv
Copy link
Contributor Author

estermv commented May 23, 2022

@elasticmachine merge upstream

@estermv estermv marked this pull request as ready for review May 23, 2022 09:05
@estermv estermv requested review from a team as code owners May 23, 2022 09:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/unified-observability (Team:Unified observability)

@estermv estermv changed the title Add Page load distribution chart to overview page [Unified Observability] Add Page load distribution chart to overview page May 23, 2022
mgiota
mgiota approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

AO changes LGTM!

afgomez
afgomez reviewed May 23, 2022
View reviewed changes
Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

I don't have uptime data locally so I cannot test, but the code in general looks good.

I left a comment regarding how we pass the Kibana services to the exploratory view embeddable but that can be done in a different PR if necessary.

x-pack/plugins/observability/public/application/index.tsx Show resolved Hide resolved
x-pack/plugins/observability/public/components/app/section/ux/index.tsx
'service.name': ['ALL_VALUES'],
},
breakdown: 'service.name',
dataType: 'ux' as AppDataType,
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for me without the as AppDataType. Maybe there was some other error before?

Suggested change
dataType: 'ux' as AppDataType,
dataType: 'ux',

Copy link
Contributor

Choose a reason for hiding this comment

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

AppDataType probably should an enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it in a different PR to make it easier to review.

shahzad31
shahzad31 approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

@estermv estermv mentioned this pull request May 23, 2022
Closed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
observability 453.7KB 454.1KB +502.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 92.2KB 92.3KB +132.0B

History

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

@estermv estermv merged commit 3c648df into elastic:main May 23, 2022
@estermv estermv deleted the 129472-page-load-distribution-overview-page branch May 23, 2022 15:58
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

@shahzad31 shahzad31 shahzad31 approved these changes

@afgomez afgomez afgomez approved these changes

@mgiota mgiota mgiota approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Unified observability v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Unified Observability] Overview page: Page load distribution
8 participants
@estermv @elasticmachine @kibana-ci @afgomez @mgiota @shahzad31 @formgeist @kibanamachine