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

fix(legend): improve last value handling #2115

Merged
merged 24 commits into from
Jan 9, 2024

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jul 27, 2023

Summary

This PR clean up the concept of what the value, shown after the legend item label, represents.

In cartesian charts, the value represents the last available data point laid on the X-axis.
When multiple series are displayed, the last value represents only the data points with the same X as the greatest value on the X axis.

This value, in the future, can also represent another aspect of the series, like the last non-null value, an average/max/min value, but this PR limits is scope to the last value.

We removed every concept of bucket/time interval from this calculation to clarify the legend item value concept.
This allows us to decouple our code from Elasticsearch or other data sources.
A small, but fixable side-effect is:

  • the last value depends entirely on the passed data, so if you pass just 10 values of a series that should cover 100 data points, we consider the last value the last of these 10 values: if the 10th value is not at the extreme edge of your data domain we still consider that 10th value as the last one. If you instead, are looking to depict the extreme edge of your data domain (the 100th element) that in the mentioned example is empty/null then you should pass all the 100 data points with null values where needed.

Related issues
elastic/kibana#153079
elastic/kibana#94280

BREAKING CHANGE

In cartesian charts, the default legend value now represents the data points that coincide with the latest datum in the X domain. Please consider passing every data point, even the empty ones (like empty buckets/bins/etc) if your x data domain doesn't cover fully a custom x domain passed to the chart configuration.

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@markov00 markov00 force-pushed the 2023_07_27-fix_last_bucket_time branch from 19e3f98 to b3ad6fe Compare October 2, 2023 14:10
@markov00
Copy link
Member Author

markov00 commented Dec 1, 2023

buildkite test this

@markov00 markov00 added bug Something isn't working enhancement New feature or request :data Data/series/scales related issue :xy Bar/Line/Area chart related breaking change labels Dec 18, 2023
@markov00 markov00 force-pushed the 2023_07_27-fix_last_bucket_time branch from 7638ba4 to 1df6977 Compare December 19, 2023 08:34
@markov00
Copy link
Member Author

buildkite update screenshots

@markov00 markov00 marked this pull request as ready for review December 20, 2023 17:21
@markov00 markov00 changed the title fix(legend): last time bucket for legend extra fix(legend): improve last value handling Dec 21, 2023
@markov00
Copy link
Member Author

markov00 commented Jan 8, 2024

@nickofthyme could you please review this?

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Changes LGTM, just one suggestion on the story styles and a question.

@markov00 markov00 merged commit 9f99447 into elastic:main Jan 9, 2024
13 checks passed
@markov00 markov00 deleted the 2023_07_27-fix_last_bucket_time branch January 9, 2024 09:23
nickofthyme pushed a commit that referenced this pull request Jan 23, 2024
# [62.0.0](v61.2.0...v62.0.0) (2024-01-23)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^91.3.1 ([#2286](#2286)) ([d4d7b5d](d4d7b5d))
* **deps:** update dependency @elastic/eui to v92 ([#2290](#2290)) ([cc537fa](cc537fa))
* **legend:** improve last value handling ([#2115](#2115)) ([9f99447](9f99447))

### BREAKING CHANGES

* **legend:** In cartesian charts, the default legend value now represents the data points that coincide with the latest datum in the X domain. Please consider passing every data point, even the empty ones (like empty buckets/bins/etc) if your x data domain doesn't fully cover a custom x domain passed to the chart configuration.
mistic pushed a commit to elastic/kibana that referenced this pull request Feb 8, 2024
## Note about `@elastic/charts` BREAKING CHANGE

In version 62.0.0 we introduced a breaking change in time-series charts:
the default "extra" legend value now represents the last data point in
the passed data array. It doesn't try to reconcile anymore the data
computed domain with a passed domain in `Settings.xDomain` but instead
it renders directly the last element of the passed array.
The reasons for this change can be found at
elastic/elastic-charts#2115 or can be asked
directly to our `#charts` slack channel

There are a couple of implementations in Kibana that use both the
`showLegendExtra` in the chart configuration. I've commented them out so
that the owner teams can help me fix this breaking change if necessary.

In general, the fix requires that the data passed to the chart contains
all the buckets, even empty buckets with null/zeros should be passed. To
achieve this, your ES query you should provide the `extended_bounds`
settings in the [data histogram
agg](https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-histogram-aggregation.html#search-aggregations-bucket-histogram-aggregation-extended-bounds)
and use a `min_doc_count:0`. If that doesn't work, please ping me and we
can find an alternative solution.

This should not limit the query performance, generating empty date
buckets on the server side has a similar or even less performance impact
than what we were doing on the client side to calculate every missing
bucket, to fillup the chart in particular situations.

Please double-check your queries/data fetches and push a commit to this
PR or ping me with the updated data fetch strategy.
 

fix #153079


This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@elastic/charts](https://togithub.com/elastic/elastic-charts) |
[`61.2.0` ->
`63.0.0`](https://renovatebot.com/diffs/npm/@elastic%2fcharts/61.2.0/63.0.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@elastic%2fcharts/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@elastic%2fcharts/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@elastic%2fcharts/61.2.0/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@elastic%2fcharts/61.2.0/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>elastic/elastic-charts (@&#8203;elastic/charts)</summary>

###
[`v63.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6300-2024-01-24)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v62.0.0...v63.0.0)

##### Features

- **legend:** expose extra raw values
([#&#8203;2308](https://togithub.com/elastic/elastic-charts/issues/2308))
([85bfe06](https://togithub.com/elastic/elastic-charts/commit/85bfe0668d66fd24e78f2bba8be4570fa926e94c))

##### BREAKING CHANGES

- **legend:** The `CustomLegend.item` now exposes both the `raw` and the
`formatted` version of the extra value.

###
[`v62.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6200-2024-01-23)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v61.2.0...v62.0.0)

##### Bug Fixes

- **deps:** update dependency
[@&#8203;elastic/eui](https://togithub.com/elastic/eui) to ^91.3.1
([#&#8203;2286](https://togithub.com/elastic/elastic-charts/issues/2286))
([d4d7b5d](https://togithub.com/elastic/elastic-charts/commit/d4d7b5db6681ec0c65ef8b7e576f1b5fc8b5433a))
- **deps:** update dependency
[@&#8203;elastic/eui](https://togithub.com/elastic/eui) to v92
([#&#8203;2290](https://togithub.com/elastic/elastic-charts/issues/2290))
([cc537fa](https://togithub.com/elastic/elastic-charts/commit/cc537faf43d88acc9abab7e0dac9360bd460b574))
- **legend:** improve last value handling
([#&#8203;2115](https://togithub.com/elastic/elastic-charts/issues/2115))
([9f99447](https://togithub.com/elastic/elastic-charts/commit/9f9944734c4a13bfe9e4ffc9f4c0f39da5f9931f))

##### BREAKING CHANGES

- **legend:** In cartesian charts, the default legend value now
represents the data points that coincide with the latest datum in the X
domain. Please consider passing every data point, even the empty ones
(like empty buckets/bins/etc) if your x data domain doesn't fully cover
a custom x domain passed to the chart configuration.

</details>

---

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Elena Stoeva <elenastoeva99@gmail.com>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
## Note about `@elastic/charts` BREAKING CHANGE

In version 62.0.0 we introduced a breaking change in time-series charts:
the default "extra" legend value now represents the last data point in
the passed data array. It doesn't try to reconcile anymore the data
computed domain with a passed domain in `Settings.xDomain` but instead
it renders directly the last element of the passed array.
The reasons for this change can be found at
elastic/elastic-charts#2115 or can be asked
directly to our `#charts` slack channel

There are a couple of implementations in Kibana that use both the
`showLegendExtra` in the chart configuration. I've commented them out so
that the owner teams can help me fix this breaking change if necessary.

In general, the fix requires that the data passed to the chart contains
all the buckets, even empty buckets with null/zeros should be passed. To
achieve this, your ES query you should provide the `extended_bounds`
settings in the [data histogram
agg](https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-histogram-aggregation.html#search-aggregations-bucket-histogram-aggregation-extended-bounds)
and use a `min_doc_count:0`. If that doesn't work, please ping me and we
can find an alternative solution.

This should not limit the query performance, generating empty date
buckets on the server side has a similar or even less performance impact
than what we were doing on the client side to calculate every missing
bucket, to fillup the chart in particular situations.

Please double-check your queries/data fetches and push a commit to this
PR or ping me with the updated data fetch strategy.
 

fix elastic#153079


This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@elastic/charts](https://togithub.com/elastic/elastic-charts) |
[`61.2.0` ->
`63.0.0`](https://renovatebot.com/diffs/npm/@elastic%2fcharts/61.2.0/63.0.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@elastic%2fcharts/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@elastic%2fcharts/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@elastic%2fcharts/61.2.0/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@elastic%2fcharts/61.2.0/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>elastic/elastic-charts (@&elastic#8203;elastic/charts)</summary>

###
[`v63.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6300-2024-01-24)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v62.0.0...v63.0.0)

##### Features

- **legend:** expose extra raw values
([#&elastic#8203;2308](https://togithub.com/elastic/elastic-charts/issues/2308))
([85bfe06](https://togithub.com/elastic/elastic-charts/commit/85bfe0668d66fd24e78f2bba8be4570fa926e94c))

##### BREAKING CHANGES

- **legend:** The `CustomLegend.item` now exposes both the `raw` and the
`formatted` version of the extra value.

###
[`v62.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6200-2024-01-23)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v61.2.0...v62.0.0)

##### Bug Fixes

- **deps:** update dependency
[@&elastic#8203;elastic/eui](https://togithub.com/elastic/eui) to ^91.3.1
([#&elastic#8203;2286](https://togithub.com/elastic/elastic-charts/issues/2286))
([d4d7b5d](https://togithub.com/elastic/elastic-charts/commit/d4d7b5db6681ec0c65ef8b7e576f1b5fc8b5433a))
- **deps:** update dependency
[@&elastic#8203;elastic/eui](https://togithub.com/elastic/eui) to v92
([#&elastic#8203;2290](https://togithub.com/elastic/elastic-charts/issues/2290))
([cc537fa](https://togithub.com/elastic/elastic-charts/commit/cc537faf43d88acc9abab7e0dac9360bd460b574))
- **legend:** improve last value handling
([#&elastic#8203;2115](https://togithub.com/elastic/elastic-charts/issues/2115))
([9f99447](https://togithub.com/elastic/elastic-charts/commit/9f9944734c4a13bfe9e4ffc9f4c0f39da5f9931f))

##### BREAKING CHANGES

- **legend:** In cartesian charts, the default legend value now
represents the data points that coincide with the latest datum in the X
domain. Please consider passing every data point, even the empty ones
(like empty buckets/bins/etc) if your x data domain doesn't fully cover
a custom x domain passed to the chart configuration.

</details>

---

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Elena Stoeva <elenastoeva99@gmail.com>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
## Note about `@elastic/charts` BREAKING CHANGE

In version 62.0.0 we introduced a breaking change in time-series charts:
the default "extra" legend value now represents the last data point in
the passed data array. It doesn't try to reconcile anymore the data
computed domain with a passed domain in `Settings.xDomain` but instead
it renders directly the last element of the passed array.
The reasons for this change can be found at
elastic/elastic-charts#2115 or can be asked
directly to our `#charts` slack channel

There are a couple of implementations in Kibana that use both the
`showLegendExtra` in the chart configuration. I've commented them out so
that the owner teams can help me fix this breaking change if necessary.

In general, the fix requires that the data passed to the chart contains
all the buckets, even empty buckets with null/zeros should be passed. To
achieve this, your ES query you should provide the `extended_bounds`
settings in the [data histogram
agg](https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-histogram-aggregation.html#search-aggregations-bucket-histogram-aggregation-extended-bounds)
and use a `min_doc_count:0`. If that doesn't work, please ping me and we
can find an alternative solution.

This should not limit the query performance, generating empty date
buckets on the server side has a similar or even less performance impact
than what we were doing on the client side to calculate every missing
bucket, to fillup the chart in particular situations.

Please double-check your queries/data fetches and push a commit to this
PR or ping me with the updated data fetch strategy.
 

fix elastic#153079


This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@elastic/charts](https://togithub.com/elastic/elastic-charts) |
[`61.2.0` ->
`63.0.0`](https://renovatebot.com/diffs/npm/@elastic%2fcharts/61.2.0/63.0.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@elastic%2fcharts/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@elastic%2fcharts/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@elastic%2fcharts/61.2.0/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@elastic%2fcharts/61.2.0/63.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>elastic/elastic-charts (@&elastic#8203;elastic/charts)</summary>

###
[`v63.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6300-2024-01-24)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v62.0.0...v63.0.0)

##### Features

- **legend:** expose extra raw values
([#&elastic#8203;2308](https://togithub.com/elastic/elastic-charts/issues/2308))
([85bfe06](https://togithub.com/elastic/elastic-charts/commit/85bfe0668d66fd24e78f2bba8be4570fa926e94c))

##### BREAKING CHANGES

- **legend:** The `CustomLegend.item` now exposes both the `raw` and the
`formatted` version of the extra value.

###
[`v62.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#6200-2024-01-23)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v61.2.0...v62.0.0)

##### Bug Fixes

- **deps:** update dependency
[@&elastic#8203;elastic/eui](https://togithub.com/elastic/eui) to ^91.3.1
([#&elastic#8203;2286](https://togithub.com/elastic/elastic-charts/issues/2286))
([d4d7b5d](https://togithub.com/elastic/elastic-charts/commit/d4d7b5db6681ec0c65ef8b7e576f1b5fc8b5433a))
- **deps:** update dependency
[@&elastic#8203;elastic/eui](https://togithub.com/elastic/eui) to v92
([#&elastic#8203;2290](https://togithub.com/elastic/elastic-charts/issues/2290))
([cc537fa](https://togithub.com/elastic/elastic-charts/commit/cc537faf43d88acc9abab7e0dac9360bd460b574))
- **legend:** improve last value handling
([#&elastic#8203;2115](https://togithub.com/elastic/elastic-charts/issues/2115))
([9f99447](https://togithub.com/elastic/elastic-charts/commit/9f9944734c4a13bfe9e4ffc9f4c0f39da5f9931f))

##### BREAKING CHANGES

- **legend:** In cartesian charts, the default legend value now
represents the data points that coincide with the latest datum in the X
domain. Please consider passing every data point, even the empty ones
(like empty buckets/bins/etc) if your x data domain doesn't fully cover
a custom x domain passed to the chart configuration.

</details>

---

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Elena Stoeva <elenastoeva99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something isn't working :data Data/series/scales related issue enhancement New feature or request :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants