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

Time metrics use Milliseconds instead of base unit - seconds #57

Closed
dannyk81 opened this issue Jun 5, 2017 · 2 comments · Fixed by #58
Closed

Time metrics use Milliseconds instead of base unit - seconds #57

dannyk81 opened this issue Jun 5, 2017 · 2 comments · Fixed by #58

Comments

@dannyk81
Copy link
Contributor

dannyk81 commented Jun 5, 2017

Great work on this exporter! we are using it with great results 👍

One small comment, the following metrics use Milliseconds instead of a base unit (seconds)

elasticsearch_indices_flush_time_ms_total
elasticsearch_indices_indexing_index_time_ms_tot
elasticsearch_indices_merges_total_time_ms_total
elasticsearch_indices_refresh_total_time_ms_total
elasticsearch_indices_store_throttle_time_ms_total
elasticsearch_indices_search_query_time_ms

Is there any specific reason to use ms instead of sec?

I know I'm probably nit-picking here, but was reviewing the best practices from Prometheus and they seem to suggest to use base units, and leave handling the presentation in the Dashboarding tool (i.e. Grafana).

...should use base units (e.g. seconds, bytes, meters - not milliseconds, megabytes, kilometers).

https://prometheus.io/docs/practices/naming/

Any thoughts?

@dominikschulz
Copy link
Contributor

You're perfectly right. These should use seconds as well. Feel free to open an PR to change this.

dannyk81 pushed a commit to dannyk81/elasticsearch_exporter that referenced this issue Jun 8, 2017
@dannyk81
Copy link
Contributor Author

dannyk81 commented Jun 8, 2017

#58 attempts to address that, for your consideration :)

dominikschulz pushed a commit that referenced this issue Jun 12, 2017
* Change time metrics from milliseconds to seconds

Should fix #57

* Fixing styling
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 a pull request may close this issue.

2 participants