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

[Exploratory View] Multi Series View #103855

Merged
merged 50 commits into from Jul 22, 2021
Merged

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jun 30, 2021

Summary

Some notes regarding implementation decisions

  1. First series is a main series
  2. First series can't be deleted if there are more than one series
  3. User can change data types
  4. While adding series, we will not by default select data type or report metric

Chart/Series Resize panels:

User can hide chart in configure mode and can hide series list in preview mode.

User can resize both panels in both mode using an anchor.

Date time picker/comparison

  1. In case of KPI over time, user can select any range in the main series in Subsequent series user can only select the interval defined in main series, like if user select 24 hours period, user can only select any time period in subsequent series, but it's length should always remain 24 hours.

  2. In case of distribution there are no constraints, user can select any time period/length in any series

E2e/Functional tests:

I have added few basic functional tests, but we may need to expand coverage

Known issues:

  • Not being able to add space into Text box for name
  • Design workflow and layout is a work in progress and will probably need few iterations

image

@shahzad31 shahzad31 changed the title Improve styling [Exploratory View} Multi Series View Jul 1, 2021
@shahzad31 shahzad31 self-assigned this Jul 9, 2021
@shahzad31 shahzad31 added release_note:enhancement v7.15.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jul 9, 2021
@shahzad31 shahzad31 marked this pull request as ready for review July 9, 2021 10:04
@shahzad31 shahzad31 requested review from a team as code owners July 9, 2021 10:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic changed the title [Exploratory View} Multi Series View [Exploratory View] Multi Series View Jul 9, 2021
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jul 9, 2021
@elasticmachine
Copy link
Contributor

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

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.

Great work!

*
* This function accepts two strings start and end time. I the start value is now then it uses the end value to parse.
*/
export function parseTimeParts(start: string, end: string): QuickSelect | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there tests for these utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are actually copied from EUI, i have asked in EUI channel, if these can be reused
https://github.com/elastic/eui/blob/master/src/components/date_picker/super_date_picker/relative_utils.ts


return (
<div ref={seriesBuilderRef}>
<EuiTabs size="s">{renderTabs()}</EuiTabs>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use EuiTabbedContent for this instead of managing which content appears yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EuiTabbedContent doesn't allow managing state from outside, which is required here. this is why i had to opt out of it,

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

getUrl() changes in @kbn/test LGTM

time: { from: rangeFrom, to: rangeTo },
reportDefinitions: {
[SERVICE_NAME]: [serviceName],
...(environment ? getEnvironmentDefinition(environment) : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this was simpler when all of the conditional logic was in the function. Now it's split between here and the body of the function.

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 agree. But typescript wasn't happy with that. Apparently switch/case isn't friendly with ts.

Case undefined: return {} wasn't resolving with ts.

it was logging warning that service.enivornment can't be assigned environemnt undefined.

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 also removed as SeriesUrl

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke
Copy link
Contributor

dominiqueclarke commented Jul 20, 2021

Here are a few issues I am seeing. It is unclear if these are related to multi series or if they are existing, as I haven't tested each one of these against master.

  1. Content for this popover could be improved

Screen Shot 2021-07-20 at 2 28 40 PM

  1. When filtering by a value that is also a breakdown, filtered out values still appear in the break down. In this example, it is the monitor name. Although we only have selected three monitor names, all of the monitor names appear in the breakdown. This is an especially bad experience when there are lots of values, causing the color options to be very similar.

Screen Shot 2021-07-20 at 2 19 57 PM

Screen Shot 2021-07-20 at 2 19 10 PM

  1. When there is a lot of filters, they can press up against the Operation field
    image

  2. Y axis labels appear to be incorrect. In this example, I entered exploratory view through the Uptime monitor page entry. Since starting from that entry, all of my y axis labels have remained "All monitors response duration". This is happening for every report type EDIT: This appears to be the name of the series, but can lead to a lot of confusion. It appears that the series name can also be used as the x axis label in some scenarios, both of which I found confusing.
    image

  3. It is not possible to change select no breakdown for the core web vitals report. In this example, I'm trying to look at vitals for just one application

Screen Shot 2021-07-20 at 3 14 16 PM

  1. It is possible for report configuration to be cached causing errors. In this example, I have switched from report type KPI over time. To report type Core Web Vitals, but the wrong report configuration is listed. Notice how data type is missing. When I select data type, the configuration updates and I'm presented with the right experience.

chrome-capture (16)

  1. Edge case: Select metrics and other "select x" fields are technically selectable, which can cause issues when you accidentally select them.
    chrome-capture (15)

@liciavale
Copy link
Contributor

liciavale commented Jul 20, 2021

@justinkambic @dominiqueclarke I have a list of issues/bugs in terms of usability that I found, I'm attaching them here.

  1. When the users add a new series, the accordion is missing, same for colors picker and chart type. How this should work is that when a new series has been added the first one(s) should automatically collapse.

Screenshot 2021-07-20 at 16 10 25

@liciavale
Copy link
Contributor

liciavale commented Jul 20, 2021

  1. The filters should work in the same way as the user experience app, instead of wrapping up, they should continue in a single line, which will allow saving some space. Screenshot 2021-07-20 at 20 08 08

Screenshot 2021-07-20 at 20 22 56

@liciavale
Copy link
Contributor

  1. I'm not sure if the selected value, in this case, breakdown by, should be in bold.

Screenshot 2021-07-20 at 20 31 04

@dominiqueclarke
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@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
observability 240 260 +20

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 215 219 +4

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.3MB 4.3MB +327.0B
observability 460.5KB 490.3KB +29.9KB
uptime 947.1KB 948.2KB +1.1KB
total +31.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
observability 9 10 +1

Page load bundle

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

id before after diff
apm 40.6KB 40.7KB +136.0B
observability 58.4KB 59.6KB +1.2KB
total +1.4KB
Unknown metric groups

API count

id before after diff
observability 215 219 +4

History

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

cc @shahzad31

@shahzad31 shahzad31 added the auto-backport Deprecated: Automatically backport this PR after it's merged label Jul 22, 2021
@shahzad31 shahzad31 merged commit 48e6195 into elastic:master Jul 22, 2021
@shahzad31 shahzad31 deleted the improve-styling branch July 22, 2021 08:14
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 22, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
@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 Jul 22, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>

Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged release_note:enhancement Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants