-
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
Conversation
Not ready for merging, but discussion/reviews welcome. |
Example log line:
|
@@ -21,8 +24,18 @@ type Logging struct { | |||
ToSyslog *bool `config:"to_syslog"` | |||
ToFiles *bool `config:"to_files"` | |||
Level string | |||
Metrics LoggingMetricsConfig `config:"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.
LGTM |
6d38f6a
to
5746349
Compare
Added docs and a basic system test. @ruflin, can you have a look before I squash? |
LGTM |
Prints the delta since the last period, if it's non-zero. The idea is to help with troubleshooting while still creating relatively little output in the logs. 30s seems like a good compromise. Part of/related to elastic#1931
2b881d2
to
b378ed8
Compare
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 comment
The 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 fetches.mysql-info.failures
metric that is nested two levels deep in maps.
Prints the delta since the last period, if it's non-zero. The idea is
to help with troubleshooting while still creating relatively little output
in the logs. 30s seems like a good compromise.
Part of/related to #1931