Add prometheus sensor component #44508

mweinelt · 2020-12-24T14:22:23Z

Breaking change

None

Proposed change

Implement a sensor that queries Prometheus using PromQL expressions.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml
sensor:
  - platform: prometheus
    url: http://localhost:9090  # this is also the default
    queries:
      - name: entropy
        expr: node_entropy_available_bits{instance="localhost:9100"}
      - name: free disk space
        expr: node_filesystem_free_bytes{instance="localhost:9100", mountpoint="/"}
        unit_of_measurement: Bytes

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: TODO

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io TODO

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

CC @knyar

mweinelt · 2020-12-24T14:25:55Z

homeassistant/components/prometheus/sensor.py

+    def __init__(self, url, session):
+        """Initialize the Prometheus API wrapper."""
+        self._session = session
+        self._url = f"{url}/api/v1/query"


I'm not really happy about using string formatting to build a URL, but urllib.parse.urljoin won't work for https://monitoring.example.com/prometheus, as it would override /prometheus with /api/v1/query instead of appending to it.

Won't urljoin(f"{url}/", "api/v1/query") work for you here?

>>> urljoin("http://monitoring.example.com/prometheus", "/api/v1/query") 'http://monitoring.example.com/api/v1/query'

I strips the /prometheus base path.

Note that I am suggesting to append a slash to url and add api/v1/query as a relative path.

>>> urljoin("http://monitoring.example.com/prometheus/", "/api/v1/query") 'http://monitoring.example.com/api/v1/query'

Like this?

>>> url = "http://monitoring.example.com/prometheus" >>> urljoin(f"{url}/", "api/v1/query") 'http://monitoring.example.com/prometheus/api/v1/query'

homeassistant/components/prometheus/sensor.py

knyar · 2020-12-24T18:36:02Z

homeassistant/components/prometheus/sensor.py

+DEFAULT_URL = "http://localhost:9090"
+CONF_QUERIES = "queries"
+CONF_EXPR = "expr"
+MIN_TIME_BETWEEN_UPDATES = timedelta(seconds=60)


Should this be configurable as well, or is it hard to implement because it's passed to a decorator?

Good question. I'm not sure if a decorator can access a member object.

knyar · 2020-12-24T18:51:03Z

homeassistant/components/prometheus/sensor.py

+
+    async def query(self, expr: str) -> Union[str, float]:
+        """Query expression response."""
+        response = await self._session.get(self._url, params={"query": expr})


Are you happy with the default request timeout (5 minutes) or should it be overriden or configurable?

Haven't looked into it. Should probably be somewhat related to the MIN_TIME_BETWEEN_UPDATES.

knyar · 2020-12-24T19:02:00Z

homeassistant/components/prometheus/sensor.py

+            _LOGGER.error("Invalid query response: %s", error)
+            return STATE_UNKNOWN
+
+        return float(result[0]["value"][1])


I would advise against ignoring all results other than the first one. This could surprise users, and also I don't think Prometheus really guarantees to return results in the same order.

Two possible solutions here: either return an error if the response contains multiple results (pretty simple) or use all of them (for example, by setting a separate state attribute for each result).

There is two unrelated things to consider:

I think from a series I think I can only consider the last value in a sensor entity, or can I backfill it?

Expressions need to map to exactly one series or else its value is ambiguous.

/api/v1/query is an instant query API that I believe will return the latest point for each metric. If you wanted to get a time series, you would need to use a range query API (/api/v1/query_range), but I agree that it does not seem like that can be useful for a Home Assistant sensor (history in HA seems to be provided by the separate history integration that does not seem to have a backfill API).

Yes, that's exactly the issue that I wanted to point out here. I would suggest to return an error unless result contains exactly one element.

Logging an error and returning STATE_PROBLEM if zero or multiple metrics are returned.

homeassistant/components/prometheus/sensor.py

knyar · 2020-12-24T19:10:44Z

homeassistant/components/prometheus/sensor.py

+    }
+)
+
+PLATFORM_SCHEMA = SENSOR_PLATFORM_SCHEMA.extend(


I don't know what's idiomatic here, but I believe most integrations create one entity per each defined sensor, while in this case a single sensor definition will might result in several sensor entities created. Flattening the configuration structure might make it slightly more straightforward (even if a bit more verbose).

What I mean is something like this:

sensor: - platform: prometheus name: entropy query: min(node_entropy_available_bits{instance="localhost:9100"}) - platform: prometheus name: free disk space url: http://localhost:9090 query: min(node_filesystem_free_bytes{instance="localhost:9100", mountpoint="/"}) unit_of_measurement: Bytes

Anyway, hopefully one of the Home Assistant core mainteners will weigh in and provide guidance here.

Then you'd have to pass the url over and over again. The way I did it is basically how the influxdb sensor does it, so I think this is fine.

I guess you can still create multiple prometheus platforms for different prometheus instances though.

I think keeping it this way offers better usability.

We no longer allow new integrations to use a platform config section or old integrations to do changes to the platform config section. See https://github.com/home-assistant/architecture/blob/master/adr/0007-integration-config-yaml-structure.md.

Please refactor this integration to have the config schema in the base integration module and the config yaml section under the top level integration domain key.

knyar · 2020-12-24T19:20:24Z

homeassistant/components/prometheus/sensor.py

+    def __init__(self, url, session):
+        """Initialize the Prometheus API wrapper."""
+        self._session = session
+        self._url = f"{url}/api/v1/query"


Won't urljoin(f"{url}/", "api/v1/query") work for you here?

knyar · 2020-12-26T14:38:38Z

Thanks! I don't have any other comments.
Hopefully one of the core maintainers will be able to review this soon and eventually get it merged.

MartinHjelmare · 2020-12-27T07:26:44Z

homeassistant/components/prometheus/sensor.py

+    async_add_entities(sensors, update_before_add=True)
+
+
+class Prometheus:


Please extract the service specific interface code and data parsing to a standalone library published on PyPI.

https://developers.home-assistant.io/docs/creating_component_code_review#4-communication-with-devicesservices

MartinHjelmare · 2021-02-23T10:28:56Z

There hasn't been any movement for some time here. I'll close now. We're trying to decrease our open PR buffer. Please open a new PR when ready to finish. Thanks!

homeassistant added the cla-signed label Dec 24, 2020

probot-home-assistant bot added integration: prometheus new-integration new-platform labels Dec 24, 2020

mweinelt commented Dec 24, 2020

View reviewed changes

Add prometheus sensor component

47de698

mweinelt force-pushed the prometheus-sensor branch from 864485d to 47de698 Compare December 24, 2020 14:40

knyar suggested changes Dec 24, 2020

View reviewed changes

mweinelt added 2 commits December 25, 2020 21:09

fixup! Add prometheus sensor component

b7e8187

fixup! Add prometheus sensor component

4c313f7

fixup! fixup! Add prometheus sensor component

72a7440

MartinHjelmare requested changes Dec 27, 2020

View reviewed changes

MartinHjelmare closed this Feb 23, 2021

github-actions bot locked and limited conversation to collaborators Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prometheus sensor component #44508

Add prometheus sensor component #44508

mweinelt commented Dec 24, 2020 •

edited

Loading

mweinelt Dec 24, 2020 •

edited

Loading

knyar Dec 24, 2020

mweinelt Dec 24, 2020

knyar Dec 25, 2020

mweinelt Dec 25, 2020

knyar Dec 25, 2020

mweinelt Dec 25, 2020

knyar Dec 24, 2020

mweinelt Dec 24, 2020

knyar Dec 24, 2020

mweinelt Dec 24, 2020

knyar Dec 24, 2020

mweinelt Dec 24, 2020

knyar Dec 25, 2020

mweinelt Dec 25, 2020

knyar Dec 24, 2020

mweinelt Dec 24, 2020 •

edited

Loading

MartinHjelmare Dec 27, 2020

knyar Dec 24, 2020

knyar commented Dec 26, 2020

MartinHjelmare Dec 27, 2020

MartinHjelmare commented Feb 23, 2021

		async_add_entities(sensors, update_before_add=True)


		class Prometheus:

Add prometheus sensor component #44508

Add prometheus sensor component #44508

Conversation

mweinelt commented Dec 24, 2020 • edited Loading

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

mweinelt Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweinelt Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knyar commented Dec 26, 2020

Choose a reason for hiding this comment

MartinHjelmare commented Feb 23, 2021

mweinelt commented Dec 24, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

mweinelt Dec 24, 2020 •

edited

Loading

mweinelt Dec 24, 2020 •

edited

Loading