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

[TSVB] Improve schema validation performance #97061

Closed
wylieconlon opened this issue Apr 13, 2021 · 6 comments · Fixed by #99063
Closed

[TSVB] Improve schema validation performance #97061

wylieconlon opened this issue Apr 13, 2021 · 6 comments · Fixed by #99063
Assignees
Labels
Feature:TSVB TSVB (Time Series Visual Builder) performance Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@wylieconlon
Copy link
Contributor

This is the last and most important piece of improving TSVB performance in a dashboard. I have been profiling a dashboard that uses 46 panels of TSVB using the metricbeat-* and filebeat-* index patterns, but this dashboard I'm testing is totally empty- I have no matching data, so all the visualizations are empty. I've been able to optimize other parts of the request to return in 2-3 seconds, down from 8-10 seconds before, but the schema validation performance is our biggest bottleneck.

As you can see in this screenshot, the schema validation code is the slowest part of loading the dashboard:

Screen Shot 2021-04-13 at 6 12 47 PM

I don't have a solution to this issue yet, but brainstorming some possibilities:

  1. The @elastic/kibana-core team might improve the performance of schema validation overall
  2. We could simplify the TSVB schema
  3. Maybe there is a way to improve performance by batching requests together

cc @alexwizp

@wylieconlon wylieconlon added Feature:TSVB TSVB (Time Series Visual Builder) performance Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Apr 13, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@pgayvallet
Copy link
Contributor

Point 1 is directly related to #78351

@alexwizp
Copy link
Contributor

alexwizp commented Apr 14, 2021

@wylieconlon interesting point. I thought we validate schema only on development mode. Initially we used that validation for telemetry and then it allowed us to find some errors in schema working with JS files. Now the following code doesn't have a lot of sense especially for production.

try {
  visPayloadSchema.validate(request.body);
} catch (error) {
  framework.logger.debug(
    `Request validation error: ${error.message}. This most likely means your TSVB visualization contains outdated configuration. You can report this problem under https://github.com/elastic/kibana/issues/new?template=Bug_report.md`
  );
}

You plan probably is good but I suggest:

  1. [Step 1] remove code above for 7.13. It should fix thee validation performance issue.
  2. [Step 2] remove schema and use TS types instead (for 7.14)

@stratoula any ideas?

@flash1293
Copy link
Contributor

We introduced this a while back to check whether our schema is containing all of the real world cases - we actually found a bunch of edge cases this way, but if it's hurting performance this badly, IMHO we should remove it until we find a better solution.

@stratoula
Copy link
Contributor

Yes I agree let's remove it. Move to TS is really important, not sure if we can make it for 7.14 but let's see how it goes

alexwizp added a commit that referenced this issue Apr 14, 2021
* [TSVB][performance] remove visPayloadSchema.validate

Part of: #97061

* Update vis.ts
alexwizp added a commit to alexwizp/kibana that referenced this issue Apr 14, 2021
* [TSVB][performance] remove visPayloadSchema.validate

Part of: elastic#97061

* Update vis.ts
alexwizp added a commit that referenced this issue Apr 14, 2021
* [TSVB][performance] remove visPayloadSchema.validate

Part of: #97061

* Update vis.ts
@wylieconlon
Copy link
Contributor Author

Thanks @alexwizp I've run some comparisons before and after your commit, also including the other optimizations that I'm testing locally (improved KQL handling of * query and caching uiSettings). What I'm seeing is that removing TSVB schema validation is a 20-25% improvement over the state I showed in the issue:

Screen Shot 2021-04-14 at 10 35 32 AM

cc @pgayvallet yes, it looks like even if we remove overhead on all the TSVB requests we are still seeing the schema checking being a bottleneck.

madirey pushed a commit to madirey/kibana that referenced this issue May 11, 2021
* [TSVB][performance] remove visPayloadSchema.validate

Part of: elastic#97061

* Update vis.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) performance Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants