Support pushing all sensors and fix wrong metrics (prometheus).

[michaelkuty]

Description:

For example all metrics with unit % match humidity.
This generate correct metrics like this

# HELP nut_ups_battery_charge sensor.nut_ups_battery_charge
# TYPE nut_ups_battery_charge gauge
nut_ups_battery_charge{entity="sensor.nut_ups_battery_charge",friendly_name="NUT UPS Battery Charge"} 98.0
nut_ups_battery_charge{entity="sensor.nut_ups_battery_charge_2",friendly_name="NUT UPS Battery Charge"} 97.0

Contains breaking changes => changed some metric labels

Related issue (if applicable): fixes #

fixes #10084, fixes #9346

• https://community.home-assistant.io/t/prometheus-missing-data/23888

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass
• New dependencies have been added to the REQUIREMENTS variable (example).
• New dependencies are only imported inside functions that use them (example).
• New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
• New files were added to .coveragerc.

If the code does not interact with devices:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass
• Tests have been added to verify that the new code works.

[homeassistant]

Hi @michaelkuty,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[fabaff]

Could you please upgrade the REQUIREMENTS? The lastest release of prometheus_client seems to be 0.1.0 and I assume that you have a running setup which would allow to test it.

Thanks

[MartinHjelmare]

I'm guessing this PR is a breaking change even though it's presumably for the better. @michaelkuty am I correct?

@bah2830 you have done the recent changes to this component. What do you think about this change?

[brentahughes]

This is a breaking change but it is a needed change. Seems fine to me.

Defining the metric name based on the entity name could generate a lot of metrics when it was a label (No difference to prometheus), but that should be fine.

[MartinHjelmare]

@michaelkuty please add a paragraph in the description, describing the breaking changes, for the release notes.

[michaelkuty]

Yes, this produces a lot of metrics, but you can use exclude for pushing what you want.

[michaelkuty]

@fabaff updated

[fabaff]

Looks good to me 🐦

[tcastberg]

I'd like to object quite strongly to this change, this is not the correct way to solve this problem, it completely messes up statistics and it is not the correct way to deal with statistics in prometheus.

This should have been solved by improving labelling, not creating a hundred new metrics. A much better way to solve this would be to add labels/type to devices in the prometheus configuration. We have gone from a situation where we had a selection of available types in Home Assistant / Prometheus to no longer being able to select and graph a set of devices based on type.
From what i can see it should not be too hard to be able to define own types and include/exclude devices in the types.

Please revert urgently and if necessary do a complete rewrite of the prometheus component.

[michaelkuty]

You have one metric per device type and you can use entity_id or friendly name /job to selecting correct metric ? What is wrong here

[tcastberg]

No, you do not have a metric per device type. You have a metric per device name. I have for example named my z-wave devices. Consider also that all z-wave devices that I have report energy consumption. They are not all the same type.
If type was a property of devices in Home Assistant, that could have been used, but unfortunately we don't have that.

[tcastberg]

Just to make it clear, I don't disagree with the prometheus module being improved. I'm all for that as I'm suffering with the same issue with regards to f.ex. battery % vs humidity %. It can be solved in several ways for the short term by either customizing the unit_of_measurement (thought that will drop it out of prometheus) or you can exclude (using regex if you want) in prometheus when you query.

I imagine a proper fix for the prometheus component would be to be able to create your own metrics and include devices in that.
Something like this maybe?

prometheus:
    battery_percentage:
        devices:
            - phone.my_phone
            - ups.my_ups
        description: Percentage of battery remaining

Though to be fair that will cause a fair amount of manual work. I would have preferred to be able to set a type on all Home Assistant devices which would be useful for more than Prometheus I think.

[brentahughes]

With the new changes I have adjusted all my queries to work as needed. Rather than selecting by metric name directly and filters further with labels, I am wildcarding the metric names just like any label.

Example for batteried devices: {entity=~".*(battery|dimmer).*",entity!~".*phone.*"}
- If this required something in the name I could easily add another filter __name__=~".*some_other_thing$

While I agree with @tcastberg it would be nice to have a type in HA to use for the metric but we don't. Luckily Prometheus doesn't really care if you have a lot of metrics with few labels. Or few metrics with lots of labels. It treats them all the same in the end and queries haven't taken a hit nor has storage.

https://prometheus.io/docs/practices/naming/ - From this, the way before and the new way is not following their recommended practices. In terms of making a full series aggregation meaningful the new way checks that box while the old way didn't at all.

I like the new changes strictly for the fact that I get everything and I can change my query to get what I want. If this is changed to something else that is also fine by me as long we still get all metrics.

But as it stands IMO it's not horrible at all. Yes it is against the recommended prometheus best practices. But like I said, prometheus is smart enough to not actually care in the backend.

[tcastberg]

I see what you're saying and to be fair I've never had to wildcard the metric so I was not aware of that. I'll downgrade my estimation of it from horrible to ugly. :) I still don't think this is an appropriate way to solve this and I would have preferred to reject this PR to wait for a proper fix.
The way this has been solved you might as well have just stuck all the metrics in one single metric, the effect would have been the same.

[MartinHjelmare]

Please take this discussion in an RFC issue where you can discuss the future of the prometheus integration.

Merged PRs should not be used for discussion or bug reports. If you've found a bug it's ok to make a review with inline comments and link to an issue that reports the bug.

Thanks!