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

[Canvas] Adds support for palette in embeddable function #115278

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Oct 15, 2021

Summary

This adds the palette argument to the embeddable function. This argument is only compatible with lens type embeddables, but I added it to maintain feature parity with the existing savedLens function.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@cqliu1 cqliu1 added Feature:Canvas Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Oct 15, 2021
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from 40ebca5 to 4671abc Compare October 18, 2021 16:24
@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from 6b2bc48 to 196b94d Compare October 18, 2021 17:08
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from 4671abc to dca603d Compare October 19, 2021 15:59
@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from 7557017 to f8dbcaf Compare October 19, 2021 16:00
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from dff15e3 to aec465c Compare October 19, 2021 21:45
@cqliu1 cqliu1 marked this pull request as ready for review October 19, 2021 21:46
@cqliu1 cqliu1 requested a review from a team as a code owner October 19, 2021 21:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from f8dbcaf to 34831c5 Compare October 19, 2021 22:18
@cqliu1 cqliu1 requested review from a team as code owners October 19, 2021 22:18
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from c414d23 to 214ba29 Compare October 19, 2021 22:19
@lukeelmers lukeelmers removed request for a team October 19, 2021 22:46
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 19, 2021

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/jobs/categorization_field_examples·ts.apis Machine Learning jobs Categorization example endpoint - partially valid, more than 75% are null

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]     │
[00:00:00]       └-: apis
[00:00:00]         └-> "before all" hook in "apis"
[00:09:56]         └-: Machine Learning
[00:09:56]           └-> "before all" hook in "Machine Learning"
[00:09:56]           └-> "before all" hook in "Machine Learning"
[00:09:56]             │ debg creating role ft_ml_source
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_ml_source]
[00:09:56]             │ debg creating role ft_ml_source_readonly
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_ml_source_readonly]
[00:09:56]             │ debg creating role ft_ml_dest
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_ml_dest]
[00:09:56]             │ debg creating role ft_ml_dest_readonly
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_ml_dest_readonly]
[00:09:56]             │ debg creating role ft_ml_ui_extras
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_ml_ui_extras]
[00:09:56]             │ debg creating role ft_default_space_ml_all
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_default_space_ml_all]
[00:09:56]             │ debg creating role ft_default_space1_ml_all
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_default_space1_ml_all]
[00:09:56]             │ debg creating role ft_all_spaces_ml_all
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_all_spaces_ml_all]
[00:09:56]             │ debg creating role ft_default_space_ml_read
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_default_space_ml_read]
[00:09:56]             │ debg creating role ft_default_space1_ml_read
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_default_space1_ml_read]
[00:09:56]             │ debg creating role ft_all_spaces_ml_read
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_all_spaces_ml_read]
[00:09:56]             │ debg creating role ft_default_space_ml_none
[00:09:56]             │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [ft_default_space_ml_none]
[00:09:56]             │ debg creating user ft_ml_poweruser
[00:09:56]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_poweruser]
[00:09:56]             │ debg created user ft_ml_poweruser
[00:09:56]             │ debg creating user ft_ml_poweruser_spaces
[00:09:56]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_poweruser_spaces]
[00:09:56]             │ debg created user ft_ml_poweruser_spaces
[00:09:56]             │ debg creating user ft_ml_poweruser_space1
[00:09:56]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_poweruser_space1]
[00:09:56]             │ debg created user ft_ml_poweruser_space1
[00:09:56]             │ debg creating user ft_ml_poweruser_all_spaces
[00:09:56]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_poweruser_all_spaces]
[00:09:56]             │ debg created user ft_ml_poweruser_all_spaces
[00:09:56]             │ debg creating user ft_ml_viewer
[00:09:57]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_viewer]
[00:09:57]             │ debg created user ft_ml_viewer
[00:09:57]             │ debg creating user ft_ml_viewer_spaces
[00:09:57]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_viewer_spaces]
[00:09:57]             │ debg created user ft_ml_viewer_spaces
[00:09:57]             │ debg creating user ft_ml_viewer_space1
[00:09:57]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_viewer_space1]
[00:09:57]             │ debg created user ft_ml_viewer_space1
[00:09:57]             │ debg creating user ft_ml_viewer_all_spaces
[00:09:57]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_viewer_all_spaces]
[00:09:57]             │ debg created user ft_ml_viewer_all_spaces
[00:09:57]             │ debg creating user ft_ml_unauthorized
[00:09:57]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_unauthorized]
[00:09:57]             │ debg created user ft_ml_unauthorized
[00:09:57]             │ debg creating user ft_ml_unauthorized_spaces
[00:09:57]             │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [ft_ml_unauthorized_spaces]
[00:09:57]             │ debg created user ft_ml_unauthorized_spaces
[00:14:07]           └-: jobs
[00:14:07]             └-> "before all" hook in "jobs"
[00:14:07]             └-: Categorization example endpoint - 
[00:14:07]               └-> "before all" hook for "valid with good number of tokens"
[00:14:07]               └-> "before all" hook for "valid with good number of tokens"
[00:14:07]                 │ info [x-pack/test/functional/es_archives/ml/categorization] Loading "mappings.json"
[00:14:07]                 │ info [x-pack/test/functional/es_archives/ml/categorization] Loading "data.json.gz"
[00:14:07]                 │ info [o.e.c.m.MetadataCreateIndexService] [node-01] [ft_categorization] creating index, cause [api], templates [], shards [1]/[0]
[00:14:07]                 │ info [x-pack/test/functional/es_archives/ml/categorization] Created index "ft_categorization"
[00:14:07]                 │ debg [x-pack/test/functional/es_archives/ml/categorization] "ft_categorization" settings {"index":{"number_of_replicas":"0","number_of_shards":"1"}}
[00:14:08]                 │ info [x-pack/test/functional/es_archives/ml/categorization] Indexed 1501 docs into "ft_categorization"
[00:14:08]                 │ debg applying update to kibana config: {"dateFormat:tz":"UTC"}
[00:14:09]               └-> valid with good number of tokens
[00:14:09]                 └-> "before each" hook: global before each for "valid with good number of tokens"
[00:14:09]                 └- ✓ pass  (152ms)
[00:14:09]               └-> invalid, too many tokens.
[00:14:09]                 └-> "before each" hook: global before each for "invalid, too many tokens."
[00:14:10]                 │ info [r.suppressed] [node-01] path: /_analyze, params: {}
[00:14:10]                 │      org.elasticsearch.transport.RemoteTransportException: [node-01][127.0.0.1:63101][indices:admin/analyze[s]]
[00:14:10]                 │      Caused by: java.lang.IllegalStateException: The number of tokens produced by calling _analyze has exceeded the allowed maximum of [10000]. This limit can be set by changing the [index.analyze.max_token_count] index level setting.
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction$TokenCounter.increment(TransportAnalyzeAction.java:397) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.simpleAnalyze(TransportAnalyzeAction.java:229) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.analyze(TransportAnalyzeAction.java:204) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.analyze(TransportAnalyzeAction.java:122) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.shardOperation(TransportAnalyzeAction.java:110) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.shardOperation(TransportAnalyzeAction.java:62) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.support.single.shard.TransportSingleShardAction.lambda$asyncShardOperation$0(TransportSingleShardAction.java:99) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.ActionRunnable.lambda$supply$0(ActionRunnable.java:47) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.ActionRunnable$2.doRun(ActionRunnable.java:62) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:737) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) [?:?]
[00:14:10]                 │      	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]
[00:14:10]                 │      	at java.lang.Thread.run(Thread.java:833) [?:?]
[00:14:10]                 │ info [r.suppressed] [node-01] path: /_analyze, params: {}
[00:14:10]                 │      org.elasticsearch.transport.RemoteTransportException: [node-01][127.0.0.1:63101][indices:admin/analyze[s]]
[00:14:10]                 │      Caused by: java.lang.IllegalStateException: The number of tokens produced by calling _analyze has exceeded the allowed maximum of [10000]. This limit can be set by changing the [index.analyze.max_token_count] index level setting.
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction$TokenCounter.increment(TransportAnalyzeAction.java:397) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.simpleAnalyze(TransportAnalyzeAction.java:229) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.analyze(TransportAnalyzeAction.java:204) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.analyze(TransportAnalyzeAction.java:122) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.shardOperation(TransportAnalyzeAction.java:110) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.admin.indices.analyze.TransportAnalyzeAction.shardOperation(TransportAnalyzeAction.java:62) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.support.single.shard.TransportSingleShardAction.lambda$asyncShardOperation$0(TransportSingleShardAction.java:99) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.ActionRunnable.lambda$supply$0(ActionRunnable.java:47) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.action.ActionRunnable$2.doRun(ActionRunnable.java:62) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:737) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[00:14:10]                 │      	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) [?:?]
[00:14:10]                 │      	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]
[00:14:10]                 │      	at java.lang.Thread.run(Thread.java:833) [?:?]
[00:14:10]                 └- ✓ pass  (314ms)
[00:14:10]               └-> partially valid, more than 75% are null
[00:14:10]                 └-> "before each" hook: global before each for "partially valid, more than 75% are null"
[00:14:10]                 └- ✖ fail: apis Machine Learning jobs Categorization example endpoint -  partially valid, more than 75% are null
[00:14:10]                 │       Error: expected 249 to sort of equal 250
[00:14:10]                 │       + expected - actual
[00:14:10]                 │ 
[00:14:10]                 │       -249
[00:14:10]                 │       +250
[00:14:10]                 │       
[00:14:10]                 │       at Assertion.assert (/dev/shm/workspace/parallel/10/kibana/node_modules/@kbn/expect/expect.js:100:11)
[00:14:10]                 │       at Assertion.eql (/dev/shm/workspace/parallel/10/kibana/node_modules/@kbn/expect/expect.js:244:8)
[00:14:10]                 │       at Context.<anonymous> (test/api_integration/apis/ml/jobs/categorization_field_examples.ts:303:36)
[00:14:10]                 │       at runMicrotasks (<anonymous>)
[00:14:10]                 │       at processTicksAndRejections (node:internal/process/task_queues:96:5)
[00:14:10]                 │       at Object.apply (/dev/shm/workspace/parallel/10/kibana/node_modules/@kbn/test/target_node/functional_test_runner/lib/mocha/wrap_function.js:87:16)
[00:14:10]                 │ 
[00:14:10]                 │ 

Stack Trace

Error: expected 249 to sort of equal 250
    at Assertion.assert (/dev/shm/workspace/parallel/10/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/10/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/api_integration/apis/ml/jobs/categorization_field_examples.ts:303:36)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at Object.apply (/dev/shm/workspace/parallel/10/kibana/node_modules/@kbn/test/target_node/functional_test_runner/lib/mocha/wrap_function.js:87:16) {
  actual: '249',
  expected: '250',
  showDiff: true
}

History

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

@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from 34831c5 to 6a29232 Compare October 20, 2021 18:58
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from 214ba29 to 0047fc9 Compare October 20, 2021 19:00
@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from 6a29232 to 8ac6d41 Compare October 20, 2021 22:45
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from c1d63c3 to 294e377 Compare October 21, 2021 01:42
Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Code LGTM (wasn't able to test it locally cuz my environment is a little messed up at the moment)

@cqliu1 cqliu1 added the stalled label Oct 21, 2021
@cqliu1
Copy link
Contributor Author

cqliu1 commented Oct 21, 2021

I came across an issue where the palette arg gets removed when you edit a Lens embeddable and come back to the workpad. This is because we rewrite the entire embeddable expression when returning to Canvas with an incoming embeddable via the embeddable state transfer service.

While trying to fix the bug, I wasn't sure which palette should take priority: the palette configured via the Lens editor or the palette provided via the expression in Canvas.

I'm punting on this for now. We can revisit this as a future enhancement, but for now users can configure their Lens color palettes by editing their visualization in the Lens editor.

@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from c637735 to 780f71c Compare October 25, 2021 07:46
@cqliu1 cqliu1 requested a review from a team as a code owner October 25, 2021 07:46
@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch from 1834014 to 151ceac Compare October 25, 2021 20:02
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch 2 times, most recently from e0d2b2e to 3f005a8 Compare October 26, 2021 15:37
@cqliu1 cqliu1 mentioned this pull request Oct 26, 2021
11 tasks
@cqliu1 cqliu1 force-pushed the canvas/by-value-embeddables branch 2 times, most recently from 46edbbe to dc6a861 Compare October 26, 2021 16:23
@cqliu1 cqliu1 force-pushed the canvas/support-embeddable-palette branch from 3f005a8 to 3abb10f Compare October 26, 2021 16:26
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 26, 2021

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [dc6a861]

History

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

@cqliu1 cqliu1 deleted the branch elastic:canvas/by-value-embeddables October 27, 2021 18:58
@cqliu1 cqliu1 closed this Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Canvas stalled Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants