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

Export expvar metrics of badger to the metricsFactory #1704

Merged
merged 10 commits into from
Aug 7, 2019
68 changes: 67 additions & 1 deletion plugin/storage/badger/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@
package badger

import (
"expvar"
"flag"
"io/ioutil"
"os"
"strings"
"time"

"github.com/dgraph-io/badger"

// init() badger's metrics to make them available in Initialize()
_ "github.com/dgraph-io/badger/y"
"github.com/spf13/viper"
"github.com/uber/jaeger-lib/metrics"
"go.uber.org/zap"
Expand Down Expand Up @@ -58,6 +63,9 @@ type Factory struct {
LastMaintenanceRun metrics.Gauge
// LastValueLogCleaned stores the timestamp (UnixNano) of the previous ValueLogGC run
LastValueLogCleaned metrics.Gauge

// Expose badger's internal expvar metrics, which are all gauge's at this point
badgerMetrics map[string]metrics.Gauge
}
}

Expand Down Expand Up @@ -118,7 +126,10 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)
f.metrics.LastMaintenanceRun = metricsFactory.Gauge(metrics.Options{Name: lastMaintenanceRunName})
f.metrics.LastValueLogCleaned = metricsFactory.Gauge(metrics.Options{Name: lastValueLogCleanedName})

f.registerBadgerExpvarMetrics(metricsFactory)

go f.maintenance()
go f.metricsCopier()

logger.Info("Badger storage configuration", zap.Any("configuration", opts))

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be metricsTicker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the ticker is closed when the select ends.

Copy link
Member

Choose a reason for hiding this comment

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

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.


err := f.store.Close()

Expand All @@ -175,6 +187,7 @@ func (f *Factory) maintenance() {
return
case t := <-maintenanceTicker.C:
var err error

// After there's nothing to clean, the err is raised
for err == nil {
err = f.store.RunValueLogGC(0.5) // 0.5 is selected to rewrite a file if half of it can be discarded
Expand All @@ -190,3 +203,56 @@ func (f *Factory) maintenance() {
}
}
}

func (f *Factory) metricsCopier() {
metricsTicker := time.NewTicker(10 * time.Second)
defer metricsTicker.Stop()
for {
select {
case <-f.maintenanceDone:
return
case <-metricsTicker.C:
expvar.Do(func(kv expvar.KeyValue) {
if strings.HasPrefix(kv.Key, "badger") {
if intVal, ok := kv.Value.(*expvar.Int); ok {
if g, found := f.metrics.badgerMetrics[kv.Key]; found {
g.Update(intVal.Value())
}
} else if mapVal, ok := kv.Value.(*expvar.Map); ok {
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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

if g, found := f.metrics.badgerMetrics[kv.Key]; found {
g.Update(intVal.Value())
}
}
})
}
}
})
}
}
}

func (f *Factory) registerBadgerExpvarMetrics(metricsFactory metrics.Factory) {
f.metrics.badgerMetrics = make(map[string]metrics.Gauge)

expvar.Do(func(kv expvar.KeyValue) {
if strings.HasPrefix(kv.Key, "badger") {
if _, ok := kv.Value.(*expvar.Int); ok {
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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// The metrics we're interested in have only a single inner key (dynamic name)
// and we're only interested in its value
if _, ok = innerKv.Value.(*expvar.Int); ok {
g := metricsFactory.Gauge(metrics.Options{Name: kv.Key})
f.metrics.badgerMetrics[kv.Key] = g
}
})
}
}
})
}
2 changes: 1 addition & 1 deletion plugin/storage/badger/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type NamespaceConfig struct {
}

const (
defaultMaintenanceInterval time.Duration = 5 * time.Minute
defaultMaintenanceInterval time.Duration = 5 * time.Second
defaultTTL time.Duration = time.Hour * 72
)

Expand Down