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

[TSVB] Change the default mode from last value to entire timerange #93608

Merged
merged 33 commits into from
Mar 29, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Mar 4, 2021

Closes: #91506

Summary

Sets default value for "Data timerange mode" as entire time range. Also adds telemetry for count TSVB visualizations which use last value mode.

Telemetry has the following structure:
image

Dev Docs

Uses entire time range as default value for "Data timerange mode" in TSVB (top N, markdown, gauge, metric, table).

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa VladLasitsa self-assigned this Mar 9, 2021
@VladLasitsa VladLasitsa added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0 labels Mar 9, 2021
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

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.

These changes looks good to me. I have only one question about description. For me it looks like a breaking change cause all new visualizations will use a different mode. Maybe we should somehow to highlight that by adding Release Nodes / Dev Docs. @stratoula any ideas?

@stratoula
Copy link
Contributor

I agree let's add a brief Dev Docs section to notify the users about this change.

@VladLasitsa VladLasitsa marked this pull request as ready for review March 11, 2021 13:36
@VladLasitsa VladLasitsa requested review from a team as code owners March 11, 2021 13:36
@VladLasitsa VladLasitsa requested a review from a team March 11, 2021 13:36
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Thank you for adding the descriptions! LGTM!

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@@ -86,7 +89,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
const labelString = await PageObjects.visualBuilder.getGaugeLabel();
expect(labelString).to.be('Count');
const gaugeCount = await PageObjects.visualBuilder.getGaugeCount();
expect(gaugeCount).to.be('156');
expect(gaugeCount).to.be('13,830');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this needs to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it because with that changes by default we use EntireTimeRange instead of LastValue. 13,830 was calculated for the whole timeperiod

Copy link
Contributor

@stratoula stratoula Mar 23, 2021

Choose a reason for hiding this comment

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

Oh, I see. I am fine then. We could also set the mode to Last Value and not change the test but I have no strong objection.

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.

The usage collector works fine now. What doesn't is the following scenario:
I have an already existing timeseries TSVB viz. I choose to edit it, if I navigate to the metric Tab for example, I see that the mode is set to Last Value. It should default to entire timerange

Moreover I suggest to add a functional test to check if the entire timerange is selected for an new TSVB viz.

Finally, I see that we changed the value of a functional test. Why? I see above that you set the value to Last value, so this shouldn't change, right?

@VladLasitsa
Copy link
Contributor Author

The usage collector works fine now. What doesn't is the following scenario:
I have an already existing timeseries TSVB viz. I choose to edit it, if I navigate to the metric Tab for example, I see that the mode is set to Last Value. It should default to entire timerange

Fixed it.

Moreover I suggest to add a functional test to check if the entire timerange is selected for an new TSVB viz.

Added functional test for it.

Finally, I see that we changed the value of a functional test. Why? I see above that you set the value to Last value, so this shouldn't change, right?

I have done this so that we cover both cases when we have 'Last value' and 'Entire time range'.

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.

Code LGTM, it works as expected now. Thank you @VladLasitsa, feel free to merge it when it is green

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@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
visTypeTimeseries 1.6MB 1.6MB +484.0B

History

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

cc @VladLasitsa

@stratoula
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

@VladLasitsa VladLasitsa merged commit 0e40b94 into elastic:master Mar 29, 2021
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Mar 29, 2021
…lastic#93608)

* Make 'enter time range' value as default and add telemetry for 'last value' mode

* Fix telemetry schema

* Fix test

* Add possibility count timeseries created from dashboard

* Fix remark

* Fix remark

* Fix problem with time_range_mode

* Fix tests

* Fix tests

* Fix tests for markdown and table

* exclude TSVB which have type as timeseries

* Add description for field in schema in telemetry

* Fix telemetry schema

* Fix some remarks

* Added check for hits

* fix CI

* fix CI

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
VladLasitsa added a commit that referenced this pull request Mar 30, 2021
…93608) (#95662)

* Make 'enter time range' value as default and add telemetry for 'last value' mode

* Fix telemetry schema

* Fix test

* Add possibility count timeseries created from dashboard

* Fix remark

* Fix remark

* Fix problem with time_range_mode

* Fix tests

* Fix tests

* Fix tests for markdown and table

* exclude TSVB which have type as timeseries

* Add description for field in schema in telemetry

* Fix telemetry schema

* Fix some remarks

* Added check for hits

* fix CI

* fix CI

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
@timroes timroes added the Feature:TSVB TSVB (Time Series Visual Builder) label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Change the default mode from last value to entire timerange
8 participants