canonical / prometheus-hardware-exporter

Prometheus Hardware Exporter is an exporter for Hardware Observer
GNU General Public License v3.0
10 stars 9 forks source link

Add config option collect_timeout #67

Closed sudeephb closed 6 months ago

sudeephb commented 6 months ago

Add a config option --collect-timeout which can be used to change the timeout used by the collectors while making subprocess calls.

Pjack commented 6 months ago

I suggest to provide only 1 configuration to user , maybe named "scrape_timeout". We can align the timeout value between grafana-agent and commandline tool internal.

sudeephb commented 6 months ago

@Pjack I have changed the name of the option.

facundofc commented 6 months ago

Even though a detail, I strongly disagree with the name change. Scraping is what prometheus does to the exporter, and it will lend to confusion using the same name here. Just imagine having a conversation about this exporter and scrape timeout comes up.

I'd suggest collect_timeout, as that is the right name for this concept: the exporter collects the data when prometheus scrapes the target.

Pjack commented 6 months ago

Even though a detail, I strongly disagree with the name change. Scraping is what prometheus does to the exporter, and it will lend to confusion using the same name here. Just imagine having a conversation about this exporter and scrape timeout comes up.

I'd suggest collect_timeout, as that is the right name for this concept: the exporter collects the data when prometheus scrapes the target.

scrape_timeout is configurable from exporter when we relate with grafana-agent. We also need to change it. That's why I suggest to use the same name. But collect_timeout or timeout , some common name are good to me too. I hope we only expose one configuration to user and we align the timeout within grafana-agent and subprocess internally.

sudeephb commented 6 months ago

It does feel a bit wasteful that we pass the full Config object to each of the collector classes even though they just need to use thecollect_timeout parameter to send it to their Command super class.

If we only pass the collect_timeout option, we'd need to provide more arguments if we need to add extra config in the future, so I think managing all of them inside the config object is okay. Additionally, the config won't be big, it's just 5-6 options, so it's not a huge amount of data.

jneo8 commented 6 months ago

Even though a detail, I strongly disagree with the name change. Scraping is what prometheus does to the exporter, and it will lend to confusion using the same name here. Just imagine having a conversation about this exporter and scrape timeout comes up.

I'd suggest collect_timeout, as that is the right name for this concept: the exporter collects the data when prometheus scrapes the target.

+1 for this. We should keep the exporter simple and leave the relate configuration with grafana-agent in the charm.