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

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

Merged
merged 1 commit into from Jan 8, 2018

Conversation

Projects
None yet
7 participants
@michaelkuty
Contributor

michaelkuty commented Dec 15, 2017

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

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:

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

This comment has been minimized.

homeassistant commented Dec 15, 2017

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!

@michaelkuty michaelkuty force-pushed the michaelkuty:prometheus-generic-sensors branch Dec 15, 2017

@michaelkuty michaelkuty changed the title from Support pushing all sensors and fix wrong metrics. to Support pushing all sensors and fix wrong metrics (prometheus). Dec 15, 2017

@michaelkuty michaelkuty force-pushed the michaelkuty:prometheus-generic-sensors branch Dec 15, 2017

@fabaff

This comment has been minimized.

Member

fabaff commented Dec 30, 2017

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

This comment has been minimized.

Member

MartinHjelmare commented Dec 30, 2017

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?

@bah2830

This comment has been minimized.

Contributor

bah2830 commented Dec 31, 2017

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

This comment has been minimized.

Member

MartinHjelmare commented Dec 31, 2017

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

@michaelkuty

This comment has been minimized.

Contributor

michaelkuty commented Dec 31, 2017

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

Support pushing all sensors and fix wrong metrics.
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

@michaelkuty michaelkuty force-pushed the michaelkuty:prometheus-generic-sensors branch to d7e1ba7 Dec 31, 2017

@michaelkuty michaelkuty requested a review from andrey-git as a code owner Dec 31, 2017

@michaelkuty

This comment has been minimized.

Contributor

michaelkuty commented Dec 31, 2017

@fabaff updated

@fabaff

fabaff approved these changes Jan 8, 2018

Looks good to me 🐦

@fabaff fabaff merged commit 0e71009 into home-assistant:dev Jan 8, 2018

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 94.102%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Jan 11, 2018

Merged

0.61 #11589

@tcastberg

This comment has been minimized.

Contributor

tcastberg commented Jan 18, 2018

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

This comment has been minimized.

Contributor

michaelkuty commented Jan 18, 2018

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

This comment has been minimized.

Contributor

tcastberg commented Jan 18, 2018

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

This comment has been minimized.

Contributor

tcastberg commented Jan 18, 2018

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.

@bah2830

This comment has been minimized.

Contributor

bah2830 commented Jan 18, 2018

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

This comment has been minimized.

Contributor

tcastberg commented Jan 18, 2018

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

This comment has been minimized.

Member

MartinHjelmare commented Jan 18, 2018

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!

@home-assistant home-assistant locked and limited conversation to collaborators Jan 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.