[Lens] Register all expression functions to the server #107836

alexwizp · 2021-08-06T15:50:33Z

Part of: #97134

Summary

What was done in that PR:

List of functions which were registered to the server:
- lens_time_scale
- lens_counter_rate
- lens_rename_columns
- lens_format_column
- lens_datatable_column
- lens_datatable (relies on field formatter service)
- lens_metric_chart
- lens_pie
- lens_xy_legendConfig
- lens_xy_yConfig
- lens_xy_tickLabelsConfig
- lens_xy_gridlinesConfig
- lens_xy_axisTitlesVisibilityConfig
- lens_xy_layer
- lens_xy_chart
removed deprecation usage of data.fieldsFormats service. This service was moved into separate plugin which should be used independently of data plugin.
formatFactory method was refactored to avoid using of the following code:
const formatFactory = await dependencies.formatFactory;

Part of: elastic#97134

elasticmachine · 2021-08-09T07:51:14Z

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

dej611 · 2021-08-09T09:20:17Z

Had a first code review of this PR. While 1 and 2 seems ok, the 3rd point is what makes up the most of the file count.
I am not 100% sure about the changes in the formatFactory scope for this PR. Probably it would be better to address it in another PR so we could better discuss about the changes implications? What do you think?

alexwizp · 2021-08-09T09:42:53Z

x-pack/plugins/lens/public/plugin.ts

    const getStartServices = async (): Promise<LensEmbeddableStartServices> => {
      const [coreStart, deps] = await core.getStartServices();
-      this.initParts(core, data, embeddable, charts, expressions, usageCollection);


Just want to clarify is it expected that we execute initParts without await?

alexwizp · 2021-08-09T09:47:54Z

the 3rd point is what makes up the most of the file count.

@dej611 This fix allow me to simplify typing, cause now for all cases it's not a promise. Plus in all cases where we execute initParts we already have all needed start services to remove

But if you insist we can probably move that into a separate PR. But from my point of view all of this related... Again I'm not insist.

dej611 · 2021-08-09T10:08:42Z

Maybe I would ask @ppisljar to check as well here.

ppisljar · 2021-08-09T13:05:39Z

x-pack/plugins/lens/public/indexpattern_datasource/index.ts

+      expressions,
+      editorFrame,
+      charts,
+    }: IndexPatternDatasourceSetupPlugins
  ) {
    editorFrame.registerDatasource(async () => {
      const {


registration of functions on the client seems to happen late (after setup lifecycle)

this should be fixed (not necesarry as part of this PR) as we plan to introduce checks in expression service that will prevent this.

yes you are right, I also wanted to highlight that in my PR, but then decided that it's not related.... In Lens we have some setup methods which executed on start phase =(

Yes this is important, but we will take care of it in another PR

ppisljar

code LGTM

VladLasitsa

code LGTM!

alexwizp · 2021-08-10T11:58:15Z

@elasticmachine merge upstream

kibanamachine · 2021-08-10T14:19:07Z

💚 Build Succeeded

Metrics [docs]

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.6MB	1.6MB	+118.0B

Page load bundle

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

id	before	after	diff
`lens`	29.0KB	29.1KB	+133.0B

Unknown metric groups

API count

id	before	after	diff
`lens`	208	209	+1

API count missing comments

id	before	after	diff
`lens`	192	193	+1

History

💚 Build #143577 succeeded c3caa8d
💚 Build #143482 succeeded acc17df

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

cc @alexwizp

stratoula

Code LGTM, I can't find any regression caused by these changes. I tested it locally in safari. If @mbondyra is also fine, let's merge it

mbondyra

Tested and couldn't break it. Code LGTM 🆗

Part of: elastic#97134 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…8175) Part of: #97134 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

elastic deleted a comment from kibanamachine Aug 6, 2021

alexwizp force-pushed the 97134 branch from 42ec41a to acc17df Compare August 6, 2021 19:08

[Lens] Register all expression functions to the server

c3caa8d

Part of: elastic#97134

alexwizp force-pushed the 97134 branch from acc17df to c3caa8d Compare August 9, 2021 07:43

alexwizp requested review from dej611 and stratoula August 9, 2021 07:50

alexwizp marked this pull request as ready for review August 9, 2021 07:50

alexwizp requested a review from a team August 9, 2021 07:50

alexwizp added this to In progress in Lens via automation Aug 9, 2021

alexwizp added v7.15.0 v8.0.0 Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Aug 9, 2021

alexwizp added the release_note:skip Skip the PR/issue when compiling release notes label Aug 9, 2021

alexwizp self-assigned this Aug 9, 2021

alexwizp requested review from mbondyra and VladLasitsa August 9, 2021 08:29

alexwizp commented Aug 9, 2021

View reviewed changes

ppisljar reviewed Aug 9, 2021

View reviewed changes

ppisljar approved these changes Aug 9, 2021

View reviewed changes

VladLasitsa approved these changes Aug 9, 2021

View reviewed changes

Merge branch 'master' into 97134

2df5947

stratoula approved these changes Aug 11, 2021

View reviewed changes

mbondyra approved these changes Aug 11, 2021

View reviewed changes

alexwizp merged commit 82bec98 into elastic:master Aug 11, 2021

Lens automation moved this from In progress to Done Aug 11, 2021

alexwizp added a commit to alexwizp/kibana that referenced this pull request Aug 11, 2021

[Lens] Register all expression functions to the server (elastic#107836)

2a162d5

Part of: elastic#97134 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

alexwizp mentioned this pull request Aug 11, 2021

[7.x] [Lens] Register all expression functions to the server (#107836) #108175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Register all expression functions to the server #107836

[Lens] Register all expression functions to the server #107836

alexwizp commented Aug 6, 2021 •

edited

Loading

elasticmachine commented Aug 9, 2021

dej611 commented Aug 9, 2021

alexwizp Aug 9, 2021

alexwizp commented Aug 9, 2021

dej611 commented Aug 9, 2021

ppisljar Aug 9, 2021

alexwizp Aug 9, 2021

stratoula Aug 11, 2021

ppisljar left a comment

VladLasitsa left a comment

alexwizp commented Aug 10, 2021

kibanamachine commented Aug 10, 2021

API count

API count missing comments

stratoula left a comment

mbondyra left a comment

[Lens] Register all expression functions to the server #107836

[Lens] Register all expression functions to the server #107836

Conversation

alexwizp commented Aug 6, 2021 • edited Loading

Summary

elasticmachine commented Aug 9, 2021

dej611 commented Aug 9, 2021

alexwizp Aug 9, 2021

Choose a reason for hiding this comment

alexwizp commented Aug 9, 2021

dej611 commented Aug 9, 2021

ppisljar Aug 9, 2021

Choose a reason for hiding this comment

alexwizp Aug 9, 2021

Choose a reason for hiding this comment

stratoula Aug 11, 2021

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

VladLasitsa left a comment

Choose a reason for hiding this comment

alexwizp commented Aug 10, 2021

kibanamachine commented Aug 10, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

API count

API count missing comments

History

stratoula left a comment

Choose a reason for hiding this comment

mbondyra left a comment

Choose a reason for hiding this comment

alexwizp commented Aug 6, 2021 •

edited

Loading