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

Replace flot with elastic-chart in Timelion #81565

Merged
merged 75 commits into from Aug 2, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Oct 23, 2020

Closes: #79984, #74888, #69122, #10810

Summary

Migrated timelion from jquery-flot to elastic-charts

Problems which I found described in the following issues: elastic/elastic-charts#878 and elastic/elastic-charts#893

Here points that we should do:

Below you can find some screenshots to compare timelion with jquery-flot and elastic-charts:

  1. .es(index=metricbeat-*,
    timefield='@timestamp',
    metric='avg:system.cpu.user.pct')

timelion with jquery-flot (as now we have in master):
image

timelion with elastic-charts (this PR):
image

  1. .es(offset=-1h, index=metricbeat-*, timefield='@timestamp', metric='avg:system.cpu.user.pct').label('last hour').lines(fill=1,width=0.5).color(gray), .es(index=metricbeat-*, timefield='@timestamp', metric='avg:system.cpu.user.pct').label('current hour').title('CPU usage over time').color(#1E90FF)

timelion with jquery-flot (as now we have in master):
image

timelion with elastic-charts (this PR):
image

  1. .es(index=metricbeat*, timefield=@timestamp, metric=max:system.network.in.bytes).derivative().divide(1048576), .es(index=metricbeat*, timefield=@timestamp, metric=max:system.network.out.bytes).derivative().multiply(-1).divide(1048576)

timelion with jquery-flot (as now we have in master):
image

timelion with elastic-charts (this PR):
image

Checklist

Delete any items that are not applicable to this PR.

  • Documentation was added for features that require explanation or tutorials

For maintainers

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

Comment on lines 195 to 220
{chart[0]._global && chart[0]._global.yaxes ? (
chart[0]._global.yaxes.map((axis: IAxis, index: number) => {
return (
<Axis
key={index}
id={axis.position + axis.axisLabel}
title={axis.axisLabel}
position={axis.position}
tickFormat={axis.tickFormatter}
gridLine={{
stroke: 'rgba(125,125,125,0.3)',
visible: true,
}}
domain={axis.domain as YDomainRange}
/>
);
})
) : (
<Axis
id="left"
position={Position.Left}
gridLine={{
stroke: 'rgba(125,125,125,0.3)',
visible: true,
}}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

it's too difficult to read. Maybe we should name it somehow or move into separate component?

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 don't think it's needed. Create a separate document just for rendering Axis it's strange for me. Maybe do you want that I create s separate function for rendering Axis?

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanx Vlad for this PR. Some comments from my side:

  • It seems that Timelion app is not working anymore

Screenshot 2020-11-06 at 12 26 36 PM

- The bar's width scales differently. And the red line that follows user's mouse is huge in some cases. Here I have set the bard width on 1 but it is definitely not 1. Moreover the red line has the same width as the bars. It shouldn't

Screenshot 2020-11-06 at 12 40 33 PM

- .multiply(-1) creates a weird chart

Screenshot 2020-11-06 at 1 02 57 PM

While in 7.10

Screenshot 2020-11-06 at 1 03 17 PM

@VladLasitsa
Copy link
Contributor Author

So this works pretty well! I have added some comments about the new setting.
I can only see a difference between the two implementations with this expression:
.static(50).points(symbol=cross, radius=2)

On the old implementation:
image

On the new:
image

Two comments here:

  • The static value appears twice in the legend
  • In the new implementation, the y axis has values from 0 to 50, while on the old implementation the y axis starts from 49.9925. As a result the static value is being displayed in the center.

@stratoula, I checked TSVB and Lens with static value and get the following:
Lens:
MicrosoftTeams-image

TSVB:

MicrosoftTeams-image (1)

As you can see elastic-charts works with static value everywhere like in timelion now. I understand that is not like before but I think we can it leave this behavior so that we will be consistent. Also about legend, elastic-chart show in legend the last value of graphic if we don't have cursor. As we have name (50) and value (50) we see twice it. The similar behavior you can see in TSVB:

image

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Ok I see, thanx @VladLasitsa I think we can merge it as it is.
LGTM, I have tested it multiple times and taking under consideration that we also have the fallback on the old implementation I think it is ready to be merged 🎉

@stratoula
Copy link
Contributor

@elastic/kibana-telemetry can you please take a look? 🙇‍♀️

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

telemetry changes LGTM. mikhail already approved this so we are not blocking the PR. seems it needs a review from the kibana-design team

@stratoula
Copy link
Contributor

Correct! @elastic/kibana-design we need your review ❤️

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally

@alexwizp
Copy link
Contributor

Correct! @elastic/kibana-design we need your review ❤️

@elastic/kibana-design please have a look, we really want to merge it 🙏

@ryankeairns ryankeairns requested review from ryankeairns and removed request for wylieconlon July 30, 2021 17:27
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM. It appears Michael's suggestions were addressed as well.

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@VladLasitsa VladLasitsa added the auto-backport Deprecated: Automatically backport this PR after it's merged label Aug 2, 2021
@VladLasitsa VladLasitsa enabled auto-merge (squash) August 2, 2021 08:41
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimelion 48 66 +18

Async chunks

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

id before after diff
timelion 197.4KB 197.4KB +15.0B
visTypeTimelion 69.1KB 89.3KB +20.2KB
total +20.2KB

Page load bundle

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

id before after diff
core 421.5KB 421.5KB +78.0B
visTypeTimelion 24.4KB 24.6KB +262.0B
total +340.0B
Unknown metric groups

async chunk count

id before after diff
visTypeTimelion 3 4 +1

History

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

cc @VladLasitsa

@VladLasitsa VladLasitsa merged commit 8088565 into elastic:master Aug 2, 2021
Legacy visualizations automation moved this from In progress to Done Aug 2, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 2, 2021
* First draft migrate timelion to elastic-charts

* Some refactoring. Added brush event.

* Added title. Some refactoring

* Fixed some type problems. Added logic for yaxes function

* Fixed some types, added missing functionality for yaxes

* Fixed some types, added missing functionality for stack property

* Fixed unit test

* Removed unneeded code

* Some refactoring

* Some refactoring

* Fixed some remarks.

* Fixed some styles

* Added themes. Removed unneeded styles in BarSeries

* removed unneeded code.

* Fixed some comments

* Fixed vertical cursor across Timelion visualizations of a dashboad

* Fix some problems with styles

* Use RxJS instead of jQuery

* Remove unneeded code

* Fixed some problems

* Fixed unit test

* Fix CI

* Fix eslint

* Fix some gaps

* Fix legend columns

* Some fixes

* add 2 versions of Timeline app

* fix CI

* cleanup code

* fix CI

* fix legend position

* fix some cases

* fix some cases

* remove extra casting

* cleanup code

* fix issue with static

* fix header formatter

* fix points

* fix ts error

* Fix yaxis behavior

* Fix some case with yaxis

* Add deprecation message and update asciidoc

* Fix title

* some text improvements

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 4, 2021
kibanamachine added a commit that referenced this pull request Aug 4, 2021
* First draft migrate timelion to elastic-charts

* Some refactoring. Added brush event.

* Added title. Some refactoring

* Fixed some type problems. Added logic for yaxes function

* Fixed some types, added missing functionality for yaxes

* Fixed some types, added missing functionality for stack property

* Fixed unit test

* Removed unneeded code

* Some refactoring

* Some refactoring

* Fixed some remarks.

* Fixed some styles

* Added themes. Removed unneeded styles in BarSeries

* removed unneeded code.

* Fixed some comments

* Fixed vertical cursor across Timelion visualizations of a dashboad

* Fix some problems with styles

* Use RxJS instead of jQuery

* Remove unneeded code

* Fixed some problems

* Fixed unit test

* Fix CI

* Fix eslint

* Fix some gaps

* Fix legend columns

* Some fixes

* add 2 versions of Timeline app

* fix CI

* cleanup code

* fix CI

* fix legend position

* fix some cases

* fix some cases

* remove extra casting

* cleanup code

* fix issue with static

* fix header formatter

* fix points

* fix ts error

* Fix yaxis behavior

* Fix some case with yaxis

* Add deprecation message and update asciidoc

* Fix title

* some text improvements

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

Co-authored-by: Uladzislau Lasitsa <Uladzislau_Lasitsa@epam.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 4, 2021
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
* First draft migrate timelion to elastic-charts

* Some refactoring. Added brush event.

* Added title. Some refactoring

* Fixed some type problems. Added logic for yaxes function

* Fixed some types, added missing functionality for yaxes

* Fixed some types, added missing functionality for stack property

* Fixed unit test

* Removed unneeded code

* Some refactoring

* Some refactoring

* Fixed some remarks.

* Fixed some styles

* Added themes. Removed unneeded styles in BarSeries

* removed unneeded code.

* Fixed some comments

* Fixed vertical cursor across Timelion visualizations of a dashboad

* Fix some problems with styles

* Use RxJS instead of jQuery

* Remove unneeded code

* Fixed some problems

* Fixed unit test

* Fix CI

* Fix eslint

* Fix some gaps

* Fix legend columns

* Some fixes

* add 2 versions of Timeline app

* fix CI

* cleanup code

* fix CI

* fix legend position

* fix some cases

* fix some cases

* remove extra casting

* cleanup code

* fix issue with static

* fix header formatter

* fix points

* fix ts error

* Fix yaxis behavior

* Fix some case with yaxis

* Add deprecation message and update asciidoc

* Fix title

* some text improvements

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
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 Feature:Timelion Timelion app and visualization release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.0 v8.0.0
Development

Successfully merging this pull request may close these issues.

Replace flot with elastic-chart in Timelion