AMDResearch / omnistat

https://amdresearch.github.io/omnistat/
MIT License
5 stars 0 forks source link

Remove unused Flask metrics from exporter #100

Closed jordap closed 1 month ago

jordap commented 1 month ago

This is a small PR inspired by one of the changes in @omri-amd is suggesting in #98: commenting out the register_metrics line in node_monitoring.py.

register_metrics provides several metrics that we are not using. Since we are attempting to export the minimum amount of metrics we need, it makes sense to exclude these metrics in production environments.

This is the list of metrics currently exposed by flask_prometheus_metrics with register_metrics:

app_request_count_total
app_request_latency_seconds_bucket
app_version_info
app_request_count_created
app_request_latency_seconds_created
app_request_latency_seconds_sum
app_request_latency_seconds_count
app_request_count_total

Some of the values like the versioning can be misleading since we are using a hardcoded 0.1.0 instead of Omnistat's version.

The only reason to keep this around is to measure the performance of the Omnistat exporter. But if we only use these for Omnistat performance, I think it would make sense to keep them as optional.

This PR removes everything related flask_prometheus_metrics, partially because I wanted to test it and because I want to be careful about adding more configuration options.

Any thoughts about adding another option or entirely removing the Flask metrics?

koomie commented 1 month ago

I'm fine with tossing it.