-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Add memory usage statistics to Prometheus #9465
Conversation
diagnostics/mem.go
Outdated
defer ticker.Stop() | ||
|
||
for { | ||
dbg.GetMemUsage() |
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.
suggest to be explicit when swallowing an err:
_, _ := dbg.GetMemUsages() // err ignored since this is best effort background information logging
or
_, err := dbg.GetMemUsage()
if err != nil {
logger.Trace("...")
}
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.
From one side: “it maybe does stop the world”, from another: do stop the world in “only 1” goroutine with predictable timer maybe better than from many goroutines.
so, i would say: yes, and remove all other usages. With 1min timer.
seems this PR is related to https://github.com/ledgerwatch/erigon/pull/9482/files |
) | ||
|
||
func SetupMemAccess(metricsMux *http.ServeMux) { | ||
metricsMux.HandleFunc("/mem", func(w http.ResponseWriter, r *http.Request) { |
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.
a bit unclear why need separated http endpoint for memory metrics. we already have /debug/metrics/prometheus
(enabling by flag --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.
I may want to use it to show it in diagnostics
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.
@dvovk i see. but how it answers the question: "why not use existing /debug/metrics/prometheus
? (enabling by flag --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.
Diagnostics are enabled by the metrics flag, however metrics doesn't use the prometheus protocol for gathering data and works using individual requests for specific diagnostics rather than scraping all. Hence it uses additional endpoints for the data its needs.
Having said that it should hit the same data point.
# Conflicts: # go.mod # go.sum
### Prometheus preview ![image](https://github.com/ledgerwatch/erigon/assets/7486955/b720da11-71d1-4f81-8841-35c30e9a1348) --------- Co-authored-by: alex.sharov <AskAlexSharov@gmail.com>
Prometheus preview