-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add prometheus metrics exporter #73
base: main
Are you sure you want to change the base?
Conversation
Thanks for your efforts, but there is already a |
Hey @michbeck100. Thank you for your input. Hopefully I can convince you to have a separate Prometheus exporter 🙂
For example one of the currently exported metrics is shown below
The use of
The address (5003) or unit (kWh) is not going to change for the life time of the metrics gathering. |
I still don’t see the need for a dedicated exporter. You can reuse and change/fix the current |
It looks like we could add this to the existing /metrics endpoint with a few extra settings in the config file? Would there a be use case for both styles to run at the same time though? |
There would be no need to have two exporters, that I fully agree with. However I'm sure there's active usage of the current |
@Ranhiru just extend/replace the existing one. I think the endpoint should still be /metrics. Since it will be a new release version we can replace it and people can switch to the new version once they adopted to the new format. |
Hey @michbeck100, apologies for not getting back to you sooner, it's been a busy week. I'm still hoping I can convince you or @bohdan-s to make metrics its own exporter. I'd say that if anything, the HTML and JSON content should have been served through the same exporter with different Although Prometheus style of metrics is different from HTML or JSON, I understand that it's still plain text served through a web endpoint. ( I have a question around the current configuration I've added here. I've added configuration for each of the fields that's returned from my inverter but I'm assuming not all inverters will support all the fields. Do I add some example configuration and leave it up to the user to fill in the rest ? |
metric_name = metric_config['metric_name'] | ||
metric_type = metric_config['type'] | ||
|
||
logging.info(f"Prometheus: Initialising metric {metric_name} as type {metric_type}") |
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.
Maybe rather logging.debug
?
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.
Cool, I'll change that
I think that's ok for now. But for the future we should try to implement an automatism that exports all registers without extra config. |
Basically I like the implementation. What I don't like is the rather complex configuration. That's what I like about the current implementation which just exports all registers. Would be great to have this ability, too. Maybe decide by What do you think? |
@michbeck100 Fair enough. The configuration is definitely complex. How about instead of having all of the current configuration in the config.yaml, we leave only the most commonly used metrics that the influx db exporter currently exports, and move the rest as examples to the README or a Wiki ? In that way, we let the end user decide how complex they want the configuration to be |
Anything else that needs to be done to merge this? |
Update web server to export configurable Prometheus metrics using Prometheus Python Client
Labels are following the Metric and Label naming guide
The metrics output looks like below.