[Expressions] Update expressions public API to expose partial results support #102403

dokmic · 2021-06-16T19:31:57Z

Summary

Updates expressions plugin to expose partial results support.
Resolves #98280.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

spalger · 2021-06-16T20:18:10Z

jenkins, test this

(restarting due to jenkins upgrade)

elasticmachine · 2021-06-21T09:15:34Z

Pinging @elastic/kibana-app-services (Team:AppServices)

ppisljar · 2021-06-21T09:29:02Z

src/plugins/expressions/public/loader.ts

-    this.data$.subscribe((data) => {
-      this.render(data);
+    this.data$.subscribe(({ result }) => {
+      this.render(result);


loader needs to receive an option if partial results should be rendered or not,

if option is false (default), we only render when partial===false

ppisljar · 2021-06-21T09:30:08Z

src/plugins/expressions/public/react_expression_renderer.tsx

@@ -30,7 +30,11 @@ export interface ReactExpressionRendererProps extends IExpressionLoaderParams {
  ) => React.ReactElement | React.ReactElement[];
  padding?: 'xs' | 's' | 'm' | 'l' | 'xl';
  onEvent?: (event: ExpressionRendererEvent) => void;
-  onData$?: <TData, TInspectorAdapters>(data: TData, adapters?: TInspectorAdapters) => void;
+  onData$?: <TData, TInspectorAdapters>(


we need a prop to allow opting in to partial result. we should pass that to loader then, if prop is not enabled partial data should not be rendered (only final results)

flash1293

I might be missing something but this looks like it's not possible to disable partial rendering when using the expression renderer component? As we want to provide indication for partial results in the rendered Lens chart, we need a way to selectively disable partial results.

Edit: What Peter said (#102403 (comment))

dokmic · 2021-06-21T17:54:16Z

@ppisljar @flash1293 I have added an option to disable partial results in the loader and the renderer. Could you please take another look?

wylieconlon

I'm reviewing this and seeing that I have two assumptions that are not fully met here:

Lens might conditionally render partial results, for example if there is a time series vs terms aggregation.
Lens might implement some kind of progress indicator as a percentage, like 50% loaded.

wylieconlon · 2021-06-21T20:04:58Z

src/plugins/expressions/common/execution/execution.ts

+  /**
+   * Partial result flag.
+   */
+  partial: boolean;


We need more than just a flag for partial data, we need some kind of status indicator as well. Typically I'd imagine this is the shard responses: 100 total shards, 5 successful, 50 skipped, 1 failure.

We cannot predict that because an expression function can emit an indefinite number of results. So the best you can do is to implement the progress indication in a function.

@dokmic

So the best you can do is to implement the progress indication in a function

Can you elaborate on how this would look like? I think this is an important part of supporting partial results.
I can imagine this information being part of the datatable (what do you think @ppisljar ?) but I think we need to find some kind of solution for it.

@dokmic Elasticsearch indicates the number of successful vs total shards on every partial status call, this is what I'm referring to: async search status

@flash1293 @wylieconlon this PR does not try to address progress indication apart from letting the consumer know when result is partial vs final. Progress reporting will be added at a later time, right now its not even supported on our low level search service.

@ppisljar Right, I understand that it's not supported yet. What I'm asking is that we implement an API that can support a progress indicator, not just a boolean.

we didn't want to do anything more for now as we don't know yet in what form progress from es will be provided to us or how we'd want to report this on expression level. in the future we are likely to keep this partial flag and add another progress variable.

wylieconlon · 2021-06-21T20:06:16Z

src/plugins/expressions/common/execution/execution.ts

+
+              subscriber.add(
+                source.subscribe({
+                  next: (result) => {


It seems like sources need to provide their own partial status indicator here, not just automatically setting it to partial: true.

That's being overwritten later on the same frame upon completion. So for the last result that will be true. You can also see that in the unit test.

flash1293 · 2021-06-22T07:23:36Z

@wylieconlon

Lens might conditionally render partial results, for example if there is a time series vs terms aggregation.

Maybe I'm missing something, but this should be possible with the changes on this PR, right?

https://github.com/elastic/kibana/pull/102403/files#diff-7baf074ce8b6f875e3a7bae021ba4cbf98ed6907c9d7819025954b037587395aR51

adds the partial flag to the loader params which can be used on the expression renderer component as well. We can put custom logic in Lens to decide whether to execute an expression with or without partial flag (seems like a relatively easy extension on our side)

flash1293 · 2021-06-23T15:17:32Z

@elasticmachine merge upstream

wylieconlon · 2021-06-23T15:20:48Z

@flash1293 I think you're right that this PR does expose options for us. We can always set <ReactExpressionRenderer partial={shouldRenderPartial()} ...> from Lens so that partial data is rendered only when we need it to be.

flash1293 · 2021-06-23T15:32:32Z

I'm not sure whether this is fully working for Lens (at least for the other bucket).

I set the partial flag here:

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

Line 643 in b35cde5

renderMode="edit"

but the onData callback is still only called once when I'm running a configuration using the other bucket setting on a top values:

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

Line 456 in b35cde5

if (inspectorAdapters && inspectorAdapters.tables) {

As far as I understood the other bucket should be treated as partial result, right?

dokmic · 2021-06-29T19:09:17Z

@flash1293 That does not work because the partial prop is being discarded in the ExpressionWrapper higher-order component.

ppisljar

LGTM

flash1293 · 2021-06-30T09:18:03Z

@dokmic The piece of code you linked is not used in the Lens editor, it's only used on dashboards.

I retried on the latest version and it doesn't work for me, I guess I'm doing something wrong. These are the steps I took:

add partial={true} here

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

Line 644 in b35cde5

renderError={(errorMessage?: string | null, error?: ExpressionRenderError | null) => {
Verify the flag is passed to the expression renderer component

Load a chart which includes an other bucket (top values function). My understanding is that it should cause the chart to render twice - once without the other bucket row, then again with the other bucket. However it's just rendering once (with the other bucket):

If you are seeing something else I'm happy to sync offline to clear up my confusion.

flash1293

Discussed this offline and this PR won't give the ability the actually render partial results in Lens yet - further steps are necessary (esaggs has to return an observable instead of a promise).

I tested Lens, Visualize and TSVB and didn't notice anything breaking. LGTM for Kibana app side.

…etion

… flag

wylieconlon

Okay, LGTM. I agree that we can add an additional parameter to indicate the status of the partial results.

@flash1293 Other bucket is not the only way to test the partial results, but it does seem like an easy one. The other one that I've found to work pretty well is creating a script that performs more work on older data and less work on newer data. Sharing this script in case it helps:

ZonedDateTime date = doc['@timestamp'].value;
long doctime = date.toInstant().toEpochMilli();
String datetime = '2021-07-07T22:15:30Z';
ZonedDateTime now = ZonedDateTime.parse(datetime);
long nowtime = now.toInstant().toEpochMilli();
long multiple = 20 * (nowtime - doctime) / 31536000000L;
for (int i = 0; i < multiple; i++) {
    String local = '1983-10-13T22:15:30Z';
    ZonedDateTime localz = ZonedDateTime.parse(datetime);
    long epoch = localz.toInstant().toEpochMilli();
}
emit(multiple);

The other change that needs to be made to test locally is updating batched_reduce_size: 64 -> batched_reduce_size: 2 in request_utils.ts

clintandrewhall

LGTM

kibanamachine · 2021-07-01T20:44:45Z

💚 Build Succeeded

Metrics [docs]

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
`expressions`	1534	1535	+1

Async chunks

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

id	before	after	diff
`canvas`	1.2MB	1.2MB	+222.0B
`visTypeTimeseries`	1007.3KB	1007.3KB	+68.0B
total			+290.0B

Page load bundle

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

id	before	after	diff
`expressions`	210.2KB	211.3KB	+1.0KB

Unknown metric groups

API count

id	before	after	diff
`expressions`	1962	1966	+4

History

💚 Build #135286 succeeded c5b793eb702320838db2daa2f613ffe76528439d
💚 Build #133612 succeeded 0affc070e8385224f795dab771bedba4fa17ebc1
💚 Build #132690 succeeded 206f873b5f344d43fbc0185378ca14010e94a72f
💔 Build #132680 failed 63bddeb359977f94197b57bcf0c61c2775b50ea3
💚 Build #132551 succeeded 2bc22d4e1eeb6a0434c6429d03cd04e8fcc1fd54

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

… support (#102403) (#104210) * Add partial result flag to the execution result * Update expressions plugin run method to return observable * Update data getter in the execution contract to return observable * Update the expression loader to take into account the partial results flag

dokmic added release_note:enhancement review Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) v8.0.0 Team:AppServices v7.14.0 labels Jun 16, 2021

dokmic changed the title ~~[Expressions] Update expressions public API to expose partial results~~ [Expressions] Update expressions public API to expose partial results support Jun 16, 2021

dokmic force-pushed the feature/98280 branch 4 times, most recently from 7e79486 to 2bc22d4 Compare June 21, 2021 07:09

dokmic marked this pull request as ready for review June 21, 2021 09:15

dokmic requested a review from a team June 21, 2021 09:15

dokmic requested review from a team as code owners June 21, 2021 09:15

dokmic requested a review from ppisljar June 21, 2021 09:16

ppisljar reviewed Jun 21, 2021

View reviewed changes

flash1293 requested changes Jun 21, 2021

View reviewed changes

dokmic force-pushed the feature/98280 branch from 63bddeb to 206f873 Compare June 21, 2021 13:48

wylieconlon suggested changes Jun 21, 2021

View reviewed changes

dokmic force-pushed the feature/98280 branch from 0affc07 to c5b793e Compare June 29, 2021 18:45

ppisljar approved these changes Jun 30, 2021

View reviewed changes

spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

flash1293 approved these changes Jul 1, 2021

View reviewed changes

dokmic added 6 commits July 1, 2021 20:40

Update execution service to complete result observable on input compl…

ad296f2

…etion

Add partial result flag to the execution result

6a93bf0

Update expressions plugin run method to return observable

fecbc72

Update data getter in the execution contract to return observable

41b51c0

Update the expression loader to take into account the partial results…

d94816a

… flag

Update API documentation

5372b86

dokmic force-pushed the feature/98280 branch from c5b793e to 5372b86 Compare July 1, 2021 18:40

wylieconlon approved these changes Jul 1, 2021

View reviewed changes

clintandrewhall approved these changes Jul 1, 2021

View reviewed changes

dokmic merged commit 975e361 into elastic:master Jul 1, 2021

dokmic deleted the feature/98280 branch July 1, 2021 20:48

dokmic mentioned this pull request Jul 1, 2021

[7.x] [Expressions] Update expressions public API to expose partial results support (#102403) #104210

Merged

dokmic mentioned this pull request Aug 10, 2021

[Expressions] Add support of partial results to the switch expression function #108086

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Expressions] Update expressions public API to expose partial results support #102403

[Expressions] Update expressions public API to expose partial results support #102403

dokmic commented Jun 16, 2021 •

edited

Loading

spalger commented Jun 16, 2021

elasticmachine commented Jun 21, 2021

ppisljar Jun 21, 2021 •

edited

Loading

ppisljar Jun 21, 2021

flash1293 left a comment •

edited

Loading

dokmic commented Jun 21, 2021

wylieconlon left a comment

wylieconlon Jun 21, 2021

dokmic Jun 22, 2021

flash1293 Jun 22, 2021 •

edited

Loading

wylieconlon Jun 23, 2021

ppisljar Jun 23, 2021

wylieconlon Jun 23, 2021

ppisljar Jun 28, 2021

wylieconlon Jun 21, 2021

dokmic Jun 22, 2021

flash1293 commented Jun 22, 2021 •

edited

Loading

flash1293 commented Jun 23, 2021

wylieconlon commented Jun 23, 2021

flash1293 commented Jun 23, 2021 •

edited

Loading

dokmic commented Jun 29, 2021

ppisljar left a comment

flash1293 commented Jun 30, 2021 •

edited

Loading

flash1293 left a comment •

edited

Loading

wylieconlon left a comment

clintandrewhall left a comment

kibanamachine commented Jul 1, 2021

API count

[Expressions] Update expressions public API to expose partial results support #102403

[Expressions] Update expressions public API to expose partial results support #102403

Conversation

dokmic commented Jun 16, 2021 • edited Loading

Summary

Checklist

For maintainers

spalger commented Jun 16, 2021

elasticmachine commented Jun 21, 2021

ppisljar Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

dokmic commented Jun 21, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Jun 22, 2021 • edited Loading

flash1293 commented Jun 23, 2021

wylieconlon commented Jun 23, 2021

flash1293 commented Jun 23, 2021 • edited Loading

dokmic commented Jun 29, 2021

ppisljar left a comment

Choose a reason for hiding this comment

flash1293 commented Jun 30, 2021 • edited Loading

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

clintandrewhall left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 1, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

dokmic commented Jun 16, 2021 •

edited

Loading

ppisljar Jun 21, 2021 •

edited

Loading

flash1293 left a comment •

edited

Loading

flash1293 Jun 22, 2021 •

edited

Loading

flash1293 commented Jun 22, 2021 •

edited

Loading

flash1293 commented Jun 23, 2021 •

edited

Loading

flash1293 commented Jun 30, 2021 •

edited

Loading

flash1293 left a comment •

edited

Loading