[Reporting] Fix slow CSV with large max size bytes #120365

jloleysens · 2021-12-03T15:58:48Z

Summary

Follow up PR to improve CSV generation. Using the same reporting config as #120309.

Currently, there is a lot of CPU time spent copying Buffers in the concat method that is called on stream _write.

This contribution attempts to update the behaviour to only concat the necessary buffers when we are about to flush. The CPU profile for a CSV export is the following after these changes are applied

How to test

Start Kibana + ES on basic and start trial license (start Kibana with node --inspect scripts/kibana --dev if you want to profile)
run node scripts/makelogs.js -c 1m --url http://elastic:changeme@localhost:9200 to get larger volumes of data into the cluster
Create a new Data View for the logstash-*
View the data view in Discover and generate a CSV
(Optional) in Chrome, open the node debugger and capture a CPU profile
The CSV should complete, but hits a max length limit

Additional notes

Is there a way that this can be made more performant?
Could be related to [Reporting] Streaming CSV download appears to not be working #119540 (@tsullivan WDYT?)

Release note

We fixed a performance issue that occurred when generating a large CSV export.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
We could introduce new performance issues from unforeseen scenarios	Med	Med	The code has been hardened by adding tests and receiving multiple rounds of review. Each changes was also performance tested with 900k rows.

x-pack/plugins/reporting/server/lib/content_stream.ts

tsullivan · 2021-12-03T20:23:53Z

Could be related to "[Reporting] Streaming CSV download appears to not be working" #119540 (@tsullivan WDYT?)

I think this is a standalone fix.

jloleysens · 2021-12-06T09:47:08Z

@elasticmachine merge upstream

elasticmachine · 2021-12-06T09:55:02Z

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

x-pack/plugins/reporting/server/lib/content_stream.ts

jloleysens · 2021-12-06T17:50:58Z

thanks @dokmic for the feedback. I think I addressed your points. do you think you could take another look?

kibana-ci · 2021-12-06T18:40:31Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #11349 succeeded f82b80e
💔 Build #11226 failed b8c48a6
💔 Build #10996 failed e6996a2

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

tsullivan

LGTM

* use Buffer.alloc + .set API instead of .concat * refactor variable names and actually assign to this.buffer * ok, looks like an array of buffers could work * added large comment and refactored some variable name * fix comment * refactored logic to deal with an edge case where partial buffers should be added, also throw if bad config is detected * added new test for detecting when the write stream throws for bad config * updated logic to not ever call .slice(0), updated the guard for the config error, updated a comment * refactor totalBytesConsumed -> bytesToFlush * use the while loop mike wrote * remove unused variable * update comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* use Buffer.alloc + .set API instead of .concat * refactor variable names and actually assign to this.buffer * ok, looks like an array of buffers could work * added large comment and refactored some variable name * fix comment * refactored logic to deal with an edge case where partial buffers should be added, also throw if bad config is detected * added new test for detecting when the write stream throws for bad config * updated logic to not ever call .slice(0), updated the guard for the config error, updated a comment * refactor totalBytesConsumed -> bytesToFlush * use the while loop mike wrote * remove unused variable * update comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/reporting/server/lib/content_stream.ts

…-chromium-before-compiling-pdf * 'main' of github.com:elastic/kibana: (121 commits) FTR should use the new kibana_system user (elastic#120436) [Lens] Temporarily exclude Mosaic/Waffle from the suggestions list (elastic#120488) [Reporting] Fix slow CSV with large max size bytes (elastic#120365) [CTI] Threat Intel Card on Overview page needs to accommodate Fleet TI integrations - (elastic#120459) [Dashboard] Added KibanaThemeProvider. (elastic#120122) add more rule_registry unit tests (elastic#120323) [Lens] update xpack.lens.pie.smallValuesWarningMessage text (elastic#120478) [Fleet] Simplified package policy create API, enriching default values (elastic#119739) mock `elastic-apm-node` in `@kbn/test` jest preset (elastic#120324) [RAC] Rename occurrences of alert_type to rule_type in Infra (elastic#120455) [Security Solutions] Removes tech debt of exporting all from linter rule for timeline plugin (elastic#120437) [Workplace Search] Add API Keys view to replace Access tokens (elastic#120147) [Security Solutions] Removes tech debt of exporting all from linter rule for cases plugin in the server section (elastic#120411) Add support for threat.feed.name (elastic#120250) [Rule Registry] Rewrite APM registry rules for Observability (elastic#117740) [docs] Fix artifact layout formatting (elastic#119386) [Workplace Search] Add a technical preview of connecting GitHub via GitHub apps (elastic#119764) add osquery notes for 7.16 (elastic#120407) chore(NA): splits types from code on @kbn/docs-utils (elastic#120533) [Reporting] Decouple screenshotting plugin from the reporting (elastic#120110) ... # Conflicts: # x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.test.ts # x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.ts # x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts

* use Buffer.alloc + .set API instead of .concat * refactor variable names and actually assign to this.buffer * ok, looks like an array of buffers could work * added large comment and refactored some variable name * fix comment * refactored logic to deal with an edge case where partial buffers should be added, also throw if bad config is detected * added new test for detecting when the write stream throws for bad config * updated logic to not ever call .slice(0), updated the guard for the config error, updated a comment * refactor totalBytesConsumed -> bytesToFlush * use the while loop mike wrote * remove unused variable * update comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* use Buffer.alloc + .set API instead of .concat * refactor variable names and actually assign to this.buffer * ok, looks like an array of buffers could work * added large comment and refactored some variable name * fix comment * refactored logic to deal with an edge case where partial buffers should be added, also throw if bad config is detected * added new test for detecting when the write stream throws for bad config * updated logic to not ever call .slice(0), updated the guard for the config error, updated a comment * refactor totalBytesConsumed -> bytesToFlush * use the while loop mike wrote * remove unused variable * update comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/reporting/server/lib/content_stream.ts

* use Buffer.alloc + .set API instead of .concat * refactor variable names and actually assign to this.buffer * ok, looks like an array of buffers could work * added large comment and refactored some variable name * fix comment * refactored logic to deal with an edge case where partial buffers should be added, also throw if bad config is detected * added new test for detecting when the write stream throws for bad config * updated logic to not ever call .slice(0), updated the guard for the config error, updated a comment * refactor totalBytesConsumed -> bytesToFlush * use the while loop mike wrote * remove unused variable * update comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

jloleysens added 3 commits December 3, 2021 16:11

use Buffer.alloc + .set API instead of .concat

d82b4ef

refactor variable names and actually assign to this.buffer

c3ff734

ok, looks like an array of buffers could work

e6996a2

jloleysens added release_note:fix (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 v8.1.0 Team:AppServicesUx v7.16.1 v7.15.3 labels Dec 3, 2021

jloleysens requested a review from dokmic December 3, 2021 15:58

jloleysens changed the title ~~Reporting/fix slow csv large max size bytes~~ [Reporting] Fix slow CSV with large max size bytes Dec 3, 2021

dokmic reviewed Dec 3, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 3, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 3, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

tsullivan self-requested a review December 3, 2021 20:21

tsullivan closed this Dec 3, 2021

tsullivan reopened this Dec 3, 2021

added large comment and refactored some variable name

566296c

Merge branch 'main' into reporting/fix-slow-csv-large-max-size-bytes

7f3b1ad

jloleysens marked this pull request as ready for review December 6, 2021 09:55

jloleysens requested review from a team as code owners December 6, 2021 09:55

fix comment

b8c48a6

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic approved these changes Dec 6, 2021

View reviewed changes

jloleysens requested a review from dokmic December 6, 2021 14:39

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Outdated Show resolved Hide resolved

dokmic reviewed Dec 6, 2021

View reviewed changes

x-pack/plugins/reporting/server/lib/content_stream.ts Show resolved Hide resolved

jloleysens added 3 commits December 6, 2021 18:20

use the while loop mike wrote

a00439a

remove unused variable

af1dbfb

update comment

938089f

tsullivan approved these changes Dec 6, 2021

View reviewed changes

jloleysens merged commit c4edb8c into elastic:main Dec 7, 2021

jloleysens deleted the reporting/fix-slow-csv-large-max-size-bytes branch December 7, 2021 09:15

jloleysens mentioned this pull request Dec 7, 2021

[8.0] [Reporting] Fix slow CSV with large max size bytes (#120365) #120579

Merged

jloleysens mentioned this pull request Dec 7, 2021

[7.16] [Reporting] Fix slow CSV with large max size bytes (#120365) #120580

Merged

jloleysens mentioned this pull request Dec 7, 2021

[7.15] [Reporting] Fix slow CSV with large max size bytes (#120365) #120582

Merged

jloleysens added the v7.17.0 label Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Fix slow CSV with large max size bytes #120365

[Reporting] Fix slow CSV with large max size bytes #120365

jloleysens commented Dec 3, 2021 •

edited

Loading

tsullivan commented Dec 3, 2021 •

edited

Loading

jloleysens commented Dec 6, 2021

elasticmachine commented Dec 6, 2021

jloleysens commented Dec 6, 2021

kibana-ci commented Dec 6, 2021

tsullivan left a comment

[Reporting] Fix slow CSV with large max size bytes #120365

[Reporting] Fix slow CSV with large max size bytes #120365

Conversation

jloleysens commented Dec 3, 2021 • edited Loading

Summary

How to test

Additional notes

Release note

Checklist

Risk Matrix

tsullivan commented Dec 3, 2021 • edited Loading

jloleysens commented Dec 6, 2021

elasticmachine commented Dec 6, 2021

jloleysens commented Dec 6, 2021

kibana-ci commented Dec 6, 2021

💚 Build Succeeded

Metrics [docs]

History

tsullivan left a comment

Choose a reason for hiding this comment

jloleysens commented Dec 3, 2021 •

edited

Loading

tsullivan commented Dec 3, 2021 •

edited

Loading