expvar related resource leaks

[jonseymour]

expvar.Map are used internally by influx to track various kinds of statistics which are eventually logged into various measurements of the _internal database by the monitor component.

expvar does not have an API that easily allows variables, once created to be removed. As a result once a statistic set has been created, the statistic set persists until the server is restarted. This is acceptable for statistic sets that are essentially singletons, but isn't ideal for statistics sets that grow in number as the size of the database grows; for example the statistic sets created by #5758 to track various cache related stats on TSM WAL segment caches.

The biggest problem is not actually memory - the size of statistics is insignificant to the time series data being processed, but rather the amount of CPU and IO that is being used to publish statistics that stop changing once a shard has been closed for writing or the database it was contained in has been deleted. In this case, the statistic for the shard continues to get published into a measurement every 10 seconds until the infuxd process is restarted even though none of the data in the statistic set ever changes again. In influxd processes that generate lots of shards and stay up for a very long period of time, this overhead may not be insignificant.

There are three problems to be solved:

1. detecting that a statistic map is no longer in active use (e.g. is stale)
2. efficiently removing a reference to a stale statistic map from the "expvar" tree
3. suppressing writes into the _internal database for "stale" statistics

One way to solve the second problem might be to periodically rewrite the expvar tree with new maps that replace maps that contain 'stale' sub maps with new 'sub maps' that don't.

Problem 3 can be solved without solving problem 2, by using a special key value to mark "quiet" maps that are to be ignored by the monitor.

A way to start making progress to an alternative solution without making large changes that touch the entire code base all at once (because of the pervasiveness of expvar references) would be to introduce into influxvar.go a new type and function:

type StatisticsMap interface {
   Get(key string) expvar.Var
   Set(key string, av expvar.Var)
   Add(key string, delta int64)
   AddFloat(key string, delta float64)
}

func NewStatisticsNG(key, name string, tags map[string]string) StatisticsMap

A trivial implementation of NewStatisticsNG would simply be to return the result of NewStatistics. Any client of NewStatisticsNG would continue to operate unchanged except for the type they declare for their statistics map.

A straightforward extension of StatisticsMap would be to add:

   Close()

This would provide a way for the client of the statistics map hint to the monitor that the statistics map will no longer be updated. The hint could be implemented in the form a special key "$do-not-publish" being set to 1 in the underlying expvar map. If a Set or an Add ever occurs again, then the implementation could simply create a new expvar map and add that to the expvar tree in the same location.

If the memory consumed by stale expvar maps did ever prove to be an issue in practice, then a cleaner could be written that walks the expvar tree and rewrites the parents of maps containing "$do-not-publish" keys.

Final Design

• modify influxdb to use a expvar-like Map that supports deletes
• use this map only in the implementation of influxdb to allow management of a top level namespace
• change the monitor so that it completes the termination of a half-dead statistics map after it has persisted any remaining stats
• move all knowledge of how the top level namespace is managed inside the influxdb project by exposing a DoStatistics method to the monitor
• no change to the interface seen by other packages, except to the extent required to call the new influxdb method called DeleteStatistics

Revisions

1. added a 3rd problem
2. added a description of a possible way forward.
3. updated with final design

[jonseymour]

/cc @mark-rushakoff

[mark-rushakoff]

@nathanielc has done some research into removing expvar data and may be able to add some insight here.

[nathanielc]

@mark-rushakoff @jonseymour I recently had the same issue with Kapacitor but much more pronounced. I solved it by creating a new expvar.Map type with a Delete method, basically a copy of the existing implementation + a delete method. Then I published all stats in to a global kapacitor.expvar.Map with a unique key. Once the stat was no longer used it is removed from the global Map and no longer reported. See this Kapacitor commit for the details specifically this file. Those changes are still part of a WIP PR and subject to change but so far I have been pleased with how it works.

[jonseymour]

Thanks @nathanielc - I'll take a look.

[jonseymour]

Is there an influxdata repo that code that, in principle, be shared across both influxdb and kapacitor, can live? (I am assuming in asking this that making influxdb dependent on code from kapacitor might be frowned upon).

[nathanielc]

@jonseymour Go ahead an add it directly to InfluxDB. Kapacitor already pulls code from InfluxDB. Once your changes are in, Kapacitor might be able to just use it. If not then its not shared code :) Thanks for checking

[jonseymour]

As of 2e4dedf, this is the subset of expvar methods that the influxdb tree uses these types:

type Var interface
type Map struct
type String struct
type Int struct
type Float struct
type KeyValue struct

and these methods:

func NewMap(n string) *Map
func NewInt(n string) *Int
func Get(n string) Var
func Do(func(kv KeyValue))
func (v *String) Set(s string)
func (v *Int) Set(i int64)
func (m *Map) Set(n string, v Var)
func (m *Map) Init()
func (m *Map) Add(n string, v int64)
func (m *Map) String() string
func (m *Map) Do(func(kv KeyValue))
func (m *Int) String() string
func (m *String) String() string
func (f *Float) String() string

in these files:

cluster/points_writer.go
cluster/query_executor.go
cluster/service.go
influxvar.go
monitor/service.go
services/collectd/service.go
services/continuous_querier/service.go
services/graphite/service.go
services/hh/node_processor.go
services/hh/service.go
services/httpd/handler.go
services/httpd/service.go
services/opentsdb/handler.go
services/opentsdb/service.go
services/subscriber/service.go
services/udp/service.go
stress/stress_test_server/server.go
tsdb/engine/tsm1/cache.go
tsdb/engine/tsm1/file_store.go
tsdb/engine/tsm1/wal.go
tsdb/shard.go

I might explore an alternative statistics interface that is optimized for the manner that influx uses it, currently backed by expvar &/or @nathanielc's variant of expvar but replaceable in future with another implementation should that ever be necessary.

(Based on changing all imports to github.com/influxdata/influxdb/expvar/ and then creating declarations required to successfully compile the tree - see jonseymour@105dea1)

[jonseymour]

Yes, good things can be done here. Currently the way we are updating stats via the Map.Add and Map.Set methods requires acquisition of read locks on the map. With suitable and reasonable constraints that a new type could place on how expvar is used, we can avoid these locks and only incur the cost of an atomic update. All will be revealed when I push a POC.

[jonseymour]

I decided that the minimum required to address this issue was to introduce a new method DeleteStatistics to the influxdb package which could by called objects that use statistics maps and have bounded life times. I also added a method DoStatistics so that the monitor could iterate over just the statistics maps introduced by influx without having to be aware how the top level name space is managed.

[e-dard]

Assuming this is no longer an issue with the Statistics rewrite done earlier this year.