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

InverterLimit is missing in Prometheus Metrics #1617

Closed
eloo opened this issue Jan 4, 2024 · 10 comments
Closed

InverterLimit is missing in Prometheus Metrics #1617

eloo opened this issue Jan 4, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@eloo
Copy link

eloo commented Jan 4, 2024

What happened?

Im using prometheus to scrape the inverter data and visualize it using Grafana and thus i have discovered that there is not metric for the inverter limit in the Prometheus metrics.
Especially when you want to set up a zero feed-in the limit value is good to know.

As there are already a lot of data i guess the limit was just "missed"?
Also the limit is preset in MQTT data.

To Reproduce Bug

  • Setup OpenDTU
  • Fetch Prometheus metrics from OpenDTU
  • Do not find something with "limit"

Expected Behavior

When i fetch the prometheus metrics from OpenDTU i would expect the same data i see in mqtt (limit is available in mqtt)

Install Method

Pre-Compiled binary from GitHub

What git-hash/version of OpenDTU?

v23.12.24

Relevant log/trace output

No response

Anything else?

No response

@eloo eloo added the bug Something isn't working label Jan 4, 2024
@tbnobody tbnobody added enhancement New feature or request and removed bug Something isn't working labels Jan 4, 2024
@tbnobody
Copy link
Owner

tbnobody commented Jan 4, 2024

I don't know anything about prometheus. How should the output look like? I need to know the exact text. And keep in mind we have the two values limit_relative and limit_absolute

@eloo
Copy link
Author

eloo commented Jan 4, 2024

ah okay. yes here i can help i guess ;)

first there is a best practice guide for naming: https://prometheus.io/docs/practices/naming/

then all names should be lower-cased

and should have the unit in the name

so i would suggest the following according to the current naming:

opendtu_limit_watts
opendtu_limit_percent

and these metrics should have the label "persistent" with value "true" and "false"

so an example would be

opendtu_limit_watts{persistent="true", ...} 800
opendtu_limit_watts{persistent="false", ...} 600
opendtu_limit_percent{persistent="true", ...} 100
opendtu_limit_percent{persistent="false", ...} 0.75

maybe also "powerlimit" instead of only limit would be good as well

further it maybe would make sense to add the "inverter" component to the metric names in general so distinguish between opendtu itself metrics and metrics related to the inverter

so maybe like this

opendtu_inverter_limit_watts

it also looks like ahoydtu has implemented the same feature a few days ago here:
https://github.com/lumapu/ahoy/pull/1310/files#diff-158378e3b06c7b194af7713696a6d61f59eaa07fc3ca26ea7fae6e7595ab0937

@tbnobody
Copy link
Owner

tbnobody commented Jan 9, 2024

Unfortuatly I cannot distinct between persistent and non-persistent limits when reading the data from the inverter.

The current output looks like this:

# HELP opendtu_inverter_limit_relative current relative limit of the inverter
# TYPE opendtu_inverter_limit_relative gauge
opendtu_inverter_limit_relative{serial="xxxxx",unit="0",name="Meine Solaranlage"} 100.000000

I am wondering if the percentage value should be shown as 0-100 or 0-1 ?

@stefan123t
Copy link

@tbnobody I am struggling with the naming of topics / variables too, but on a more general level, ie. JSON / MQTT ;)
See lumapu/ahoy#1212 (comment) and the MQTT_Topics.xlsx there.

Keeping what @eloo mentioned in mind I would name the two topics / metrics:

current_powerlimit_watts
current_powerlimit_percent

This does away with the absolute and relative keywords many have to think twice about. Great!

Regarding the setting of a new powerlimit, please note that AhoyDTU deprecated setting the persistent powerlimit.
The reason being that this may wear out flash on the inverter which is used to store the permanent powerlimit.

I would vouch for doing away with the rather awkward reverse-polish naming convention non-persistent and replace it with

set_temporary_powerlimit_watts
set_temporary_powerlimit_percent

AhoyDTU accepts both powerlimits on the same MQTT topic, whereby the default is percent
and if it is followed by either % or W the AhoyDTU will treat it as percent / watts.

Tasmota has a similar feature where it assumes 0-100 is always percent
and 101-XXX is a value (minus 100) in watts.

I don't think either is a very good practice / hack and therefor would prefer the two topics above.

@tbnobody
Copy link
Owner

tbnobody commented Jan 9, 2024

This does away with the absolute and relative keywords many have to think twice about.

But this also breaks naming with all current locations where a power limit can be set. If I would change all other locations it would mean incompatibility for all who update to the new version.

On the other hand, is the destinction between relative and absolute really that hard? In the WebUI there are even the units shown. (The same btw. in the documentation for the MQTT topics.)
image

I would this change include in a version which breaks more compatibility.

But the question stays... Whats best practice for prometheus regarding percentage values? 0-1 or 0-100 ? :-)

@stefan123t
Copy link

stefan123t commented Jan 9, 2024

  1. Regarding your question:

Whats best practice for prometheus regarding percentage values? 0-1 or 0-100 ? 😄

Metric and label naming - Base units
https://prometheus.io/docs/practices/naming/#base-units

Base units
Prometheus does not have any units hard coded. For better compatibility, base units should be used. The following lists some metrics families with their base unit. The list is not exhaustive.
Percent ratio Values are 0–1 (rather than 0–100). ratio is only used as a suffix for names like disk_usage_ratio. The usual metric name follows the pattern A_per_B.

  1. Regarding the overhaul of MQTT topics we are aware of the nature of such a Breaking Change, which is why we would consider this for the next version of MQTT topics which would then be same / very similar between OpenDTU and AhoyDTU.

The reason being that many users of dashboards would like to use these extensions for both OpenDTU and AhoyDTU in general.

As most of the current MQTT topics are sent as single topics / values we would target the consolidation of the MQTT topics for the switch from these one metric = 1 topic to a newer JSON based topic structure / tree layout.

I do not know if you would implement such a change too, but we also keep thinking about documenting / defining the Frequency of these topics and the Retained / LWT flags for those topics where it shall apply.
Currently some of the OpenDTU toplevel MQTT topics bssid, rssi, version, etc. topics are sent rather frequently.

Edit: here #1608 (comment) we also discussed the introduction of a meta PV-pool to group several inverters and specify a common powerlimit for a group of inverters only.
This would need to be reflected in such a "new" mqtt topic structure too.

@tbnobody
Copy link
Owner

tbnobody commented Jan 9, 2024

https://prometheus.io/docs/practices/naming/#base-units

Thank you, didn't see it in the first place!

Just a small offtopic related MQTT. I will NOT implement JSON output. Thats just not how MQTT was designed. One value per topic.

There also exists this discussion (#317) for several month. Just had no time/desire to implement it.

@tbnobody
Copy link
Owner

Another question regarding this prometheus stuff. currently all the "names" have the "opendtu_" prefix. is this required to identify the data point correctly? Or is it clear anyways as the endpoint itself identifies the device as opendtu and the prefix can be removed?
What would be the impact? Are you able to rename the existing datapoints in your database? Or will it render all incompatible?

@eloo
Copy link
Author

eloo commented Jan 10, 2024

@tbnobody

TL;DR: please keep the prefix as it makes it easier to find related metrics of this project

imho the prefix is pretty important because you can have tons of metrics in clusters and thus is otherwise hard to find "related" metrics..

e.g. the dtu metrics itself and the inverter metrics.. if they do not share a common prefix you will have hard times to find all metrics related to this project

also this is covered by the best practices as well ;)

...should have a (single-word) application prefix relevant to the domain the metric belongs to.

and others projects follow this as well as you can see in this screenshot (thats just a very short list of metrics in my homelab cluster)

Bildschirmfoto vom 2024-01-10 21-15-18

Or is it clear anyways as the endpoint itself identifies the device as opendtu

in general yes you can use the endpoint, job or some other label as well.. but its often easier to start with the metric name.. also grafana provides a autocompletion for the metrics name

What would be the impact? Are you able to rename the existing datapoints in your database? Or will it render all incompatible?

i guess "renaming" old datapoints would maybe doable somehow but not out of the box or not very easy as the data is "already written"

but the impact would be not sooo hard because you can combine different metrics in a single graph in grafana easily..

Copy link

github-actions bot commented Apr 2, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants