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

Added an option for whether to import go runtime metrics also #22

Merged
merged 1 commit into from
Oct 29, 2017

Conversation

nipuntalukdar
Copy link
Contributor

The command line parameter -no-go-metrics will switch on or off importing go runtime and http handler metrics.

@jonnenauha
Copy link
Owner

Sorry, I have to ask also here why does one want to remove these? Is this some pattern you are seeing in other prom exporters? The scrapes are tens of hundreds of milliseconds atm. so I don't think its performance you are looking for.

You said you are new to prometheus (I'm kind of a noob myself tbh). Have you ran this exporter and seen some problem with the standard metrics prometheus includes. They are also not all go related (some are like number of goroutines) but also potentially useful stuff like request/response timings/stats.

I don't see immediate benefits myself from this code change. Could you explain the rationale or use case for the change.

@nipuntalukdar
Copy link
Contributor Author

nipuntalukdar commented Oct 29, 2017

We are really not removing the metrics, but giving an option to the users to export/not-export the go runtime, http handler metrics.
Actually, exporters written in Golangs force us to export go specific and http handler specific metrics even when user may be least interested in them. This is because of the default behavior of the Prometheus go client library (client_golang). I checked the the official Python (https://github.com/prometheus/client_python) and it exports only the metrics user wants to export, though it could have exported Python interpreter Specific and the http handler specific metrics as well; thankfully it has left that decision to the user. Why should we export some metrics which we are least interested in analyzing :)
Performance is not at all a issue here. The exporters will be in general very lightly loaded and they will return the metrics in milliseconds. And most of the time users' concern will be to analyze the target system metrics, not the exporter metrics. At best, the exporter should give an option whether to export exporter specific (i.e. metrics for goroutine etc.) metrics. Also, these metrics will occupy space in the time series database, and we may be running thousands of agents to collect metrics from many many systems. The metrics retention period may be long enough and hence how much space the metrics will occupy is also a concern. If we can save some space easily, we should go for that.

Another unrelated point: prometheus.Handler is deprecated and promhttp.Handler is the one should be used as from the comments in: https://github.com/prometheus/client_golang/blob/master/prometheus/http.go

@jonnenauha
Copy link
Owner

Thanks. Good point about metrics being bloated on the database side. I looked at the docs and you are right they don't actually recommend prometheus.Handler().

I'll merge this and actually change it so that you have to set -with-go-metrics if you want those included. I mean that is a more rare use for this exporter, the default should only give our metrics. Lets bump the version to 1.4 for the next release and document the change.

Thanks for you effort.

@jonnenauha jonnenauha merged commit fee9a10 into jonnenauha:master Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants