Export expvar metrics of badger to the metricsFactory #1704

burmanm · 2019-08-01T10:59:07Z

Which problem is this PR solving?

Badger's internal metrics are not available through jaeger's metrics interface. This adds them so they can be monitored.

Short description of the changes

Copy the metric definitions from expvar and then sync them manually in a background thread. I have a nagging feeling this can't be the best solution though, but I didn't find metricsFactory to have "external collectors" that would fetch the data from there.

Signed-off-by: Michael Burman <yak@iki.fi>

jpkrohling · 2019-08-01T11:15:49Z

plugin/storage/badger/factory.go

@@ -150,7 +161,8 @@ func (f *Factory) CreateDependencyReader() (dependencystore.Reader, error) {

 // Close Implements io.Closer and closes the underlying storage
 func (f *Factory) Close() error {
-	f.maintenanceDone <- true
+	f.maintenanceDone <- true // maintenance close
+	f.maintenanceDone <- true // metrics close


Shouldn't it be metricsTicker?

No, the ticker is closed when the select ends.

why are we writing twice to the same channel? If it's only meant to be read-once for exit signal, then I think the usual practice for that is to close the channel.

…ld be every 5m not 5s and default table loading mode should stick to memory mapping Signed-off-by: Michael Burman <yak@iki.fi>

yurishkuro · 2019-08-03T13:41:30Z

plugin/storage/badger/factory_test.go

+	waiter := func(previousValue int64) int64 {
+		sleeps := 0
+		_, gs := mFactory.Snapshot()
+		for gs["adger_memtable_gets_total"] == previousValue && sleeps < 8 {


why "adger"?

Typo. It always sleeps the maximum with that typo, so the test functions correctly - but might take too long. I'll fix that.

yurishkuro · 2019-08-03T13:46:43Z

so badger code does not use any abstraction for metrics, only writes directly to expvar?

burmanm · 2019-08-03T17:46:16Z

Yes, the badger's metric definitions are here:

https://github.com/dgraph-io/badger/blob/master/y/metrics.go

And they're written like this:

https://github.com/dgraph-io/badger/blob/master/db.go#L1009

yurishkuro · 2019-08-03T21:42:56Z

plugin/storage/badger/factory.go

@@ -150,7 +161,8 @@ func (f *Factory) CreateDependencyReader() (dependencystore.Reader, error) {

 // Close Implements io.Closer and closes the underlying storage
 func (f *Factory) Close() error {
-	f.maintenanceDone <- true
+	f.maintenanceDone <- true // maintenance close
+	f.maintenanceDone <- true // metrics close


why are we writing twice to the same channel? If it's only meant to be read-once for exit signal, then I think the usual practice for that is to close the channel.

yurishkuro · 2019-08-03T21:46:57Z

plugin/storage/badger/factory.go

+						mapVal.Do(func(innerKv expvar.KeyValue) {
+							// The metrics we're interested in have only a single inner key (dynamic name)
+							// and we're only interested in its value
+							if intVal, ok := innerKv.Value.(*expvar.Int); ok {


this is catching the metrics declared like this?

NumLSMGets = expvar.NewMap("badger_lsm_level_gets_total")

Why does Badger use a map in this case? It looks like it could have more than one entry in the map, but we're collapsing all under the map's name.

btw, if multiple values are possible, we could put that value into a tag on the metric.

Currently it is storing in the map the directory of the LSM or Value. We have that information in the Factory properties also, but it might indeed make sense to export that information somewhere. Unless there's a place to view properties already in Jaeger?

With the tags it would look like this:

# HELP jaeger_badger_lsm_size_bytes badger_lsm_size_bytes # TYPE jaeger_badger_lsm_size_bytes gauge jaeger_badger_lsm_size_bytes{directory="/tmp/badger043433444"} 0

as long as there's a guarantee that map has only one entry, it makes more sense to go without the dir name in the label (since it might be different after restart)

yurishkuro · 2019-08-03T21:49:22Z

plugin/storage/badger/factory.go

+				g := metricsFactory.Gauge(metrics.Options{Name: kv.Key})
+				f.metrics.badgerMetrics[kv.Key] = g
+			} else if mapVal, ok := kv.Value.(*expvar.Map); ok {
+				mapVal.Do(func(innerKv expvar.KeyValue) {


wouldn't this map always be empty at this point?

No, since badger.Open() is called before the metrics are fetched. And badger.Open() registers the expvars (it calculates the initial sizes during Open()).

yurishkuro · 2019-08-03T21:51:05Z

plugin/storage/badger/factory_test.go

+	mFactory := metricstest.NewFactory(0)
+	f.Initialize(mFactory, zap.NewNop())
+	assert.NotNil(t, f.metrics.badgerMetrics)
+	_, found := f.metrics.badgerMetrics["badger_memtable_gets_total"]


would be good to see a test for the expvar.Map metrics as well.

… map inner key for metric tag Signed-off-by: Michael Burman <yak@iki.fi>

codecov · 2019-08-05T14:47:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4d698ef). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1704   +/-   ##
=========================================
  Coverage          ?   98.51%           
=========================================
  Files             ?      193           
  Lines             ?     9314           
  Branches          ?        0           
=========================================
  Hits              ?     9176           
  Misses            ?      110           
  Partials          ?       28

Impacted Files	Coverage Δ
plugin/storage/badger/factory.go	`98.09% <100%> (ø)`
plugin/storage/badger/options.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d698ef...b23abb3. Read the comment docs.

Signed-off-by: Michael Burman <yak@iki.fi>

…r_metrics

Export expvar metrics of badger to the metricsFactory

7522e22

Signed-off-by: Michael Burman <yak@iki.fi>

burmanm requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners August 1, 2019 10:59

jpkrohling reviewed Aug 1, 2019

View reviewed changes

Add test for metrics collector. Fix the maintenance interval, it shou…

38595bb

…ld be every 5m not 5s and default table loading mode should stick to memory mapping Signed-off-by: Michael Burman <yak@iki.fi>

yurishkuro reviewed Aug 3, 2019

View reviewed changes

Address comments, add test for map existence, channel closing and use…

c9544d3

… map inner key for metric tag Signed-off-by: Michael Burman <yak@iki.fi>

yurishkuro approved these changes Aug 5, 2019

View reviewed changes

yurishkuro and others added 4 commits August 5, 2019 18:33

Merge branch 'master' into badger_metrics

8a71919

Merge branch 'master' into badger_metrics

ac096cd

Remove directory tag from the metric name

680e1e6

Signed-off-by: Michael Burman <yak@iki.fi>

Merge branch 'badger_metrics' of github.com:burmanm/jaeger into badge…

2e05a8e

…r_metrics

yurishkuro mentioned this pull request Aug 6, 2019

Flaky ES test - TestSpanReader_multiRead_followUp_query #1713

Closed

yurishkuro added 3 commits August 6, 2019 14:13

Merge branch 'master' into badger_metrics

40438c1

Merge branch 'master' into badger_metrics

7fc7581

Merge branch 'master' into badger_metrics

b23abb3

yurishkuro merged commit dcaea9d into jaegertracing:master Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export expvar metrics of badger to the metricsFactory #1704

Export expvar metrics of badger to the metricsFactory #1704

burmanm commented Aug 1, 2019

jpkrohling Aug 1, 2019

burmanm Aug 1, 2019

yurishkuro Aug 3, 2019

yurishkuro Aug 3, 2019

burmanm Aug 3, 2019

yurishkuro commented Aug 3, 2019

burmanm commented Aug 3, 2019

yurishkuro Aug 3, 2019

yurishkuro Aug 3, 2019

yurishkuro Aug 3, 2019

burmanm Aug 5, 2019

burmanm Aug 5, 2019

yurishkuro Aug 5, 2019

yurishkuro Aug 3, 2019

burmanm Aug 5, 2019

yurishkuro Aug 3, 2019

codecov bot commented Aug 5, 2019 •

edited

Loading

Export expvar metrics of badger to the metricsFactory #1704

Export expvar metrics of badger to the metricsFactory #1704

Conversation

burmanm commented Aug 1, 2019

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Aug 3, 2019

burmanm commented Aug 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2019 • edited Loading

Codecov Report

codecov bot commented Aug 5, 2019 •

edited

Loading