Move Lens attribute builder to a package #163422

crespocarlos · 2023-08-08T14:59:32Z

Summary

This PR creates a new package that contains a utility API that helps to generate the JSON with the attributes required to render a Lens chart with the EmbeddableComponent.

apmmachine · 2023-08-08T14:59:45Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…fix'

…builder-to-package

crespocarlos · 2023-08-09T09:41:09Z

@elasticmachine merge upstream

…-fix'

elasticmachine · 2023-08-09T13:33:28Z

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

elasticmachine · 2023-08-09T13:33:30Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

dgieselaar

Thanks a ton!

dej611

Had a quick look at the API described in the Readme.md for both chart and attributes builder, and I was wondering whether it make sense to move the formulaAPI to the main level, instead of the layer level:

const metricChart = new MetricChart({
  layers: new MetricLayer({
    data: {
      label: 'Disk Read Throughput',
      value: "counter_rate(max(system.diskio.read.count), kql='system.diskio.read.count: *')",
      format: {
        id: 'bytes',
        params: {
          decimals: 1,
        },
      },
    },
  }),
  formulaAPI,
  dataView,
});

or

const builder = new LensAttributesBuilder({
  visualization: new MetricChart({
    layers: new MetricLayer({
      data: {
        label: 'Disk Read Throughput',
        value: "counter_rate(max(system.diskio.read.count), kql='system.diskio.read.count: *')",
        format: {
          id: 'bytes',
          params: {
            decimals: 1,
          },
        },
      },
    }),
    dataView,
  }),
  formulaAPI,
});

const lensAttributes = builder.build();

<EmbeddableComponent
  attributes={lensAttributes}
  viewMode={ViewMode.VIEW}
  ...
/>

that would make the layer API a little bit less verbose and handles the "formula" complexity internally without have the consumer to think about what type of column it has passed and which dependency it might require.

…fix'

jennypavlova

LGTM 💯 (Infra changes)

dej611

I would propose few changes here before proceed.
Not too many, and I've already a branch with all of them implemented as reference if helpful:

Add a note at the beginning of the README to make it clear that it's still a work in progress and only XY and Metric charts are (partially) covered.
for any *Layer configuration object I would add a type prop, which contains the type of dimension operation. Currently it will only accept "formula".
Make the default date_histogram for XY become optional and add a new buckets property to the XYLayerOptions type which will support all 4 bucketed column types in Lens.
- the XY layer series color wasn't working for me, but it should
- the Metric series color wasn't working for me, but I think it should and backgroundColor could be removed instead
- the FormulaConfig['format'] property should not be mandatory

--- end of required changes ---

Some nice to have (but not blocking changes):

each layer can have a dataView prop to override the "chart" one
both buckets and breakdown configurations could accept (optional) params as defined by its respective Lens column definition (useful to tweak some configuration) and getHistogramColumn and getTopValuesColumn should handle that.
not sure why line has been chosen as default XY series type (in Lens is the bar_stacked) here. Is there a strong argument to change it?
why the LensAttributesBuilder accepts an object with only a single visualization prop in it? Is it designed to support more arguments there?

I've implemented 90% of both lists in this branch already if you want to use as reference: https://github.com/crespocarlos/kibana/compare/move-attribute-builder-to-package...dej611:kibana:review/163422?expand=1
I've tested this PR against the Lens embeddable example and with these changes it could replace most of custom code there.

crespocarlos · 2023-08-11T14:15:22Z

Thanks for your review @dej611 and for the changes made in your branch - it will save a lot of time

Add a note at the beginning of the README to make it clear that it's still a work in progress and only XY and Metric charts are (partially) covered.

OK!

for any *Layer configuration object I would add a type prop, which contains the type of dimension operation. Currently it will only accept "formula".

Could you please elaborate? Can a layer be anything other than a formula?

Make the default date_histogram for XY become optional and add a new buckets property to the XYLayerOptions type which will support all 4 bucketed column types in Lens.

the Metric series color wasn't working for me, but I think it should and backgroundColor could be removed instead

the FormulaConfig['format'] property should not be mandatory

Will check the above.

--- end of required changes ---

each layer can have a dataView prop to override the "chart" one

both buckets and breakdown configurations could accept (optional) params as defined by its respective Lens column definition (useful to tweak some configuration) and getHistogramColumn and getTopValuesColumn should handle that.

Should we concern about number of available configuration options that params provides?

not sure why line has been chosen as default XY series type (in Lens is the bar_stacked) here. Is there a strong argument to change it?

That's because it's what we're using in infra-ui. No strong reasons.

why the LensAttributesBuilder accepts an object with only a single visualization prop in it? Is it designed to support more arguments there?

I had other ideas of parameters for the LensAttributesBuilder. It ended up being a single prop. It can be changed.

dej611 · 2023-08-11T14:19:28Z

for any *Layer configuration object I would add a type prop, which contains the type of dimension operation. Currently it will only accept "formula".

Could you please elaborate? Can a layer be anything other than a formula?

It can, but in the current implementation only Formula metrics are supported. Adding a tagged prop type: 'formula' to it will make it easier to extend it to support for other operations, like other Quick function such Count, Unique count, Last value, etc... in the future without breaking existing configurations.

To be clear I'm talking about this:

new MetricLayer({
    data: {
      type: 'formula', // <= this tagged property here
      label: 'Disk Read Throughput',
      value: "counter_rate(max(system.diskio.read.count), kql='system.diskio.read.count: *')",
      format: {
        id: 'bytes',
        params: {
          decimals: 1,
        },
      },
    },
  }),

why the LensAttributesBuilder accepts an object with only a single visualization prop in it? Is it designed to support more arguments there?

I had other ideas of parameters for the LensAttributesBuilder. It ended up being a single prop. It can be changed.

I'm not necessarily against that, instead quite open to ideas here.

Both buckets and breakdown configurations could accept (optional) params as defined by its respective Lens column definition (useful to tweak some configuration) and getHistogramColumn and getTopValuesColumn should handle that.

Should we concern about number of available configuration options that params provides?

I would say no as usually the params object contains dedicated configuration options for the column definition rather than implementation/private one. I think let the user have full control of that at will can be quite powerful. Perhaps we could gather some feedback in v.1 here and decide for something else for v.2 .

dgieselaar · 2023-08-11T17:54:44Z

@crespocarlos @dej611 My 2 cents: let's keep it opinionated. If Marco doesn't feel comfortable with this API we can also clearly mark it as an infra/Observability thing. I think we should specifically avoid making this generic, it defeats the purpose.

…fix'

…builder-to-package

stratoula · 2023-08-14T06:21:31Z

As this is important to be merged today as it is blocking other PR to be merged before the FF let's do the following and merge:

dont make the changes that Marco proposes
move the ownership to infra/Observability
make it explicit in the Readme that this is only used for infra/Observability

We are going to work on our own generic api in a next minor

crespocarlos · 2023-08-14T07:35:03Z

@crespocarlos @dej611 My 2 cents: let's keep it opinionated. If Marco doesn't feel comfortable with this API we can also clearly mark it as an infra/Observability thing. I think we should specifically avoid making this generic, it defeats the purpose.

Agree with it being somewhat opinionated. There are many different parameters that can be used and I would be ok with the API making some decisions. It does still leave the door open for customizations.

As this is important to be merged today as it is blocking other PR to be merged before the FF let's do the following and merge:

I have already implemented the changes. The only thing that needs to be reverted is the example app. I'll update the readme.

…builder-to-package

crespocarlos · 2023-08-14T07:59:10Z

@stratoula, qq:

We are going to work on our own generic API in a next minor

Is it possible that when your solution is done, it can replace this one? In the end, I think it's beneficial that we all use a single solution to solve this problem.

…fix'

stratoula · 2023-08-14T08:02:19Z

@crespocarlos this is the goal yes! We are just not sure at this point if the same api can be used, we might create a new one (at the same package), move the ownership to us, and change the consumers of the existing api to use the new one.

kibana-ci · 2023-08-14T09:45:38Z

💚 Build Succeeded

Buildkite Build
Commit: dc59f50

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	1391	1393	+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
`@kbn/lens-embeddable-utils`	-	147	+147

Async chunks

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

id	before	after	diff
`infra`	2.0MB	2.0MB	+1.9KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/lens-embeddable-utils`	-	147	+147

History

💔 Build #149945 failed d57fc31
💚 Build #149879 succeeded 1cbc90a
💚 Build #149861 succeeded 1d25ca7
💔 Build #149818 failed fdcf3e4
💔 Build #149809 failed 6fab067

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

Move Lens attribute builder to a package

a6c664b

crespocarlos force-pushed the move-attribute-builder-to-package branch from 911a950 to a6c664b Compare August 8, 2023 15:00

kibanamachine and others added 4 commits August 8, 2023 15:07

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

fbedfe5

…fix'

Replace imports with new package

aa9ca69

README.md

4be9735

Merge branch 'main' of github.com:elastic/kibana into move-attribute-…

0882949

…builder-to-package

kibanamachine and others added 4 commits August 9, 2023 05:41

Merge branch 'main' into move-attribute-builder-to-package

204eeb0

Fix imports

9106c0c

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

1228e4b

…-fix'

Fix tsconfig.base.json

599939f

crespocarlos marked this pull request as ready for review August 9, 2023 13:33

crespocarlos requested a review from a team as a code owner August 9, 2023 13:33

crespocarlos requested review from a team and dgieselaar August 9, 2023 13:33

crespocarlos marked this pull request as draft August 9, 2023 13:34

crespocarlos marked this pull request as ready for review August 9, 2023 13:42

dgieselaar approved these changes Aug 9, 2023

View reviewed changes

dej611 reviewed Aug 9, 2023

View reviewed changes

crespocarlos and others added 2 commits August 9, 2023 16:24

Rename package

dfce17b

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6a3821b

…fix'

crespocarlos added 2 commits August 10, 2023 10:00

Moving README.md

c608aea

Moving README.md

1aaf702

jennypavlova self-requested a review August 10, 2023 15:32

jennypavlova approved these changes Aug 10, 2023

View reviewed changes

dej611 requested changes Aug 11, 2023

View reviewed changes

crespocarlos and others added 3 commits August 11, 2023 21:50

Implement proposed changes

8855012

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

37bf27e

…fix'

Merge branch 'main' of github.com:elastic/kibana into move-attribute-…

6fab067

…builder-to-package

crespocarlos requested a review from dej611 August 11, 2023 20:29

crespocarlos added 3 commits August 11, 2023 23:06

Add Layer dataView support; fix build

fdcf3e4

Fix functional test

1d25ca7

date_histogram bucket type as default

1cbc90a

dgieselaar mentioned this pull request Aug 13, 2023

Lens function dgieselaar/kibana#42

Merged

crespocarlos added 2 commits August 14, 2023 09:37

Merge branch 'main' of github.com:elastic/kibana into move-attribute-…

182df2a

…builder-to-package

Reverting some changes; changing ownership

bcafce5

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

8ae9422

…fix'

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

d57fc31

dej611 approved these changes Aug 14, 2023

View reviewed changes

Fix test

dc59f50

crespocarlos merged commit 281cc22 into elastic:main Aug 14, 2023

crespocarlos deleted the move-attribute-builder-to-package branch August 14, 2023 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Lens attribute builder to a package #163422

Move Lens attribute builder to a package #163422

crespocarlos commented Aug 8, 2023 •

edited

Loading

apmmachine commented Aug 8, 2023

crespocarlos commented Aug 9, 2023

elasticmachine commented Aug 9, 2023

elasticmachine commented Aug 9, 2023

dgieselaar left a comment

dej611 left a comment

jennypavlova left a comment

dej611 left a comment

crespocarlos commented Aug 11, 2023

dej611 commented Aug 11, 2023 •

edited

Loading

dgieselaar commented Aug 11, 2023

stratoula commented Aug 14, 2023

crespocarlos commented Aug 14, 2023

crespocarlos commented Aug 14, 2023

stratoula commented Aug 14, 2023

kibana-ci commented Aug 14, 2023

API count

Move Lens attribute builder to a package #163422

Move Lens attribute builder to a package #163422

Conversation

crespocarlos commented Aug 8, 2023 • edited Loading

Summary

apmmachine commented Aug 8, 2023

🤖 GitHub comments

crespocarlos commented Aug 9, 2023

elasticmachine commented Aug 9, 2023

elasticmachine commented Aug 9, 2023

dgieselaar left a comment

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

jennypavlova left a comment

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

crespocarlos commented Aug 11, 2023

dej611 commented Aug 11, 2023 • edited Loading

dgieselaar commented Aug 11, 2023

stratoula commented Aug 14, 2023

crespocarlos commented Aug 14, 2023

crespocarlos commented Aug 14, 2023

stratoula commented Aug 14, 2023

kibana-ci commented Aug 14, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

History

crespocarlos commented Aug 8, 2023 •

edited

Loading

dej611 commented Aug 11, 2023 •

edited

Loading