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] Service overview: Introduce time-series comparison #88665

Merged
merged 14 commits into from
Jan 21, 2021
Merged

[APM] Service overview: Introduce time-series comparison #88665

merged 14 commits into from
Jan 21, 2021

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Jan 19, 2021 •
edited
Loading

Part of #81147

This PR does not handle the data fetch, it only adds the logic in the UI fields.

Comparison will be enabled by default unless the time range selected is greater than 8 days. The select box can have two options "Yesterday" or "A week ago" depending on the time range selected

When the time range is less than 1 day: "Yesterday" and "A week ago" are visible
Screenshot 2021-01-19 at 15 26 20

When the time range is greater than 1 day and less than 8 days: "A week ago" is visible and the select box is disabled.
Screenshot 2021-01-19 at 15 26 49

When the time range is greater than 8 days: "Previous period" is visible and the select box is disabled.
Screenshot 2021-01-20 at 11 17 42

@cauemarcondes cauemarcondes marked this pull request as ready for review January 19, 2021 14:57
@cauemarcondes cauemarcondes requested a review from a team as a code owner January 19, 2021 14:57
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jan 19, 2021
@elasticmachine
Copy link
Contributor

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

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@cauemarcondes
Copy link
Contributor Author

@formgeist let me know what you think about the search bar:

Small
Screenshot 2021-01-20 at 17 54 28

Medium
Screenshot 2021-01-20 at 17 54 17

Large
Screenshot 2021-01-20 at 17 54 05

XLArge
Screenshot 2021-01-20 at 17 53 33

@cauemarcondes cauemarcondes requested a review from a team as a code owner January 20, 2021 17:14
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jan 20, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

shahzad31
shahzad31 approved these changes Jan 20, 2021
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.

UX LGTM

sorenlouv
sorenlouv reviewed Jan 20, 2021
View reviewed changes
x-pack/plugins/apm/public/context/url_params_context/types.ts
@@ -29,4 +29,6 @@ export type IUrlParams = {
searchTerm?: string;
percentile?: number;
latencyAggregationType?: string;
comparisonEnabled?: boolean;
comparisonType?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we could stop adding more urlparams but not sure there are any good alternatives until we tackle #51963...

sorenlouv
sorenlouv approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm

@formgeist
Copy link
Contributor

@formgeist let me know what you think about the search bar:

The breakpoints look good - only thing I noticed is that there's no margin to the content when the comparison and date picker breaks to another row. If we can add some to separate the content from the search bar section, that'd be 👍

Screenshot 2021-01-20 at 21 23 01

@cauemarcondes
Copy link
Contributor Author

The breakpoints look good - only thing I noticed is that there's no margin to the content when the comparison and date picker breaks to another row. If we can add some to separate the content from the search bar section, that'd be

@formgeist

Screenshot 2021-01-21 at 09 39 23

Screenshot 2021-01-21 at 09 39 30

Screenshot 2021-01-21 at 09 39 13

@formgeist
Copy link
Contributor

The breakpoints look good - only thing I noticed is that there's no margin to the content when the comparison and date picker breaks to another row. If we can add some to separate the content from the search bar section, that'd be

@formgeist

👍

@kibanamachine
Copy link
Contributor

💚 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
apm 5.5MB 5.5MB +4.5KB

History

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

@cauemarcondes cauemarcondes merged commit d997f20 into elastic:master Jan 21, 2021
@cauemarcondes cauemarcondes deleted the apm-time-comparison branch January 21, 2021 11:13
@cauemarcondes cauemarcondes mentioned this pull request Jan 21, 2021
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Jan 21, 2021
@cauemarcondes @kibanamachine
[APM] Service overview: Introduce time-series comparison (elastic#88665)
03331c6 
* adding comparision select option

* adding time comparison field on some pages

* removing unused files

* fixing unit test

* adding unit tests

* enabling comparison for more than 8 days

* removing tooltip

* refactoring search bar

* moving useBreakPoint to common hooks folder, removing useShouldUSeMobileLayout hook

* addressing PR comments

* addressing PR comments

* addressing PR comments

* addressing PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
cauemarcondes added a commit that referenced this pull request Jan 21, 2021
@cauemarcondes @kibanamachine
[APM] Service overview: Introduce time-series comparison (#88665) (#8…
edfae05 
…8948)

* adding comparision select option

* adding time comparison field on some pages

* removing unused files

* fixing unit test

* adding unit tests

* enabling comparison for more than 8 days

* removing tooltip

* refactoring search bar

* moving useBreakPoint to common hooks folder, removing useShouldUSeMobileLayout hook

* addressing PR comments

* addressing PR comments

* addressing PR comments

* addressing PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@cauemarcondes cauemarcondes mentioned this pull request Apr 21, 2021
Closed
@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
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

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees

@ogupte ogupte

Labels
apm:comparison apm:test-plan-7.13.0 apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cauemarcondes @elasticmachine @formgeist @kibanamachine @sorenlouv @shahzad31 @ogupte