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

[Lens] support legendStats along with valuesInLegend in preparation for legend statistics #180917

Merged
merged 6 commits into from
Apr 26, 2024

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Apr 16, 2024

Summary

This PR addresses phase one of #181035.

Doesn't introduce any user facing changes.
It starts supporting a new saved object property legendStats while supporting a old valuesInLegend property. In this PR, legendStats: ['values'] and valuesInLegend:true are treated as equal. When loading the saved object, valuesInLegend:true is transformed to legendStats:['values']. After loading the document, the Lens app logic is built around the new legendStats property.
When user saves the saved object, we do a reverse operation- we save the runtime state legendStats:['values'] as valuesInLegend: true to ensure backwards compatibility.

image

Changes for runtime state:

  • For xyCharts, the valuesInLegend?: boolean property is replaced with a more extensible legend.legendStats?: LegendStats[] interface

  • For partition charts, the showValuesInLegend?: boolean property is replaced with legendStats?: LegendStats[].

after loading - in initialize function:

export function convertToRuntime(
  state: XYPersistedState,
  annotationGroups?: AnnotationGroups,
  references?: SavedObjectReference[]
) {
  const outputState = needsInjectReferences(state)
    ? injectReferences(state, annotationGroups, references)
    : state;
  if ('valuesInLegend' in outputState) {
    return convertToLegendStats(outputState);
  }
  return outputState;
}

before saving :

export function convertToPersistable(state: XYState) {
  const persistableState: XYPersistedState = convertToValuesInLegend(state);
  /.../
}

In the future the legendStats prop would contain also other types of stats -see the issue.

@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v8.14.0 labels Apr 16, 2024
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch 2 times, most recently from 4bdd7ae to b818a90 Compare April 17, 2024 09:59
@mbondyra mbondyra changed the title [Lens] migrate valuesInLegend to legendStats in preparation for legend statistics [Lens] support legendStats along with valuesInLegend in preparation for legend statistics Apr 17, 2024
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch 3 times, most recently from 436148c to bfa79b2 Compare April 18, 2024 09:58
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch 3 times, most recently from 0e41043 to 63c8ec8 Compare April 18, 2024 11:21
@mbondyra mbondyra changed the title [Lens] support legendStats along with valuesInLegend in preparation for legend statistics [Lens] support legendStats along with valuesInLegend in preparation for legend statistics Apr 18, 2024
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch 2 times, most recently from 5f01d9d to b1936c9 Compare April 18, 2024 12:42
@elastic elastic deleted a comment from kibana-ci Apr 18, 2024
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch from b1936c9 to 8655045 Compare April 18, 2024 13:08
@elastic elastic deleted a comment from kibana-ci Apr 18, 2024
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch 8 times, most recently from 64abf10 to 7feb827 Compare April 23, 2024 12:10
@elastic elastic deleted a comment from kibana-ci Apr 23, 2024
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch from 7feb827 to cb566db Compare April 23, 2024 12:34
@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch from fea3cc3 to 2a5c6e7 Compare April 24, 2024 10:11
@elastic elastic deleted a comment from kibana-ci Apr 24, 2024
@mbondyra mbondyra marked this pull request as ready for review April 24, 2024 11:36
@mbondyra mbondyra requested review from a team as code owners April 24, 2024 11:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

Obs Ux Infra changes LGTM.

@mbondyra We are passed FF, should this be targeted to 8.15 instead?

@botelastic botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Apr 24, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@mbondyra mbondyra added v8.15.0 and removed v8.14.0 labels Apr 24, 2024
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Done a first code review pass. Left some comments.
Will check locally in a bit...

Copy link
Contributor

@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.

Looks good overall. I just have some ideas on how to support future migrations we want to add. Using some kind of pass-through migration, similar to flow, that migrates something and passes it to the next migration. Both converting to and from persitableState, while avoiding multiple deep clones of the state.

@mbondyra mbondyra force-pushed the lens/migrate_show_values_to_array branch from 96a22d4 to 1592b96 Compare April 24, 2024 21:13
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1405 1407 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
expressionXY 167 166 -1
lens 569 567 -2
visualizations 831 832 +1
total -2

Async chunks

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

id before after diff
expressionPartitionVis 36.4KB 36.5KB +57.0B
expressionXY 128.4KB 128.5KB +60.0B
lens 1.4MB 1.4MB +974.0B
observability 287.3KB 287.3KB +5.0B
observabilityAIAssistantApp 151.1KB 151.1KB +5.0B
ux 169.8KB 169.9KB +93.0B
visTypePie 11.2KB 11.3KB +80.0B
visTypeXy 42.2KB 42.3KB +35.0B
total +1.3KB

Page load bundle

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

id before after diff
expressionPartitionVis 26.7KB 26.6KB -13.0B
expressionXY 38.8KB 38.9KB +90.0B
visTypePie 8.4KB 8.5KB +154.0B
visTypeXy 28.5KB 28.7KB +182.0B
visualizations 58.9KB 59.0KB +94.0B
total +507.0B
Unknown metric groups

API count

id before after diff
lens 670 669 -1
visualizations 862 863 +1
total -0

References to deprecated APIs

id before after diff
lens 113 114 +1

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

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally and it works great! 🥳

@mbondyra mbondyra merged commit 52ca6ad into elastic:main Apr 26, 2024
19 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 26, 2024
@mbondyra mbondyra deleted the lens/migrate_show_values_to_array branch April 26, 2024 09:04
mbondyra added a commit that referenced this pull request May 3, 2024
…ration for legend statistics (#181953)

## Summary

To the code owners,

This PR updates the saved object of Lens with the following changes: the
property `valuesInLegend: boolean` is converted to the new property
`legendStats: ["currentAndLastValue"]` (cartesian) or `legendStats:
["value"]`(partition) to accommodate displaying additional legend
statistics in the future. There are no user-facing changes introduced. I
have updated all saved object shapes in the code to adhere to the new
structure. While the old structure will continue to function properly,
this update ensures cleaner code.

Fixes #181035 (phase 2). It's is
a continuation of #180917.

In comparison to the phase 1, we've removed the conversion of
legendStats to valuesInLegend which was initially added in the previous
PR to ensure backward compatibility for serverless applications.

<img width="807" alt="Screenshot 2024-04-25 at 10 51 50"
src="https://github.com/elastic/kibana/assets/4283304/6c23d035-f64d-4fb1-96c6-d30392ae6752">

I won't merge till the end of the week as it has to be released
separately from phase 1.

---------

Co-authored-by: Cauê Marcondes <[email protected]>
Co-authored-by: Alex Szabo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants