Adding 'namespace' for prometheus metrics #13738

alexbarcelo · 2018-04-07T13:39:18Z

Description:

Add a "namespace" for all the component metrics. This is a breaking change, motivated by "official best practices" see Prometheus documentation

A metric name [...] should have a (single-word) application prefix relevant to the domain the metric belongs to. The prefix is sometimes referred to as namespace by client libraries. For metrics specific to an application, the prefix is usually the application name itself.

The code could be tweaked to be non-breaking. IMHO, it is more sane and well-organized to force a top-level namespace and share that by all the metrics --instead of starting the metric name by the hass domain-- but it is arguably the best approach. Before adding the documentation I wanted to discuss that and listen to other opinions. If there is consensus in a different solution, I have no problem modifying this PR to follow the guidelines.

Example entry for `configuration.yaml` (if applicable):

prometheus:
  namespace: "hass"  # this happens to be the default namespace name

Result:

Going to http://hass.example.net:8123/api/prometheus shows:

...
# HELP hass_device_tracker_state State of the device tracker (0/1)
# TYPE hass_device_tracker_state gauge
hass_device_tracker_state{entity="device_tracker.04xxxxxxxx9d",friendly_name="04xxxxxxxx9d"} 1.0
hass_device_tracker_state{entity="device_tracker.androidffxxxxxxxxxxxx2f",friendly_name="android-ffxxxxxxxxxxxxxx2f"} 1.0
...

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
Documentation added/updated in home-assistant.github.io: Add information for the namespace option home-assistant.io#5152

houndci-bot · 2018-04-07T13:39:27Z

homeassistant/components/prometheus.py

@@ -93,7 +101,8 @@ def _metric(self, metric, factory, documentation, labels=None):
        try:
            return self._metrics[metric]
        except KeyError:
-            self._metrics[metric] = factory(metric, documentation, labels)
+            full_metric_name = "%s_%s" % (self.metrics_namespace, metric)
+            self._metrics[metric] = factory(full_metric_name, documentation, labels)


line too long (84 > 79 characters)

This changes includes the information required about the new configuration parameter introduced in home-assistant/core#13738

pvizeli · 2018-04-12T13:54:31Z

homeassistant/components/prometheus.py

 CONFIG_SCHEMA = vol.Schema({
-    DOMAIN: recorder.FILTER_SCHEMA,
+    DOMAIN: recorder.FILTER_SCHEMA.extend({


Use the origin filter schema from https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entityfilter.py

I didn't know that there was a helper for that :) I believe that the following commit addresses what you asked?

alexbarcelo · 2018-04-13T09:57:24Z

I messed up a little bit at the middle (I was not in my main development machine, and did some silly mistakes).

While looking into other components and seeing the entity_filter helper, I felt that moving the filter into its own entry in the settings is a better approach. That means more breaking changes, although the code is easier to mainteinance. The settings will be like this:

prometheus:
  filter:
    <filter settings here>
  namespace: "name"

Although the default

prometheus:

will keep working because everything else is optional.

I will update the documentation PR accordingly

alexbarcelo · 2018-04-25T07:30:21Z

@pvizeli I think that the changes done were the ones you were requesting. Could you confirm that it looks good? At least that specific aspect.

pvizeli · 2018-04-25T10:27:46Z

No my change request are the same: https://github.com/home-assistant/home-assistant/blob/3fd61d8f456f14796e41dc05c77a7ab60861eb84/homeassistant/components/alexa/__init__.py#L34

You should not access to a other component from your componenent. That break the interface logic they we use. As next, with the helper, we can provide a config feature they work over multible component in same way. We need migrate all the old ways to this helper.

alexbarcelo · 2018-04-25T10:32:07Z

I don't understand... Are you refering to an alexa component? I only touched Prometheus stuff... in fact, your change request was related to a stuff that was already there (but given that I was changing that part, it was the moment to fix it). I think that there is no reference to other component, or I am missing something.

alexbarcelo · 2018-04-26T08:56:09Z

homeassistant/components/prometheus.py

 CONF_PROM_NAMESPACE = 'namespace'
 PROM_NAMESPACE = 'hass'

 CONFIG_SCHEMA = vol.Schema({
-    DOMAIN: FILTER_SCHEMA.extend({
+    DOMAIN: vol.All({
+        vol.Optional(CONF_FILTER, default={}): entityfilter.FILTER_SCHEMA,


@pvizeli This is what you asked, isn't it? Using the entityfilter.FILTER_SCHEMA at this point (instead of the previous other-component-stuff-that-may-break).

fabaff · 2018-04-28T22:26:51Z

Could you please update the dependency (REQUIREMENTS)? You have a working setup and this would ensure that the upgrade works. Thanks.

alexbarcelo · 2018-04-29T08:57:00Z

@fabaff Updated to prometheus_client=0.2.0 and restarted my local home assistant installation to test that everything was ok.

Main functionality is working as expected --let me remind that there are breaking changes on the naming of metrics, albeit I defend that there is "a greater good" behind the rename.

I don't know why coveralls is complaining now (or at all). Maybe it has to do with my f*** up when I started on the wrong branch? (shame on me).

balloob · 2018-05-01T19:17:27Z

You can ignore coveralls. It has already been fixed upstream.

You removed the PR instructions that you need to follow to get Travis to pass… run script/gen_requirements_all.py

balloob · 2018-05-01T19:18:06Z

homeassistant/components/prometheus.py

-    DOMAIN: recorder.FILTER_SCHEMA,
+    DOMAIN: vol.All({
+        vol.Optional(CONF_FILTER, default={}): entityfilter.FILTER_SCHEMA,
+        vol.Optional(CONF_PROM_NAMESPACE, default=PROM_NAMESPACE): cv.string,


We should make it have no default and not add it if not provided.

you mean that the metrics name, by default, should not have application prefix at all? The reason for that is to have the "previous" (current, actual) behaviour of prometheus component by default?

alexbarcelo · 2018-05-30T10:49:36Z

I redid the branch because there were so many mistakes on my part that commit history was a mess.

@pvizeli I think that I addressed your issues at a certain point but that was not clear. On the new commits, if 72351dc is not what you were asking, then please tell me so because that means that I still not know what to change :( That commit is in fact unrelated to the purpose of this PR, but it was a good moment to improve it.

@balloob I assumed that you wanted to have no default and, if no namespace is given, maintain the current behaviour. That is what is done (just tested it): the "metrics prefix" will be <namespace>_ or empty, depending if namespace settings is set.

The breaking changes are related to the use of filter settings. Again, not my main purpose of this PR, but collateral ~~damage~~improvements.

alexbarcelo · 2018-06-27T13:57:18Z

Is this PR good to go? Or it still has issues?

* Add information for the namespace option This changes includes the information required about the new configuration parameter introduced in home-assistant/core#13738 * Nesting include/exclude into filter directive * ✏️ Language tweaks * Reflecting the optionality of namespace setting

* Updating prometheus client version * Using `entity_filter` as filter mechanism * New optional `namespace` configuration

homeassistant added cla-signed integration: prometheus merging-to-master This PR is merging into the RC branch and should probably change the branch to `dev`. small-pr PRs with less than 30 lines. labels Apr 7, 2018

houndci-bot reviewed Apr 7, 2018

View reviewed changes

homeassistant added the cla-signed label Apr 7, 2018

fabaff changed the base branch from master to dev April 7, 2018 16:32

fabaff removed the merging-to-master This PR is merging into the RC branch and should probably change the branch to `dev`. label Apr 7, 2018

fabaff changed the title ~~adding 'namespace' for prometheus metrics~~ Adding 'namespace' for prometheus metrics Apr 7, 2018

homeassistant added the cla-signed label Apr 10, 2018

alexbarcelo added a commit to alexbarcelo/home-assistant.github.io that referenced this pull request Apr 11, 2018

Add information for the namespace option

2335a7e

This changes includes the information required about the new configuration parameter introduced in home-assistant/core#13738

alexbarcelo mentioned this pull request Apr 11, 2018

Add information for the namespace option home-assistant/home-assistant.io#5152

Merged

pvizeli requested changes Apr 12, 2018

View reviewed changes

homeassistant added the cla-signed label Apr 12, 2018

alexbarcelo commented Apr 26, 2018

View reviewed changes

homeassistant added the cla-signed label Apr 29, 2018

balloob added the breaking-change label May 1, 2018

balloob reviewed May 1, 2018

View reviewed changes

fabaff mentioned this pull request May 17, 2018

Prometheus metric names #14508

Closed

alexbarcelo added 3 commits May 30, 2018 12:26

Updating prometheus client version

b54195a

Using entity_filter as filter mechanism

72351dc

New optional namespace configuration

283d724

homeassistant added the cla-signed label May 30, 2018

pvizeli approved these changes Jun 28, 2018

View reviewed changes

pvizeli merged commit a277470 into home-assistant:dev Jun 28, 2018

alexbarcelo deleted the master branch June 28, 2018 20:20

balloob mentioned this pull request Jul 6, 2018

0.73 #15330

Merged

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018

Adding 'namespace' for prometheus metrics (home-assistant#13738)

5cfca9a

* Updating prometheus client version * Using `entity_filter` as filter mechanism * New optional `namespace` configuration

home-assistant locked and limited conversation to collaborators Dec 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding 'namespace' for prometheus metrics #13738

Adding 'namespace' for prometheus metrics #13738

alexbarcelo commented Apr 7, 2018 •

edited

houndci-bot Apr 7, 2018

pvizeli Apr 12, 2018

alexbarcelo Apr 12, 2018

alexbarcelo commented Apr 13, 2018 •

edited

alexbarcelo commented Apr 25, 2018

pvizeli commented Apr 25, 2018 •

edited

alexbarcelo commented Apr 25, 2018

alexbarcelo Apr 26, 2018

fabaff commented Apr 28, 2018 •

edited

alexbarcelo commented Apr 29, 2018

balloob commented May 1, 2018

balloob May 1, 2018

alexbarcelo May 9, 2018

alexbarcelo commented May 30, 2018

alexbarcelo commented Jun 27, 2018

Adding 'namespace' for prometheus metrics #13738

Adding 'namespace' for prometheus metrics #13738

Conversation

alexbarcelo commented Apr 7, 2018 • edited

Description:

Example entry for configuration.yaml (if applicable):

Result:

Checklist:

houndci-bot Apr 7, 2018

Choose a reason for hiding this comment

pvizeli Apr 12, 2018

Choose a reason for hiding this comment

alexbarcelo Apr 12, 2018

Choose a reason for hiding this comment

alexbarcelo commented Apr 13, 2018 • edited

alexbarcelo commented Apr 25, 2018

pvizeli commented Apr 25, 2018 • edited

alexbarcelo commented Apr 25, 2018

alexbarcelo Apr 26, 2018

Choose a reason for hiding this comment

fabaff commented Apr 28, 2018 • edited

alexbarcelo commented Apr 29, 2018

balloob commented May 1, 2018

balloob May 1, 2018

Choose a reason for hiding this comment

alexbarcelo May 9, 2018

Choose a reason for hiding this comment

alexbarcelo commented May 30, 2018

alexbarcelo commented Jun 27, 2018

alexbarcelo commented Apr 7, 2018 •

edited

Example entry for `configuration.yaml` (if applicable):

alexbarcelo commented Apr 13, 2018 •

edited

pvizeli commented Apr 25, 2018 •

edited

fabaff commented Apr 28, 2018 •

edited