[TSVB] In the case of 2 or more panels on the dashboard, TSVB renderComplete fires 2 times

[alexwizp]

Closes: #143556, Closes: #144253

Root cause

TSVB uses useEfect to change the DataView. The problem is that esaggs.ts calls indexPatterns.create which updates the link to the cached DataView each time

I don't know why it was implemented that way, but it looks like we can try to get a value from the cache before creating a new one.

Screens

With that changes TSVB works fine

Screen.Recording.2022-10-26.at.13.30.50.mov

[elasticmachine]

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

[mbondyra]

Wow, good catch. It works correctly for most cases, but there's one more that I introduced that should be addressed. When creating annotation fetching logic, I also used indexPatterns.create so for the charts with Lens annotations we'll have the same problem (render twice):

The solution would be to implement the same thing (cachedDataView search first) here:
https://github.com/elastic/kibana/blob/main/src/plugins/event_annotation/common/fetch_event_annotations/request_event_annotations.ts#L95

And you'll have to also update tests in src/plugins/event_annotation/public/fetch_event_annotations/fetch_event_annotations.test.ts
Let me know if you need any help with it.

[alexwizp]

Good point @mbondyra , I think due to the fact of usage of that wrong pattern from the different places we should fix that in a different way. @elastic/kibana-app-services maybe we should introduce new method e.g. getBySpec and reuse it from all places?

[mattkime]

I think I'd go in another direction, perhaps you can tell me why I'm wrong. getBySpec would be more accurately called getByIdOrCreate. The create method was being called because we were unsure if we had a persisted data view. Unfortunately the create method sets the cache but it doesn't check it. I think it might be simpler to add a cache check similar to the dataViews.get method.

Part of me would like to see these methods named a bit better but I guess continuing with the current names is okay.

[alexwizp]

@mattkime Renaming to getByIdOrCreate looks ok to me.

About checking cache in the create method not sure that is's possible with the current implementation. I've created a draft where you can play with it. Here I added console.logs and some comments for all important places. #144110

In general, the current approach looks like:

I just wanted to show that if we interrupt the create method and return the value from the cache, in fact it will be a Promise that will never be resolved

[mattkime]

@alexwizp I still need to take a closer look at this. "At this step, we have in the cache a promise of an uninitialized data view" shouldn't be happening. It would be nice to figure out a way to unblock further work while properly addressing this. This shouldn't take too long but I understand that other things need to be completed.

[mbondyra]

As for the bug, it is fixed 👌🏼

Testing log

• multiple tsvb vises on Dashboard with Lens visualization
• single tsvb vis on Dashboard with Lens visualization
• Canvas check

[alexwizp]

@elasticmachine merge upstream

[alexwizp]

@elasticmachine merge upstream

[mattkime]

It would be nice if dataViewCache.set was called in create so that createFromSpec only creates the dataView and isn't concerned with the cache. Also, we want to clear the cache entry if creating the data view fails.

[alexwizp]

it's a really cool idea! thanks, updated

[mattkime]

changes lgtm, good work!

[alexwizp]

@elastic/kibana-operations please review limits

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 6a01d3c

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
data	2555	2553	-2
dataViews	231	228	-3
total			-5

Page load bundle

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

id	before	after	diff
dataViews	45.3KB	45.5KB	+183.0B

Unknown metric groups

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	57	63	+6
osquery	103	108	+5
securitySolution	439	443	+4
total			+17

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	20	22	+2
fleet	65	71	+6
osquery	104	110	+6
securitySolution	516	520	+4
total			+18

History

• 💔 Build #84353 failed 9dbf8e1
• 💚 Build #84244 succeeded b7d787e74b01ac812b48fdbdeda01e0da9751c07
• 💛 Build #83814 was flaky 1ab8ca77f203d71f486903c60f20a8047b3da6e7
• 💚 Build #83269 succeeded afd113822410f6e1405478733b9b13f18e1876c4
• 💚 Build #83151 succeeded 58bf0c96d4079011bbc59f533368c3920f7fac6b

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

cc @alexwizp