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

[Expressions] Add a flag to opt-out of the partial results in the expressions #144241

Merged
merged 3 commits into from
Nov 4, 2022

Conversation

dokmic
Copy link
Contributor

@dokmic dokmic commented Oct 31, 2022

Summary

Resolves #143344.

Checklist

For maintainers

@dokmic dokmic added review Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppServicesSv release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Oct 31, 2022
@dokmic dokmic marked this pull request as ready for review November 1, 2022 07:55
@dokmic dokmic requested a review from a team as a code owner November 1, 2022 07:55
@elasticmachine
Copy link
Contributor

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

@Dosant
Copy link
Contributor

Dosant commented Nov 1, 2022

Resolves #143444.

Is this is the right issue? Doesn't look like it

@Dosant
Copy link
Contributor

Dosant commented Nov 1, 2022

Trying to understand:
Previously we had partial switch inside the expression loader. Looks like it was false by default and that's why we didn't show partial results in charts.

Now we also add the switch to the execution: why? what is the benefit? How this flag related to the one in the expression loader? Why it is by default true for the execution, but false for the loader ?

@dokmic dokmic force-pushed the feature/143344 branch 2 times, most recently from b8412b2 to a61e0c8 Compare November 2, 2022 19:45
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
expressions 98.1KB 98.1KB +88.0B
Unknown metric groups

API count

id before after diff
expressions 2191 2193 +2

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 58 64 +6
osquery 106 111 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 66 72 +6
osquery 107 113 +6
securitySolution 517 523 +6
total +20

History

  • 💔 Build #84846 failed b8412b21713b572172bd2add21e2b57c2f6c558c
  • 💚 Build #84175 succeeded 9503ab74acdd867c7460437b0c56587a50fecc15
  • 💔 Build #83994 failed c6a6082c65664911edd923293e80206faa4bf02a

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

@dokmic
Copy link
Contributor Author

dokmic commented Nov 3, 2022

@Dosant Thanks for this finding. That makes total sense.

I've reused the newly added flag in the loader and changed the default value to false. Considering #141673 and #143344, it makes sense to disable partial results to avoid potential performance issues and side effects since we are not using them yet. My only concern is that we had it turned on for a while, and now it's changed. WDYT?

Additionally, I've moved the throttle parameter to the execution context to align with the partial flag.

@flash1293
Copy link
Contributor

@Dosant could you take another look at this? We depend on it for the metric trendlines

@Dosant
Copy link
Contributor

Dosant commented Nov 4, 2022

My only concern is that we had it turned on for a while, and now it's changed. WDYT?

In my opinion, If no one actually uses them, there is no harm in disabling them on the expression layer by default as this adds overhead.
And if someone needs it, then can enable it by using the flag, right?

I'll run and play with it shortly

@flash1293
Copy link
Contributor

+1 for disabling by default.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dokmic dokmic merged commit 92251b2 into elastic:main Nov 4, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 4, 2022
@dokmic dokmic deleted the feature/143344 branch November 4, 2022 12:20
@@ -426,6 +426,68 @@ describe('Execution', () => {
});
});
});

test('ignores partial results by default', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I guess this means that we won't have to do anything on the Lens side to take advantage of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes review v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to disable partial results in expressions
7 participants