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

[LogsUI] Add analysis results screen #43471

Merged
merged 54 commits into from
Aug 22, 2019

Conversation

Kerry350
Copy link
Contributor

Summary

This ticket closes #42771, it adds a results screen, which for now contains the results for the log rate analysis job.

Screenshot 2019-08-16 17 49 56

Testing notes

  • On our shared cluster there is an existing and running job / datafeed named kibana-logs-ui-default-default-log-entry-rate, this PR can be tested against that (providing you're on the default space, using the default source configuration) whilst [Logs UI] Create screen to set up analysis ML jobs  #43413 is in progress. You may also setup your own jobs / datafeed by hitting the module setup endpoint as in [Logs UI] Create ML module for log analysis #42872 to test this against, but for ease using the preexisting data is easiest.

  • The anomaly explorer of the ML UI itself (ml#/explorer) is helpful to go see where anomalies exist to zone in on them in our analysis results. We don't actually have a great deal of anomalies in our cluster (usually a good thing 😅) but they are there.

Limitations

There are a couple of things to note regarding chart rendering, which is facilitated by elastic-charts.

  • Currently there's no way to have different entries in the tooltip for y and y0 values when using a band area series. This means we have Expected: <value> twice, rather than Expected: <minValue> - <maxValue>. This is coming in the future.

E.g.

Screenshot 2019-08-16 17 49 26

  • Currently there isn't a way to style specific data points of a series with a different colour (i.e. our anomalous data points in red). I've worked around this (as recommended) by rendering a second line series with just anomalous data points, and the line hidden. It does mean this entry ends up in the tooltip, though.

Screenshot 2019-08-16 18 08 44

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

weltenwort and others added 30 commits August 8, 2019 14:37
@Kerry350
Copy link
Contributor Author

Just a note to say I'm investigating the React duplicate keys error. This wasn't happening during development, so I thought it was caused by the elastic-charts version bump within Kibana as I noticed it after merging master, but if I downgrade the version the error is still there 🤔

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350
Copy link
Contributor Author

Hmm, okay, it does look like something has changed in elastic-charts which is causing the error. Looking at #43516 we bumped from 8.1.6 to 9.1.1, earlier (where I'd commented downgrading had the same issue) I'd just changed the version number in package.json to 8.1.6 but because of the ^ syntax this actually pulled in 8.1.8.

9.1.1 has the duplicate keys problem
8.1.8 has the same duplicate keys problem
8.1.6 (the version I'd developed against) doesn't have the duplicate keys problem

The PR mentions colorAccessors, but I wasn't using this prop.

Error details (standard React duplicate key stuff):

Warning: Encountered two children with the same key, `specId:{modelBounds},colors:{}`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.

Version 8.1.6:

Screenshot 2019-08-19 12 22 03

Version 8.1.8 / 9.1.1:

Screenshot 2019-08-19 12 29 53

It's entirely possible I've used something incorrectly, but I'm not using any duplicate prop IDs anywhere. I'll chase this up with the charts peeps. PR can tentatively be reviewed by forcing version 8.1.6.

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Looking at this today. Will see if anything jumps out at me re: the charts duplicate keys stuff.

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Looking great! Obviously we need to wait for the elastic-charts fix so that the tooltip isn't broken, but I've added a few thoughts in the meantime.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350
Copy link
Contributor Author

@jasonrhodes Just pushed the changes for switching between chart / table view as discussed on Slack yesterday.

switcher

With the button placement wasn't sure whether under the title looked best, or inline and over to the right.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jasonrhodes
Copy link
Member

With the button placement wasn't sure whether under the title looked best, or inline and over to the right.

I think it's fine where it is for now. Functionally looks fantastic, thanks for knocking that out so fast!

@Kerry350
Copy link
Contributor Author

@jasonrhodes I'm trying to herd together the last remaining bits we have across the two PRs we have open. I've just pushed adding a Beta badge to this PR, although the commit doesn't seem to be showing up here yet due to GitHub's degraded service status on PRs. There's a bit of icky styling I had to add to get everything to line up properly between links, tabs, and badges, but as it's temporary and will be ripped out I think we can live with it.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350 Kerry350 merged commit 7602710 into elastic:master Aug 22, 2019
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Aug 24, 2019
* Add empty analysis tab

* Add ml capabilities check

* Add job status checking functionality

* Add a loading page for the job status check

* Change types / change method for deriving space ID / change setup requirement filtering check

* Use new structure

* Add a loading page

* Initial timeRange URL state hookup

* Hook up params to data fetching

* Fleshing out EUI structure

* Change tab syntax

* i18n translate message prop

* Fix import

* Add structural visual components

* Split section in to independent component

* Real loading and no data states

* Add initial chart rendering (WIP)

* Tick formatting for x axis

* Add series styling, tickFormatter etc

* Base bucketDuration on time range for a sensible number of data points (naieve version)

* Add auto refresh

* Adjust bucketDuration algorithm

* Add some dark theme support

* Call the functions

* Extract chart helpers

* Amend io-ts types

* i18n translations

* Add types for graph data

* Allow ability to toggle model bounds

* Add anomaly series

* Format date correctly

* Add anomalies detected text

* Simplify syntax

* Update title

* Render panel within a page

* Add ability to switch between chart and table view

* Fix typechecking errors

* Add a Beta badge to the analysis tab
Kerry350 added a commit that referenced this pull request Aug 25, 2019
* Add empty analysis tab

* Add ml capabilities check

* Add job status checking functionality

* Add a loading page for the job status check

* Change types / change method for deriving space ID / change setup requirement filtering check

* Use new structure

* Add a loading page

* Initial timeRange URL state hookup

* Hook up params to data fetching

* Fleshing out EUI structure

* Change tab syntax

* i18n translate message prop

* Fix import

* Add structural visual components

* Split section in to independent component

* Real loading and no data states

* Add initial chart rendering (WIP)

* Tick formatting for x axis

* Add series styling, tickFormatter etc

* Base bucketDuration on time range for a sensible number of data points (naieve version)

* Add auto refresh

* Adjust bucketDuration algorithm

* Add some dark theme support

* Call the functions

* Extract chart helpers

* Amend io-ts types

* i18n translations

* Add types for graph data

* Allow ability to toggle model bounds

* Add anomaly series

* Format date correctly

* Add anomalies detected text

* Simplify syntax

* Update title

* Render panel within a page

* Add ability to switch between chart and table view

* Fix typechecking errors

* Add a Beta badge to the analysis tab
@weltenwort weltenwort added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Implement the Log Rate portion of the results screen
5 participants