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]Additional metrics for kpi over time #96532

Merged
merged 110 commits into from
Apr 12, 2021

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Apr 8, 2021

Summary

PR in a series of features to cover https://github.com/elastic/uptime-dev/issues/46

Added additional metric for RUM Kpi over time

Added Page view, backend time, First contentful pain, Backend time, Total blocking time, Largest contentful paint

Also improved the chart type select component by making it a select, renamed few other variables and components.

Testing

When you select Rum data type and KPI over time report type,

you should be able to plot all those additional metrics on the chart.

image

@shahzad31 shahzad31 added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Apr 8, 2021
@shahzad31 shahzad31 self-assigned this Apr 8, 2021
@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0 labels Apr 8, 2021
@shahzad31 shahzad31 marked this pull request as ready for review April 8, 2021 12:56
@shahzad31 shahzad31 requested a review from a team April 8, 2021 12:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@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
observability 413.8KB 416.8KB +2.9KB

Page load bundle

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

id before after diff
observability 35.1KB 35.1KB +42.0B

History

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

cc @shahzad31

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.

Looks great!

One thing I noticed is that if you select the "placeholder" option, you encounter this error.

This isn't a great experience. Since we already initialize the chart with a starting metric, do we need to include the placeholder as an option? Since the fields within the Definition section all have different purposes, perhaps we can label them some other way, like with EuiFormRow.

Exploratory-view-Observability-Elastic

If we don't make use of EuiFormRow, we'll need to include aria-labels for the selects, as it doesn't appear as if the input fields are currently labeled in the html.

Also, sometimes the buttons spill out of their columns. This may not be as big of an issue, as I was viewing the feature at non-standard breakpoints.

Screen Shot 2021-04-12 at 9 12 10 AM


if (fieldType === 'date') {
return this.getDateHistogramColumn(fieldName);
}
if (fieldType === 'number') {
return this.getNumberColumn(fieldName);
if (columnType === 'operation' || operationType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is starting to get nested pretty deep. Any alternatives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's indeed, i am hoping Lens team can expose these small utils to make embeddable component life easy. May have to contribute myself there, especially these build column utils.


describe.skip('SeriesChartTypes', function () {
describe.skip('SeriesChartTypesSelect', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these skipped currently?

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 had skipped these as well because of getByRole slowness, i wasn't earlier sure why it's happening so make CI happy, i did that, will unskip in a follow up.


const { data = [], loading } = useFetcher(() => lens.getXyVisTypes(), [lens]);

let vizTypes = data ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure you need this with the above default assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++, will remove this.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Exposing FieldBasedIndexPatternColumn sounds good to me, code LGTM.

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.

LGTM

@shahzad31
Copy link
Contributor Author

Looks great!

One thing I noticed is that if you select the "placeholder" option, you encounter this error.

This isn't a great experience. Since we already initialize the chart with a starting metric, do we need to include the placeholder as an option? Since the fields within the Definition section all have different purposes, perhaps we can label them some other way, like with EuiFormRow.

@dominiqueclarke i will address these in a follow up #96782, that should be the last PR as a follow up before FF, at that point we can decide the state of things and see if we want to link actual charts to this or keep it hidden behind URL.

@shahzad31 shahzad31 merged commit a2c47ef into elastic:master Apr 12, 2021
@shahzad31 shahzad31 deleted the additional-metrics-for-kpi branch April 12, 2021 13:53
@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 12, 2021
phillipb added a commit to phillipb/kibana that referenced this pull request Apr 12, 2021
…to-metrics-tab

* 'master' of github.com:elastic/kibana: (44 commits)
  [Exploratory View]Additional metrics for kpi over time (elastic#96532)
  [Fleet] UI changes on hosted policy detail view (elastic#96337)
  Stacked line charts incorrectly shows one term as 100% (elastic#96203)
  [Fleet] Create enrollment API keys as current user (elastic#96464)
  [Lens] Make table and metric show on top Chart switcher (elastic#96601)
  skip flaky suite (elastic#96691)
  [Lens] Hide "Show more errors" once expanded (elastic#96605)
  [Discover] Unskip histogram hiding test (elastic#95759)
  skip flyout test, add linked issue elastic#96708
  skip copy_to_space_flyout_internal.test.tsx elastic#96708
  fix config validation (elastic#96502)
  Document telemetry fields for stack security features (elastic#96638)
  [Partial Results] Move inspector adapter integration into search source (elastic#96241)
  [RAC] Rule registry plugin (elastic#95903)
  [APM] Run precommit tasks sequentially (elastic#96551)
  [Maps] fix Kibana does not recognize a valid geo_shape index when attempting to create a Tracking Containment alert (elastic#96633)
  [Security Solution] [Cases] Small UI bugfixes (elastic#96511)
  [Actions UI] Changed PagerDuty action form UI to fill payload fields according to the API docs for Resolve and Acknowledge events. (elastic#96363)
  App Search: Result Component Updates (elastic#96184)
  [Alerting] Preconfigured alert history index connector (elastic#94909)
  ...
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 12, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@shahzad31 shahzad31 restored the additional-metrics-for-kpi branch April 14, 2021 15:18
@shahzad31 shahzad31 deleted the additional-metrics-for-kpi branch April 15, 2021 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants