-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Log non-zero expvars periodically #1955
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,14 @@ | ||
package logp | ||
|
||
import ( | ||
"expvar" | ||
"flag" | ||
"fmt" | ||
"io/ioutil" | ||
"log" | ||
"strconv" | ||
"strings" | ||
"time" | ||
|
||
"github.com/elastic/beats/libbeat/paths" | ||
) | ||
|
@@ -21,8 +24,18 @@ type Logging struct { | |
ToSyslog *bool `config:"to_syslog"` | ||
ToFiles *bool `config:"to_files"` | ||
Level string | ||
Metrics LoggingMetricsConfig `config:"metrics"` | ||
} | ||
|
||
type LoggingMetricsConfig struct { | ||
Enabled *bool `config:"enabled"` | ||
Period *time.Duration `config:"period" validate:"nonzero,min=0s"` | ||
} | ||
|
||
var ( | ||
defaultMetricsPeriod = 30 * time.Second | ||
) | ||
|
||
func init() { | ||
// Adds logging specific flags: -v, -e and -d. | ||
verbose = flag.Bool("v", false, "Log at INFO level") | ||
|
@@ -112,6 +125,8 @@ func Init(name string, config *Logging) error { | |
log.SetOutput(ioutil.Discard) | ||
} | ||
|
||
go logExpvars(&config.Metrics) | ||
|
||
return nil | ||
} | ||
|
||
|
@@ -141,3 +156,57 @@ func getLogLevel(config *Logging) (Priority, error) { | |
} | ||
return level, nil | ||
} | ||
|
||
// snapshotExpvars iterates through all the defined expvars, and for the top | ||
// level vars that are integers it snapshots the name and value in a separate | ||
// map. | ||
func snapshotExpvars(varsMap map[string]int64) { | ||
expvar.Do(func(kv expvar.KeyValue) { | ||
switch kv.Value.(type) { | ||
case *expvar.Int: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this PR covers the most important metrics for the time being so no action is required at the moment IMO. We can improve this when we add "metrics" within beats. We have some expvar metrics that are nested within a map that won't be printed by this. For example, in Metricbeat we have a |
||
varsMap[kv.Key], _ = strconv.ParseInt(kv.Value.String(), 10, 64) | ||
} | ||
}) | ||
} | ||
|
||
// buildMetricsOutput makes the delta between vals and prevVals and builds | ||
// a printable string with the non-zero deltas. | ||
func buildMetricsOutput(prevVals map[string]int64, vals map[string]int64) string { | ||
metrics := "" | ||
for k, v := range vals { | ||
delta := v - prevVals[k] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the libbeatXYZ metrics that are logged by this are counters, but we do have some gauges (like cache size in Winlogbeat) where reporting the delta won't make much sense. But those gauges won't be logged because they are nested inside a map so no action is required at the moment. |
||
if delta != 0 { | ||
metrics = fmt.Sprintf("%s %s=%d", metrics, k, delta) | ||
} | ||
} | ||
return metrics | ||
} | ||
|
||
// logExpvars logs at Info level the integer expvars that have changed in the | ||
// last interval. For each expvar, the delta from the beginning of the interval | ||
// is logged. | ||
func logExpvars(metricsCfg *LoggingMetricsConfig) { | ||
if metricsCfg.Enabled != nil && *metricsCfg.Enabled == false { | ||
Info("Metrics logging disabled") | ||
return | ||
} | ||
if metricsCfg.Period == nil { | ||
metricsCfg.Period = &defaultMetricsPeriod | ||
} | ||
Info("Metrics logging every %s", metricsCfg.Period) | ||
|
||
ticker := time.NewTicker(*metricsCfg.Period) | ||
prevVals := map[string]int64{} | ||
for { | ||
<-ticker.C | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even though it is running in a independent go routine, should we still make sure it is shut down properly when the beat is stopped? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about that, but I struggle with the API for it. We'd need to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A simple version could be just passing the done channel to the go routine and using a select statement. But that would require to move the start of the goroutine into the beat starting process. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok, but then it's not self contained in logp. Maybe it doesn't need to be. |
||
vals := map[string]int64{} | ||
snapshotExpvars(vals) | ||
metrics := buildMetricsOutput(prevVals, vals) | ||
prevVals = vals | ||
if len(metrics) > 0 { | ||
Info("Non-zero metrics in the last %s:%s", metricsCfg.Period, metrics) | ||
} else { | ||
Info("No non-zero metrics in the last %s", metricsCfg.Period) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
// +build !integration | ||
|
||
package logp | ||
|
||
import ( | ||
"expvar" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestSnapshotExpvars(t *testing.T) { | ||
test := expvar.NewInt("test") | ||
test.Add(42) | ||
|
||
vals := map[string]int64{} | ||
snapshotExpvars(vals) | ||
|
||
assert.Equal(t, vals["test"], int64(42)) | ||
} | ||
|
||
func TestBuildMetricsOutput(t *testing.T) { | ||
test := expvar.NewInt("testLog") | ||
test.Add(1) | ||
|
||
prevVals := map[string]int64{} | ||
snapshotExpvars(prevVals) | ||
|
||
test.Add(5) | ||
|
||
vals := map[string]int64{} | ||
snapshotExpvars(vals) | ||
|
||
metrics := buildMetricsOutput(prevVals, vals) | ||
assert.Equal(t, " testLog=5", metrics) | ||
prevVals = vals | ||
|
||
test.Add(3) | ||
vals = map[string]int64{} | ||
snapshotExpvars(vals) | ||
metrics = buildMetricsOutput(prevVals, vals) | ||
assert.Equal(t, " testLog=3", metrics) | ||
} | ||
|
||
func TestBuildMetricsOutputMissing(t *testing.T) { | ||
|
||
prevVals := map[string]int64{} | ||
snapshotExpvars(prevVals) | ||
|
||
test := expvar.NewInt("testLogEmpty") | ||
test.Add(7) | ||
|
||
vals := map[string]int64{} | ||
snapshotExpvars(vals) | ||
metrics := buildMetricsOutput(prevVals, vals) | ||
assert.Equal(t, " testLogEmpty=7", metrics) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Should we enable this by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is enabled by default with a 30s interval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see it on line 170. Would probably make sense to also use a ucfg defaultConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsg For the above: I didn't mean as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried initially but didn't work out due to circular imports (cfgfile imports logp).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-( @urso interesting issue.