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

Reduce memory usage of elasticsearch.index metricset #16538

Merged
merged 12 commits into from
Mar 17, 2020

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Feb 25, 2020

What does this PR do?

This PR reduces the memory usage of the elasticsearch.index metricset by parsing the JSON API response from Elasticsearch into structs instead of map[string]interface{}.

Why is it important?

To reduce the overall memory footprint of Metricbeat which makes it less susceptible to the OOM Killer and such.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works Benchmarking test added in Benchmark API response parsing code #17046 is used to prove the improvement in memory usage after this PR.

How to test this PR locally

cd $GOPATH/src/github.com/elastic/beats
go test -bench=. -benchmem ./metricbeat/module/elasticsearch/index/

Sample benchmark results

Before this PR

goos: darwin
goarch: amd64
pkg: github.com/elastic/beats/v7/metricbeat/module/elasticsearch/index
BenchmarkParseAPIResponse-8           25          44391131 ns/op        13464952 B/op     207889 allocs/op
PASS
ok      github.com/elastic/beats/v7/metricbeat/module/elasticsearch/index       1.585s

In terms of memory, the API response parsing implementation before this PR consumed 13.46 MB.

After this PR

goos: darwin
goarch: amd64
pkg: github.com/elastic/beats/v7/metricbeat/module/elasticsearch/index
BenchmarkParseAPIResponse-8           33          31846457 ns/op          596808 B/op      12461 allocs/op
PASS
ok      github.com/elastic/beats/v7/metricbeat/module/elasticsearch/index       1.527s

In terms of memory, the API response parsing implementation after this PR consumed 0.59 MB.

That's an improvement of approximately 1:22!

Related issues

@andresrc andresrc added [zube]: Inbox Team:Services (Deprecated) Label for the former Integrations-Services team labels Mar 10, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@urso
Copy link

urso commented Mar 10, 2020

How big of a problem is it that we have to use a manual parsing approach? We still produce some structured contents. Plus Metricbeat does not acquire these metrics like every 1s. Do we have numbers like before/after?

@ycombinator
Copy link
Contributor Author

Sorry @urso, please ignore this PR for now as it's in draft state. I'm still in the process of figuring out before/after memory usage numbers to check how much this PR would buy us.

@andresrc Can we make sure draft PRs are not given the [zube]: In Review label? I've removed the one for this PR.

@ycombinator
Copy link
Contributor Author

ycombinator commented Mar 10, 2020

Chatted with @urso about this off-PR. We already know that the high memory consumption is coming from parsing the JSON from the Stats API response. He noticed that we were parsing into a map[string]map[string]interface{} here:

Indices map[string]map[string]interface{} `json:"indices"`

He suggested parsing into structs instead, as they have a much lower memory overhead. He also suggested running microbenchmarks to test if there's actually a meaningful gain from such a change.

I did that and here are the results:

go test -bench=. -benchtime=5s -benchmem module/elasticsearch/index/bench_test.go
goos: darwin
goarch: amd64
BenchmarkParseStatsJSON/json_unmarshal_map-8                  67          84769698 ns/op        26763112 B/op     411564 allocs/op
BenchmarkParseStatsJSON/json_unmarshal_struct-8              136          41608684 ns/op         1793964 B/op       9064 allocs/op
PASS
ok      command-line-arguments  16.228s

Parsing a 4.8MB JSON API response into map[string]map[string]interface took up 26.76 MB of memory, whereas parsing the same JSON API response into structs took up 1.79 MB of memory. That's a 1:15 savings!

So I'm going to amend this PR accordingly — we will not switch to a streaming JSON parser, instead we will simply parse into structs instead of the current map[string]map[string]interface{} object.

@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator ycombinator force-pushed the mb-es-xp-mem-usage branch 2 times, most recently from 56f44de to 219d58c Compare March 16, 2020 17:16
@ycombinator ycombinator marked this pull request as ready for review March 17, 2020 15:58
@ycombinator ycombinator added needs_backport PR is waiting to be backported to other branches. v7.7.0 v8.0.0 bug v7.6.2 labels Mar 17, 2020
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Great!

@ycombinator
Copy link
Contributor Author

Travis CI is green and Jenkins CI failures are unrelated. Merging.

@ycombinator ycombinator merged commit be9e426 into elastic:master Mar 17, 2020
@ycombinator ycombinator deleted the mb-es-xp-mem-usage branch March 17, 2020 23:10
@ycombinator ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Mar 17, 2020
ycombinator added a commit that referenced this pull request Mar 18, 2020
…#17069)

* Reduce memory usage of `elasticsearch.index` metricset (#16538)

* [Debugging only!] Add charts to see memory usage over time.

THIS COMMIT MUST BE REMOVED AT THE END!!!

* [Debugging only] Disable validation so we can test only index metricset

* Streaming parser

* Removing debugcharts

* Replace maps with structs

* Running go mod tidy

* Pass pointer

* Uncomment code

* Reverting unnecessary changes

* Adding CHANGELOG entry

* Incorporating benchmark test

* Removing unnecessary nil check

* Fixing up bad rebase
ycombinator added a commit that referenced this pull request Mar 19, 2020
…#17070)

* Reduce memory usage of `elasticsearch.index` metricset (#16538)

* [Debugging only!] Add charts to see memory usage over time.

THIS COMMIT MUST BE REMOVED AT THE END!!!

* [Debugging only] Disable validation so we can test only index metricset

* Streaming parser

* Removing debugcharts

* Replace maps with structs

* Running go mod tidy

* Pass pointer

* Uncomment code

* Reverting unnecessary changes

* Adding CHANGELOG entry

* Incorporating benchmark test

* Removing unnecessary nil check

* Fixing up CHANGELOG

* Fixing up bad rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Team:Services (Deprecated) Label for the former Integrations-Services team v7.6.2 v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce memory usage of elasticsearch/index metricset
5 participants