-
Notifications
You must be signed in to change notification settings - Fork 491
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
metrics: collect and report Go runtime.metrics #4041
Conversation
@@ -5,7 +5,7 @@ | |||
"TelemetryURI": "{{TelemetryURI}}", | |||
"EnableMetrics": false, | |||
"MetricsURI": "{{MetricsURI}}", | |||
"ConfigJSONOverride": "{ \"TxPoolExponentialIncreaseFactor\": 1, \"DNSBootstrapID\": \"<network>.algodev.network\", \"DeadlockDetection\": -1, \"PeerPingPeriodSeconds\": 30, \"BaseLoggerDebugLevel\": 4, \"EnableProfiler\": true, \"CadaverSizeTarget\": 0 }", | |||
"ConfigJSONOverride": "{ \"TxPoolExponentialIncreaseFactor\": 1, \"DNSBootstrapID\": \"<network>.algodev.network\", \"DeadlockDetection\": -1, \"PeerPingPeriodSeconds\": 30, \"BaseLoggerDebugLevel\": 4, \"EnableProfiler\": true, \"EnableRuntimeMetrics\": true, \"CadaverSizeTarget\": 0 }", |
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.
how to regenerate all these node.json files?
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.
the makefiles use netgoal generate -t net
to create them — it's now enabled by default in netgoal in https://github.com/algorand/go-algorand/pull/4041/files#diff-04beb8fc68bf6283bb3a6a9c66f42da27b56f2acea297027cf4658cffcbae369R249
Codecov Report
@@ Coverage Diff @@
## master #4041 +/- ##
==========================================
+ Coverage 54.43% 54.47% +0.03%
==========================================
Files 390 391 +1
Lines 48549 48594 +45
==========================================
+ Hits 26429 26471 +42
- Misses 19897 19901 +4
+ Partials 2223 2222 -1
Continue to review full report at Codecov.
|
util/metrics/runtime.go
Outdated
} | ||
|
||
// AddMetric does not add runtime metrics to the map used for heartbeat metrics. | ||
func (rm *RuntimeMetrics) AddMetric(_ map[string]float64) {} |
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.
At least add a TODO: WRITEME
comment or something? I get that this isn't the primary way we get data back from metrics, but, see
cmd/algod/main.go:348 metrics.DefaultRegistry().AddMetrics(values)
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 was intentionally leaving it out of the heartbeat message, the reasoning being we only want this for performance testing and it's not super useful to collect at 10 minute resolution with heartbeat messages.. I hadn't considered whether we would want some of these metrics from real nodes reporting telemetry..
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.
Given the flag for turning these metrics on, if they're on they should be on everywhere?
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.
Chris is right, we probably shouldn't write these to the telemetry server. I also kinda agree with Brian on principle. The interface here isn't clear that AddMetric
is only used for heartbeat. On the other hand, what are the odds that we attempt to use this feature for something besides heartbeat?
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 added comments to the interface to explain what each method is for, and implemented AddMetric
Summary
This adds a util.Metrics integration with the builtin Go runtime/metrics package, which provides metrics about memory, GC, and scheduling. It is only enabled if the new config flag EnableRuntimeMetrics is enabled.
Test Plan
New test added to assert metrics are created and formatted properly. Prometheus output is as below: