home-assistant / architecture

Repo to discuss Home Assistant architecture
317 stars 99 forks source link

Prometheus plans & code ownership #391

Closed knyar closed 3 years ago

knyar commented 4 years ago

@MartinHjelmare suggested discussing plans and ownership for Prometheus integration in https://github.com/home-assistant/core/pull/31945, so I just wanted to kick off a discussion here.

I have started a short Google doc for us to discuss plans, but if folks would prefer to keep everything here, I can move its content into this issue.

I personally don't have a lot of features in mind, but a few questions related to metric naming came up when @mweinelt reviewed my PR linked above, so I've seeded the doc with some specific naming related improvements.

FWIW, I am also happy to be a 'codeowner' for Prometheus integration and review corresponding PRs.

CC a few other authors and reviewers of recent (2019 & 2020) Prometheus PRs, in alphabetic order: @balloob, @growse, @joegross, @mitchellrj, @nbarrientos, @perosb, @robbiet480, @springstan, @tcastberg, @treydock

balloob commented 4 years ago

Documenting the guidelines is a big 👍

I don't think that a lot of new features are necessary, we just care about exporting it and that's that. Having rules on how we export should also be documented I think.

balloob commented 4 years ago

btw we don't have currently specific README.md in integration folders, but I'm in favor of that to document developer guidelines for the integration

mweinelt commented 4 years ago

What should be the way forward with regards to attributes? I recently add https://github.com/home-assistant/core/pull/35216 to expose those, but I heard they were on the way out. Where is the discussion on that?

While attributes lack the niceties like a unit of measurement or an icon I don't believe they are really important for the Prometheus export and with Prometheus I believe we do favor a bulk export of attributes over having to define every sensor individually.

balloob commented 4 years ago

Like Martin stated in that PR, attributes are on their way out. That's not going to be changed.

If you want to bulk export, use helpers/debounce.py to send data every 5 seconds.

mweinelt commented 4 years ago

Another kink with the current prometheus integration I have is that no namespace is set by default. This makes it quite a lot harder to find hass metrics and unnecessary pollutes the metric search feature of prometheus.

I think we should default that to hass. This would be a breaking change, but no namespace by default is bad UX.

knyar commented 3 years ago

Another kink with the current prometheus integration I have is that no namespace is set by default.

Setting up a non-empty namespace as the default will break metric continuity for most users, which will be very disruptive. What I would like to propose instead is a documentation change encouraging users to configure a namespace: https://github.com/home-assistant/home-assistant.io/pull/17722

frenck commented 3 years ago

@knyar There is no architectural issue needed for discussing the contents of a documentation parent PR.

Considering the other content of this architectural discussion has been conclusive and no longer needed, I'll go ahead and close this one up.