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][Agg based XY] Convert to Lens Agg based XY. #142936

Merged
merged 26 commits into from
Oct 14, 2022

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Oct 7, 2022

Completes part of #138236

Summary

As part of phasing out TSVB and Visualize all Legacy agg based visulizations should support "open in lens" functionality.
In that PR converter for Agg based XY was added.


⚠️ Output from convert_to_lens common library was updated, all agg based visualization with conversion logic should be re-checked


Some conversion remarks:

  • Lens supports only ‘button“ as position for x-axis, because of this we ignore x-axis position from agg based xy
  • Line stacked converts to Area stacked
  • Any percentage chart converts to percentage stacked chart
  • Ignore “scale to data” for area and bars
  • Ignore '_ all' (in agg based xy when x-axis is not specified we show '_all')
  • Ignore “bounds margin”
  • As in lens for customs y axis extents (area, bar) we should include 0 in range but in agg based xy it is not required we update extents to include 0
  • In lens only bar chart can be horizontal, ignore rotation for other charts
  • Ignore “Hide tooltip”
  • Ignore Wiggle/Silhouette axis mode
  • Ignore stepped mode, get line mode for Lens only from the first metric

Some block cases:

  • More than one axis left/right/top/bottom
  • Configured metric for dot size
  • Configured split chart
  • Multiple split series
  • Sibling pipeline aggregation + split series
  • If we have more than one y and terms aggregation uses one of them as order by

@VladLasitsa VladLasitsa added the WIP Work in progress label Oct 7, 2022
@VladLasitsa VladLasitsa self-assigned this Oct 7, 2022
@VladLasitsa VladLasitsa added Feature:XYAxis XY-Axis charts (bar, area, line) Feature:Lens labels Oct 7, 2022
@VladLasitsa VladLasitsa added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0 release_note:enhancement backport:skip This commit does not require backporting labels Oct 12, 2022
Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

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

Thanks for this PR 😃
I've started reviewing, those are the first comments.

Screenshot 2022-10-12 at 16 05 18

Screenshot 2022-10-12 at 16 05 44

Screenshot 2022-10-12 at 16 06 00

Discussed, valid case. ✅

Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

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

Screenshot 2022-10-12 at 16 08 02

Screenshot 2022-10-12 at 16 08 59

Discussed, valid case. ✅

Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

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

Screenshot 2022-10-12 at 16 11 03
Screenshot 2022-10-12 at 16 12 09

Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

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

Screenshot 2022-10-12 at 16 13 03
Screenshot 2022-10-12 at 16 13 38

@VladLasitsa
Copy link
Contributor Author

  • This chart in agg based
image

results in a wrong chart (see the extra blank dimension) image This happens for all Parent pipeline aggs

Fixed

  • This configuration (2 split series) results in a broken chart
image

That should not be converted, fixed. We don't support multi split series

  • The Show values in chart setting is not configured in Lens

Fixed

@VladLasitsa
Copy link
Contributor Author

  • We don't have the ability to overwrite colod by terms but you can specify a custom color if you don't have a breakdown. I wonder if it makes sense to carry the overwrite color in these cases. For example this color could be converted to Lens
image

@stratoula I've checked this. We get color from uiState by series name and this logic is located in chart-expressions/expression-xy.
The function which generate series name you can find here: src/plugins/chart_expressions/expression_xy/public/helpers/data_layers.tsx:251
Honestly I am not sure that we can correctly configure series name in configuration part so that get correct color from uiState.

cc @flash1293

@VladLasitsa
Copy link
Contributor Author

VladLasitsa commented Oct 13, 2022

In TSVB it works like that because we have different layers (that can have different dataviews etc). So each layer is translated to a new lens layer. It makes more sense there. I get the comment, I just find it weird to have 3 layers with the exact same configuration and the only difference is the metric. If we identify that the series are of the same type? In that case, create one layer otherwise create one per chart type.

@stratoula Also we should check the mode because each metric can has its own mode (normal/stacked). Only if chart type and mode is the same theoretically we can put in in one layer.

In addition we should not forget about these conditions too:

  • Line stacked converts to Area stacked
  • Any percentage chart converts to percentage stacked chart

it means - metric with line stacked and metric with area stacked should be converted in one layer?

@flash1293 what do you think about that?

@stratoula
Copy link
Contributor

All these comments are things that I find weird as a user or that I expect to work differently.

  • About the labels thingy, if there is a technical limitation I am fine to keep it as it is although I am a fan of consistency :) Nevertheless a user can always clear the label we propose and use the lens default label. So I am cool with it 👍
  • About the overwrite color. It would be very cool to also transition the color but again if this is a technical limitation I am also fine.
  • About the multiple layers per metric, I really don't think that is the same case as TSVB (regardless of the names we have given to our code :D ) but I do understand the complexity that my proposal adds. Honestly whatever Joe prefers here.

@flash1293
Copy link
Contributor

Thanks for discussing these points. My takes:

  • About labels - if we can't tell whether a name is default or not, I'm OK with keeping it like this for now.
  • About colors - the full logic for mapping colors is complex, but Lens doesn't allow the more complex cases anyway (the ones about coloring a specific term out of a split series). The lookup for individual metric series should be pretty straightforward and I think it's worth implementing (I think it should just be comparing the label of the series with the color map in uiState). We do have the uiState available at the time of the transition, right?
  • About the layers - if it's possible to consolidate the individual series into a single lens layer (if they are all using the same series type which is very common by our telemetry), then I think we should do it
  • Current time marker is a feature in Lens, we can map it directly:

Screenshot 2022-10-13 at 11 52 08

@VladLasitsa
Copy link
Contributor Author

  • About colors - the full logic for mapping colors is complex, but Lens doesn't allow the more complex cases anyway (the ones about coloring a specific term out of a split series). The lookup for individual metric series should be pretty straightforward and I think it's worth implementing (I think it should just be comparing the label of the series with the color map in uiState). We do have the uiState available at the time of the transition, right?

@flash1293 It's not so easy we don't just use series name from vis params. We generate it based on accessors and titles.
You can see it here src/plugins/chart_expressions/expression_xy/public/helpers/data_layers.tsx:251

@VladLasitsa
Copy link
Contributor Author

  • About the layers - if it's possible to consolidate the individual series into a single lens layer (if they are all using the same series type which is very common by our telemetry), then I think we should do it

Done

  • Current time marker is a feature in Lens, we can map it directly:
Screenshot 2022-10-13 at 11 52 08

Done

@flash1293 , @stratoula Could you please check it?

@stratoula
Copy link
Contributor

stratoula commented Oct 13, 2022

@VladLasitsa about the color. I get that you don't have the information needed by the getSeriesName to find the color as we do on the data layers file. But maybe you can find the color with the data we already have. I was playing with it and I see the following pattern:

image

The name can be

  • The custom label
  • If no custom label then it is "Aggregation FieldName"
  • In case of breakdown is the value of the series (but we can skip it for now as we don't have this feature in Lens)

I think it is possible to find the color with the above information.

@VladLasitsa
Copy link
Contributor Author

@VladLasitsa about the color. I get that you don't have the information needed by the getSeriesName to find the color as we do on the data layers file. But maybe you can find the color with the data we already have. I was playing with it and I see the following pattern:

image

The name can be

  • The custom label
  • If no custom label then it is "Aggregation FieldName"
  • In case of breakdown is the value of the series (but we can skip it for now as we don't have this feature in Lens)

I think it is possible to find the color with the above information.

Thank you, I got it, Done. Could you please review?

@stratoula
Copy link
Contributor

Thanx Vlad, the fixes look awesome. Can you also fix the issue mentioned by Joe here? #142936 (comment)

@VladLasitsa
Copy link
Contributor Author

VladLasitsa commented Oct 13, 2022

Thanx Vlad, the fixes look awesome. Can you also fix the issue mentioned by Joe here? #142936 (comment)

@stratoula, It's a that block case: If we have more than one y and terms aggregation uses one of them as order by. I have already discussed it with Joe

@stratoula
Copy link
Contributor

I am pretty sure that this is convertable in Lens. Here I can use the metric I want to order by

image

@VladLasitsa
Copy link
Contributor Author

I am pretty sure that this is convertable in Lens. Here I can use the metric I want to order by

image

What if metrics will produce different layers?

@stratoula
Copy link
Contributor

Personally I would still allow the transition and order the breakdown by the metric. @flash1293 wdyt? I am also fine on blocking this for this scenario but we should def allow it in case of the same layer.

@VladLasitsa
Copy link
Contributor Author

Personally I would still allow the transition and order the breakdown by the metric. @flash1293 wdyt? I am also fine on blocking this for this scenario but we should def allow it in case of the same layer.

@stratoula Now we can convert terms with order by existing metric if all metrics in the same layer.

cc @flash1293

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.

This looks good to me now. LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeXy 43 49 +6

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 147 148 +1
lens 566 567 +1
visualizations 724 725 +1
total +3

Async chunks

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

id before after diff
expressionXY 119.0KB 119.1KB +114.0B
visTypeGauge 7.1KB 7.1KB +29.0B
visTypeMetric 464.0B 493.0B +29.0B
visTypeXy 33.0KB 40.3KB +7.3KB
visualizations 266.9KB 268.0KB +1.0KB
total +8.5KB

Page load bundle

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

id before after diff
expressionXY 37.0KB 37.1KB +87.0B
visTypeGauge 13.8KB 13.9KB +30.0B
visTypeMetric 12.7KB 12.7KB +15.0B
visTypePie 12.2KB 12.2KB +23.0B
visTypeTable 19.4KB 19.5KB +64.0B
visTypeXy 27.6KB 31.0KB +3.4KB
total +3.6KB
Unknown metric groups

API count

id before after diff
expressionXY 157 158 +1
lens 657 658 +1
visualizations 754 755 +1
total +3

async chunk count

id before after diff
visTypeXy 2 4 +2

History

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

cc @VladLasitsa

@VladLasitsa VladLasitsa merged commit 3cb5fed into elastic:main Oct 14, 2022
kibanamachine added a commit that referenced this pull request Dec 8, 2022
# Backport

This will backport the following commits from `main` to `8.6`:
- [Adds the VisEditor docs for 8.6
(#146471)](#146471)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kaarina
Tungseth","email":"kaarina.tungseth@elastic.co"},"sourceCommit":{"committedDate":"2022-12-08T20:18:03Z","message":"Adds
the VisEditor docs for 8.6 (#146471)\n\n## Summary\r\n\r\nAdds the 8.6
for the following:\r\n\r\n- #140878, #143946 and
#142187\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/tsvb.html#edit-visualizations-in-lens)\r\n\r\n-
#142936, #142561, #143820, and
#142838\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/add-aggregation-based-visualization-panels.html#edit-agg-based-visualizations-in-lens)\r\n\r\n-
#138732\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#change-the-fields)\r\n\r\n-
#141626 and #141615\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations)\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Stratoula Kalafateli
<stratoula1@gmail.com>","sha":"f918a3745be3badff9cf05950db61d7f47877961","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","release_note:skip","v8.6.0","v8.7.0"],"number":146471,"url":"https://github.com/elastic/kibana/pull/146471","mergeCommit":{"message":"Adds
the VisEditor docs for 8.6 (#146471)\n\n## Summary\r\n\r\nAdds the 8.6
for the following:\r\n\r\n- #140878, #143946 and
#142187\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/tsvb.html#edit-visualizations-in-lens)\r\n\r\n-
#142936, #142561, #143820, and
#142838\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/add-aggregation-based-visualization-panels.html#edit-agg-based-visualizations-in-lens)\r\n\r\n-
#138732\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#change-the-fields)\r\n\r\n-
#141626 and #141615\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations)\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Stratoula Kalafateli
<stratoula1@gmail.com>","sha":"f918a3745be3badff9cf05950db61d7f47877961"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/146471","number":146471,"mergeCommit":{"message":"Adds
the VisEditor docs for 8.6 (#146471)\n\n## Summary\r\n\r\nAdds the 8.6
for the following:\r\n\r\n- #140878, #143946 and
#142187\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/tsvb.html#edit-visualizations-in-lens)\r\n\r\n-
#142936, #142561, #143820, and
#142838\r\n\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/add-aggregation-based-visualization-panels.html#edit-agg-based-visualizations-in-lens)\r\n\r\n-
#138732\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#change-the-fields)\r\n\r\n-
#141626 and #141615\r\n
\r\n[Doc\r\npreview](https://kibana_146471.docs-preview.app.elstc.co/guide/en/kibana/master/lens.html#add-annotations)\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Stratoula Kalafateli
<stratoula1@gmail.com>","sha":"f918a3745be3badff9cf05950db61d7f47877961"}}]}]
BACKPORT-->

Co-authored-by: Kaarina Tungseth <kaarina.tungseth@elastic.co>
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 Feature:XYAxis XY-Axis charts (bar, area, line) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants