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] Timeseries Drop last bucket set default to false #97257

Merged
merged 20 commits into from
Apr 30, 2021

Conversation

DianaDerevyankina
Copy link
Contributor

@DianaDerevyankina DianaDerevyankina commented Apr 15, 2021

Closes #95756

Summary

  • Changed the default value of Drop last bucket setting to false.

    image

  • Added a tooltip indicating there's partial data. It's displayed for the last value in case the last bucket is not dropped either for the whole visualization or for any series. (To test that last bucket should intersect the time selector)

    image

    To drop last bucket for the series: Data -> Options select "Yes" for Override Index Pattern? and then Drop last bucket

    image
    image

Checklist

For maintainers

@DianaDerevyankina DianaDerevyankina added Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Apr 15, 2021
@DianaDerevyankina DianaDerevyankina self-assigned this Apr 15, 2021
series,
yAxis,
onBrush,
xAxisFormatter,
annotations,
syncColors,
palettesService,
interval,
shouldDropLastBucket,
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, the drop bucket happens on the server side. I think the name of the variable does not reflect the essence of what you want. Please rename

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

xAxisFormatter={this.xAxisFormatter(interval)}
annotations={this.prepareAnnotations()}
syncColors={syncColors}
palettesService={palettesService}
interval={interval}
shouldDropLastBucket={Boolean(
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 user should see that message if DropLastBucket is false =)

image

? renderEndzoneTooltip(
interval,
seriesData[0][0],
seriesData[seriesData.length - 1][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

please extract logic for getting domain into a separate method

const caclulateDomainForSeries = (series) => {
  const seriesData = series[0]?.data || [];

  return {
    domainStart: seriesData[0][0],
    domainEnd: seriesData[Math.max(seriesData.length - 1, 0)][0],
  };
};

export const TimeSeries = ({
.....

Copy link
Contributor

Choose a reason for hiding this comment

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

and then

let tooltipFormatter;
if (shouldDropLastBucket) {
  const { domainStart, domainEnd } = caclulateDomainForSeries(series);

  tooltipFormatter = renderEndzoneTooltip(
    interval,
    domainStart,
    domainEnd,
    decorateFormatter(xAxisFormatter)
  );
} else {
  tooltipFormatter = decorateFormatter(xAxisFormatter);
}

@DianaDerevyankina
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor

@stratoula @dziyanadzeraviankina I think we should also update our sample visualizations.

@stratoula
Copy link
Contributor

I was going to create an issue for that 🙂

@DianaDerevyankina
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@@ -14,7 +14,7 @@ export function dropLastBucket(resp, panel, series) {
const shouldDropLastBucket = isLastValueTimerangeMode(panel, series);

if (shouldDropLastBucket) {
const seriesDropLastBucket = get(series, 'override_drop_last_bucket', 1);
const seriesDropLastBucket = get(series, 'override_drop_last_bucket', 0);
Copy link
Contributor

@alexwizp alexwizp Apr 20, 2021

Choose a reason for hiding this comment

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

I think the following code works wrong, override_drop_last_bucket should has a priority todrop_last_bucket

// wrong code
const seriesDropLastBucket = get(series, 'override_drop_last_bucket', 0);
const dropLastBucket = get(panel, 'drop_last_bucket', seriesDropLastBucket);

@dziyanadzeraviankina @stratoula think we can that in scope of that PR

Comment on lines 33 to 34
expect(domainBounds?.domainStart).toBeUndefined();
expect(domainBounds?.domainEnd).toBeUndefined();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(domainBounds?.domainStart).toBeUndefined();
expect(domainBounds?.domainEnd).toBeUndefined();
expect(domainBounds).toBeUndefined();

Copy link
Contributor

@dimaanj dimaanj 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 it locally

Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

LGTM

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

@alexwizp alexwizp marked this pull request as ready for review April 23, 2021 07:20
@alexwizp alexwizp requested a review from a team April 23, 2021 07:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

This PR introduces a bug in the tooltip. When the last bucket is kept, the tooltip only shows the current time. Steps to reproduce:

  1. Install the ecommerce data
  2. Go to the ecommerce dashboard
  3. Hover over the TSVB chart with annotations

@timroes
Copy link
Contributor

timroes commented Apr 27, 2021

@elasticmachine run elasticsearch-ci/docs

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

LGTM!

@DianaDerevyankina
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
visTypeTimeseries 501 502 +1

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.7MB 1.7MB +724.0B

Page load bundle

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

id before after diff
visTypeTimeseries 25.3KB 25.5KB +252.0B

History

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

cc @dziyanadzeraviankina

@timroes
Copy link
Contributor

timroes commented Apr 30, 2021

@elasticmachine run elasticsearch-ci/docs

@DianaDerevyankina DianaDerevyankina merged commit 6b6ad11 into elastic:master Apr 30, 2021
DianaDerevyankina added a commit to DianaDerevyankina/kibana that referenced this pull request Apr 30, 2021
* [TSVB] Timeseries Drop last bucket should default to false

* Rename isLastBucketDropped prop and move series domain calculation to a separate file

* Fix failing tests because of wrong default value

* update drop_last_bucket.js

* Refactor drop_last_bucket and some functional tests

* Change infra metrics test values because of last bucket value changed

* Refactor series_domain_calculation and related code

* Update series_domain_calculations.test

* Update series_domain_calculations.test

* Fix tooltip showing wrong time

* Refactor index

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
DianaDerevyankina added a commit that referenced this pull request Apr 30, 2021
)

* [TSVB] Timeseries Drop last bucket should default to false

* Rename isLastBucketDropped prop and move series domain calculation to a separate file

* Fix failing tests because of wrong default value

* update drop_last_bucket.js

* Refactor drop_last_bucket and some functional tests

* Change infra metrics test values because of last bucket value changed

* Refactor series_domain_calculation and related code

* Update series_domain_calculations.test

* Update series_domain_calculations.test

* Fix tooltip showing wrong time

* Refactor index

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>
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:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Timeseries Drop last bucket should default to false
10 participants