[Lens] Move Lens functions to `common` by dej611 · Pull Request #105455 · elastic/kibana

dej611 · 2021-07-13T15:19:58Z

Summary

Fixes #97134 (first part)

List of functions:

More tasks:

Move shared types

~~Probably need a follow up on this PR to split the datatable function into 2 separate expression functions (1 data processing + 1 rendering)~~

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-07-16T17:34:25Z

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

dej611 · 2021-07-16T17:35:41Z

@elasticmachine merge upstream

dej611 · 2021-07-19T08:32:49Z

@elasticmachine merge upstream

wylieconlon

I've done a cursory scan over the code here and I have a few questions, if necessary we should have a meeting to get everyone aligned. Here are the questions:

Why do we need to move the render functions to the server? I understand moving the intermediate functions like time_scale, but why lens_xy_chart?
How will time zones be handled?
Why do we need to register the functions to the server in this PR, when the alternative approach is to only move to common in this PR?

wylieconlon · 2021-07-19T18:38:06Z

x-pack/plugins/lens/common/expressions/time_scale/time_scale.ts

+    // the datemath plugin always parses dates by using the current default moment time zone.
+    // to use the configured time zone, we are switching just for the bounds calculation.
+    const defaultTimezone = moment().zoneName();
+    moment.tz.setDefault(timeInfo.timeZone);


What I'm seeing is that timeInfo.timeZone is undefined most of the time, because the time zone is not provided on esaggs by default. The time zone information might need to be sourced from an alternative place. For example, if the alerting task starts it, it might need to store a timezone parameter that gets passed in as part of the expression context. Have you considered that here?

I've created a new argument for time_scale to pass a timeZone to be used on the server side: you think it would be better to extend the context instead?

I'm not sure how this will be handled, I think it will require discussion with the app services & alerting teams.

Left a comment on the main PR thread: this topic will be discussed in a follow up PR completely dedicated to the server side registration.

wylieconlon

@dej611 I've tried to manually review this and mostly the changes LGTM, splitting out the huge types.ts files into multiple files is a good change. The main thing I'm confused about is that you've moved some, but not all, expression renderers into common along with the core types like LensMultiTable. I thought you were going to do a smaller change?

wylieconlon · 2021-07-20T18:52:34Z

x-pack/plugins/lens/common/expressions/datatable/datatable.ts

+  DatatableRender
+> => ({
+  name: 'lens_datatable',
+  type: 'render',


Note: this lens_databable renderer is moved, trying to understand why.

Moved back lens_datatable function and all its dependencies to public. Will dedicate a PR to this as follow up.

wylieconlon · 2021-07-20T18:55:51Z

x-pack/plugins/lens/common/expressions/datatable/datatable_column.ts

+export type LensGridDirection = 'none' | Direction;
+
+export interface ColumnConfig {
+  columns: Array<


Why have you changed this type, it no longer uses columns: ColumnConfigArg[] which means you're missing the summaryRowValue type?

Seems like there's no reason to copy+paste the definition here

wylieconlon · 2021-07-20T18:59:09Z

x-pack/plugins/lens/common/expressions/datatable/datatable_column.ts

+  originalColumnId?: string;
+  originalName?: string;
+  bucketValues?: Array<{ originalBucketColumn: DatatableColumn; value: unknown }>;


I can see that you've copied the ColumnState definition from datatable/visualization.tsx but these three types appear to be completely unused. Can you verify that and maybe remove them?

All of these types are used in the transposeTable function flow within the lens_datatable expression function

wylieconlon · 2021-07-20T19:02:43Z

x-pack/plugins/lens/common/expressions/datatable/datatable.ts

+  sortingColumnId: string | undefined;
+  sortingDirection: 'asc' | 'desc' | 'none';


This change LGTM, previously it was called sorting?: SortingState in datatable_visualization/visualization.tsx

wylieconlon · 2021-07-20T19:09:36Z

x-pack/plugins/lens/common/expressions/heatmap_chart/types.ts

+import { HeatmapGridConfigResult } from './heatmap_grid';
+import { HeatmapLegendConfigResult } from './heatmap_legend';
+
+export type ChartShapes = 'heatmap';


This type has been simplified from typeof CHART_SHAPES[keyof typeof CHART_SHAPES] to just heatmap. Seems fine but it's the only change to heatmap types I can find

Correct. I assumed that in case of generalisation it could be augmented later on here.

wylieconlon · 2021-07-20T19:09:59Z

x-pack/plugins/lens/common/expressions/rename_columns/rename_columns.ts

+type OriginalColumn = { id: string; label: string } & (
+  | { operationType: 'date_histogram'; sourceField: string }
+  | { operationType: string; sourceField: never }
+);


Not entirely sure what this is about?

Unfortunately the type OriginalColumn depends on all the operation definitions (it's a union of them) and moving them here it is not necessary, or useful now.
The goal of this type is to make it work correctly the getColumnName which has a specific logic for date_histogram, therefore the type has been simplified to cover that specific case.

wylieconlon · 2021-07-20T19:11:46Z

x-pack/plugins/lens/common/expressions/time_scale/time_scale.ts

+    // the datemath plugin always parses dates by using the current default moment time zone.
+    // to use the configured time zone, we are switching just for the bounds calculation.
+    const defaultTimezone = moment().zoneName();
+    moment.tz.setDefault(timeInfo.timeZone);


I'm not sure how this will be handled, I think it will require discussion with the app services & alerting teams.

wylieconlon · 2021-07-20T19:14:06Z

x-pack/plugins/lens/common/expressions/time_scale/time_scale.ts

+  d: 1000 * 60 * 60 * 24,
+};
+
+export const timeScale: ExpressionFunctionDefinition<


So you're no longer passing in the dataPluginPublicStart object which was previously used to get the auto interval, you're directly importing it?

Correct. I had a look at the code and assumed there were no strict dependencies in the used function with its context, and that the function could have been used independently.

dej611 · 2021-07-21T15:35:19Z

Talked offline with @ppisljar and sorted out a roadmap for this:

type: "render" function should be in common as well
- as we register also the render functions, perhaps there's no longer the need to split the datatable render function any more
All functions in common will be registered on the server side (follow up PR)
- The timezone problem would be solved by either
  - add timezone as an argument to the function, add a function that gets users timezone from browser or from setting. on server we fallback to setting
  - add timezone to context, expression service can default to browser or uisetting if not provided
Later on some work will be required as the kibana-app-services team will provide the right inspector adapter on the server
- probably will require to add one/few lines in the render functions on our side
- First preview of the adapter => adds dimensionName to datatable column meta information #106514

So I'm proceeding in this PR with 1 and create a follow up PR with 2.

dej611 · 2021-07-21T16:54:10Z

Moved the render functions back, so step 1 completed.

dej611 · 2021-07-22T08:10:31Z

@elasticmachine merge upstream

dej611 · 2021-07-22T15:47:39Z

@elasticmachine merge upstream

wylieconlon

Based on previous review and a new pass this LGTM. Did not run the code, just read through it.

mbondyra · 2021-07-26T07:13:21Z

@elasticmachine merge upstream

mbondyra

The code LGTM, I tested briefly in Chrome and didn't notice anything suspicious.

kibanamachine · 2021-07-26T09:31:44Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	704	730	+26

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
`lens`	169	190	+21

Async chunks

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

id	before	after	diff
`lens`	1.5MB	1.5MB	+23.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`lens`	14	22	+8

Unknown metric groups

API count

id	before	after	diff
`lens`	185	206	+21

References to deprecated APIs

id	before	after	diff
`lens`	168	160	-8

History

💚 Build #140426 succeeded 968046b
💔 Build #140418 failed 4c18322
💚 Build #140256 succeeded 38c4ee3
💔 Build #140115 failed 8567922
💚 Build #140000 succeeded 2bde1e2

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

ppisljar

code LGTM

* 🚚 First move batch to common * 🚚 Second batch of move * 🏷️ Import types only * 🚚 Third batch * 🚚 Fourth batch move * 🚚 Another module moved * 🚚 More function moved * 🚚 Last bit of move * ⚡ Reduce page load bundle size * 🐛 Fix import issue * 🐛 More import fix * ✨ Registered functions on the server * 🐛 Expose datatable_column as well * ✅ Add server side expression test * 🚚 Moved back render functions to public * ✨ Add a timezone arg to time_scale * 🔥 Remove timezone arg * 🔥 Remove server side code for now * 👌 Integrated feedback * 🚚 Move back datatable render function * 🏷️ Fix imports * 🏷️ Fix missing export * 🚚 Move render functions back! Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-07-26T13:29:20Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* 🚚 First move batch to common * 🚚 Second batch of move * 🏷️ Import types only * 🚚 Third batch * 🚚 Fourth batch move * 🚚 Another module moved * 🚚 More function moved * 🚚 Last bit of move * ⚡ Reduce page load bundle size * 🐛 Fix import issue * 🐛 More import fix * ✨ Registered functions on the server * 🐛 Expose datatable_column as well * ✅ Add server side expression test * 🚚 Moved back render functions to public * ✨ Add a timezone arg to time_scale * 🔥 Remove timezone arg * 🔥 Remove server side code for now * 👌 Integrated feedback * 🚚 Move back datatable render function * 🏷️ Fix imports * 🏷️ Fix missing export * 🚚 Move render functions back! Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

* 🚚 First move batch to common * 🚚 Second batch of move * 🏷️ Import types only * 🚚 Third batch * 🚚 Fourth batch move * 🚚 Another module moved * 🚚 More function moved * 🚚 Last bit of move * ⚡ Reduce page load bundle size * 🐛 Fix import issue * 🐛 More import fix * ✨ Registered functions on the server * 🐛 Expose datatable_column as well * ✅ Add server side expression test * 🚚 Moved back render functions to public * ✨ Add a timezone arg to time_scale * 🔥 Remove timezone arg * 🔥 Remove server side code for now * 👌 Integrated feedback * 🚚 Move back datatable render function * 🏷️ Fix imports * 🏷️ Fix missing export * 🚚 Move render functions back! Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

🚚 First move batch to common

ee6f0a4

dej611 added the Feature:Lens label Jul 13, 2021

dej611 added 12 commits July 13, 2021 19:16

🚚 Second batch of move

21cd2c1

🏷️ Import types only

f531458

🚚 Third batch

0a49de4

🚚 Fourth batch move

b117156

🚚 Another module moved

07cb0a8

🚚 More function moved

8e9bc88

🚚 Last bit of move

f160766

⚡ Reduce page load bundle size

3585332

🐛 Fix import issue

ecab4e0

🐛 More import fix

a8e9a6d

✨ Registered functions on the server

fa843d0

🐛 Expose datatable_column as well

d59369b

dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.15.0 v8.0.0 labels Jul 16, 2021

dej611 marked this pull request as ready for review July 16, 2021 17:34

dej611 requested a review from a team July 16, 2021 17:34

Merge branch 'master' into lens/97134

3999d24

dej611 added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes labels Jul 16, 2021

kibanamachine and others added 3 commits July 19, 2021 04:32

Merge branch 'master' into lens/97134

c2b7568

✅ Add server side expression test

84184a2

Merge branch 'lens/97134' of github.com:dej611/kibana into lens/97134

dd6eb50

dej611 mentioned this pull request Jul 19, 2021

[Lens] Split datatable renderer function into processing + renderer #106123

Closed

9 tasks

wylieconlon reviewed Jul 19, 2021

View reviewed changes

dej611 mentioned this pull request Jul 20, 2021

Register expression functions to server dej611/kibana#10

Closed

7 tasks

wylieconlon reviewed Jul 20, 2021

View reviewed changes

dej611 added 6 commits July 21, 2021 10:36

👌 Integrated feedback

bed9ce3

🚚 Move back datatable render function

bb167ce

🏷️ Fix imports

59f38f3

Merge remote-tracking branch upstream/master into lens/97134

ff60635

Merge remote-tracking branch 'upstream/master' into lens/97134

2d419b8

🏷️ Fix missing export

2bde1e2

🚚 Move render functions back!

8567922

dej611 requested a review from ppisljar July 21, 2021 16:54

kibanamachine and others added 3 commits July 22, 2021 04:10

Merge branch 'master' into lens/97134

38c4ee3

Merge remote-tracking branch 'upstream/master' into lens/97134

2f17aea

Merge branch 'lens/97134' of github.com:dej611/kibana into lens/97134

4c18322

Merge branch 'master' into lens/97134

968046b

wylieconlon approved these changes Jul 22, 2021

View reviewed changes

Merge branch 'master' into lens/97134

c2b9d00

mbondyra approved these changes Jul 26, 2021

View reviewed changes

ppisljar approved these changes Jul 26, 2021

View reviewed changes

dej611 merged commit 3c6b854 into elastic:master Jul 26, 2021

kibanamachine mentioned this pull request Jul 26, 2021

[7.x] [Lens] Move Lens functions to common (#105455) #106719

Merged

		sortingColumnId: string \| undefined;
		sortingDirection: 'asc' \| 'desc' \| 'none';

Conversation

dej611 commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jul 16, 2021

Uh oh!

dej611 commented Jul 16, 2021

Uh oh!

dej611 commented Jul 19, 2021

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dej611 commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dej611 commented Jul 21, 2021

Uh oh!

dej611 commented Jul 22, 2021

Uh oh!

dej611 commented Jul 22, 2021

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

mbondyra commented Jul 26, 2021

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 26, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

API count

References to deprecated APIs

History

dej611 commented Jul 13, 2021 •

edited

Loading

dej611 commented Jul 21, 2021 •

edited

Loading