[WIP] Expressions caching

[ppisljar]

Summary

Adds caching for interpreter:

• caching can be disabled for every call at interpretAst
• at the time function should be executed we calculate a hash of: input, args, executionContext and fnDef and check the cache for existing result. If we find one we can skip executing this function.
• function can have disableCache flag set to true, which will prevent caching of that function

arguments are resolved before this, so using subexpressions, default values etc should work as expected (if subexpression evaluates to the same result it did last time executing the current function can be skipped)

todo:
as timerange which we use in esaggs is in estime format now-90d caching kicks in when we rerun same expression at later time and we don't get new result. For now esaggs function has the disableCache flag set to true. When we refactore esaggs in the future this flag should move to the function that will be generating actual timerange from the estime format

open questions:

• how to handle cache expiration and limits

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• This was checked for cross-browser compatibility, including a check against IE11
• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 3d5950b8f309dead8e0820864f38f61774c93407

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: aea64288af85aa9aff192283f221ee337e4a0708

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: dd82b84fd724fbca4470cfe524b845b89fac4846

[lukasolson]

Suggested change

	functionCache: Map<any, any>;
	functionCache: Map<string, any>;

[streamich]

functionCache: Map<string, unknown>;

I don't know if this will work out, but if it does, it is more restrictive.

[streamich]

Would be nice to have unit tests for this feature.

[lukasolson]

Mostly a nit: Can we standardize on a single variable name like disableCache or enableCache and use it as the name for both the variable in the function definition and when invoking the interpreter?

[streamich]

Also, maybe there is a way to write this in a more modular way and put the caching logic in some separate place. Otherwise, this interpreterProvider function is already too long.

[Dosant]

Possible 0, "", false edge case?
I'd use functionCache.has(hash) to check if there is a cache hit

[ppisljar]

Also, maybe there is a way to write this in a more modular way and put the caching logic in some separate place. Otherwise, this interpreterProvider function is already too long.

are you fine for leaving it as is for now, you plan to refactor interpreterProvider anyway ?

[streamich]

... you plan to refactor interpreterProvider anyway ?

@ppisljar yes in my branch this interpreterProvider function is all refactored and does not exist anymore, which also means we have huge merge conflicts. I can either add caching in my branch based on the example you have here, or we can merge refactoring first.

[ppisljar]

lets merge refactoring first, and lets add caching in separate PR. There is not much of code here so it should not be hard to reimplement in the refactored version.

[ppisljar]

updated this on top of refactoring.

[ppisljar]

retest

[streamich]

You have disabled caching from some of the functions maintained in expressions plugin and esaggs, and tsvb. But opting in by default all functions into caching behaviour seems to be a breaking change, as I understand it, or no? For example, shall we also opt-out of caching some of Lens and Canvas functions?

[streamich]

Is this something we should expose through execution context to all functions? How would functions use this?

[ppisljar]

this should already be provided to all functions on the executionContext.

if some function would to do some internal caching (esaggs with make it slow) this would indicate that it should do it.

[streamich]

Maybe we can first check if we have to cache and only then hash.

const doCaching = !this.context.disableCache && !fn.disableCache;
if (doCaching) {
  const hash = calculateObjectHash([fn, normalizedInput, args, this.context.search]);
}

[LeeDr]

Since we're past 7.11.0 FF, and this isn't a bug fix, should probably bump to 7.12.0.

[flash1293]

@ppisljar This PR is super outdated, did you mean to move it out of draft status or did you want to close it?

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: a94e790
• Pipeline Steps (look for red circles / failed steps)
• Interpreting CI Failures

Failed CI Steps

• Setup Build Environment and Dependencies
• Setup Build Environment and Dependencies

Metrics [docs]

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

History

• 💔 Build #81250 failed a94e790
• 💔 Build #28773 failed 471d95155839f2d89a57c0dbdd55c6ddfc0dc6fb
• 💚 Build #28413 succeeded f3a4677cedf4effee04caa5ced974d8c1d5dd158
• 💚 Build #27003 succeeded 4fa5fa916d32f5f0abe15e8e59a7065a939bf30a

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

[ppisljar]

we implemented request caching on search service which should resolved main needs for which we were thinking about expression caching