Reduce memory usage of `elasticsearch.index` metricset by ycombinator · Pull Request #16538 · elastic/beats

ycombinator · 2020-02-25T01:35:59Z

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

Fixes Reduce memory usage of elasticsearch/index metricset #16503.
Depends on Benchmark API response parsing code #17046.

elasticmachine · 2020-03-10T13:51:16Z

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

metricbeat/module/elasticsearch/index/data_xpack.go

urso · 2020-03-10T14:07:08Z

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 · 2020-03-10T14:10:35Z

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 · 2020-03-10T17:30:35Z

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:

beats/metricbeat/module/elasticsearch/index/data.go

Line 34 in 95626b8

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 · 2020-03-11T18:29:46Z

jenkins, test this

THIS COMMIT MUST BE REMOVED AT THE END!!!

jsoriano

Great!

metricbeat/module/elasticsearch/index/data_xpack.go

ycombinator · 2020-03-17T23:10:11Z

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

…#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

…#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

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

andresrc added [zube]: In Review and removed [zube]: Inbox labels Mar 10, 2020

urso reviewed Mar 10, 2020

View reviewed changes

metricbeat/module/elasticsearch/index/data_xpack.go Outdated Show resolved Hide resolved

ycombinator added [zube]: In Progress and removed [zube]: In Review labels Mar 10, 2020

ycombinator mentioned this pull request Mar 17, 2020

Benchmark API response parsing code #17046

Merged

2 tasks

ycombinator added 11 commits March 17, 2020 08:57

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

94e8dc3

THIS COMMIT MUST BE REMOVED AT THE END!!!

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

7e8044a

Streaming parser

4f63066

Removing debugcharts

3bf526e

Replace maps with structs

ab68e10

Running go mod tidy

884614d

Pass pointer

a767e4c

Uncomment code

e5422ed

Reverting unnecessary changes

adb913f

Adding CHANGELOG entry

62734b7

Incorporating benchmark test

7bdccfd

ycombinator requested a review from jsoriano March 17, 2020 15:58

ycombinator added [zube]: In Review v7.7.0 v8.0.0 and removed [zube]: In Progress labels Mar 17, 2020

ycombinator marked this pull request as ready for review March 17, 2020 15:58

zube bot removed v7.7.0 v8.0.0 labels Mar 17, 2020

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

jsoriano approved these changes Mar 17, 2020

View reviewed changes

metricbeat/module/elasticsearch/index/data_xpack.go Show resolved Hide resolved

Removing unnecessary nil check

4620a0f

jsoriano approved these changes Mar 17, 2020

View reviewed changes

ycombinator merged commit be9e426 into elastic:master Mar 17, 2020

ycombinator deleted the mb-es-xp-mem-usage branch March 17, 2020 23:10

zube bot added [zube]: Done and removed [zube]: In Review labels Mar 17, 2020

This was referenced Mar 17, 2020

[7.x] Reduce memory usage of elasticsearch.index metricset (#16538) #17069

Merged

[7.6] Reduce memory usage of elasticsearch.index metricset (#16538) #17070

Merged

ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Mar 17, 2020

ycombinator mentioned this pull request Mar 18, 2020

Fixing typos in JSON field names #17088

Merged

5 tasks

andresrc removed the [zube]: Done label Mar 20, 2020

blakerouse mentioned this pull request Apr 16, 2020

[Heartbeat] Add Additional ECS tls.* fields #17687

Merged

7 tasks

holygrolli mentioned this pull request Apr 22, 2020

Reduce memory usage of elasticsearch/index metricset #16503

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce memory usage of `elasticsearch.index` metricset#16538

Reduce memory usage of `elasticsearch.index` metricset#16538
ycombinator merged 12 commits intoelastic:masterfrom
ycombinator:mb-es-xp-mem-usage

ycombinator commented Feb 25, 2020 •

edited by zube bot

Loading

Uh oh!

elasticmachine commented Mar 10, 2020

Uh oh!

Uh oh!

urso commented Mar 10, 2020

Uh oh!

ycombinator commented Mar 10, 2020

Uh oh!

ycombinator commented Mar 10, 2020 •

edited

Loading

Uh oh!

ycombinator commented Mar 11, 2020

Uh oh!

jsoriano left a comment

Uh oh!

Uh oh!

ycombinator commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ycombinator commented Feb 25, 2020 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Sample benchmark results

Before this PR

After this PR

Related issues

Uh oh!

elasticmachine commented Mar 10, 2020

Uh oh!

Uh oh!

urso commented Mar 10, 2020

Uh oh!

ycombinator commented Mar 10, 2020

Uh oh!

ycombinator commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented Mar 11, 2020

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ycombinator commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ycombinator commented Feb 25, 2020 •

edited by zube bot

Loading

ycombinator commented Mar 10, 2020 •

edited

Loading