Gold and platinum integrations should implement diagnostic - Githubissues

home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.

https://www.home-assistant.io

Apache License 2.0

69.62k stars 28.8k forks source link

Gold and platinum integrations should implement diagnostic #117576

Open epenet opened 1 month ago

epenet commented 1 month ago

The problem

As per https://github.com/home-assistant/developers.home-assistant/pull/1512, gold and platinum integrations should implement diagnostic.

I discovered via #117565 that the following integrations did not implement it.

[ ] dlna_dms
[x] fronius #117845
[ ] gdacs
[ ] geonetnz_quakes
[x] google_assistant_sdk #118513
[ ] hyperion
[x] modbus #117591 or #117855
[ ] nightscout
[x] nws https://github.com/home-assistant/core/pull/117587
[x] point #117783
[ ] pvpc_hourly_pricing
[ ] risco
[ ] smarttub
[ ] songpal
[x] tellduslive #117784
[ ] vizio
[ ] yeelight

If the integration is unable to implement diagnostic, then a PR should be opened to add a comment against the integration in script/hassfest/manifest.py, explaining why it cannot be implemented. https://github.com/home-assistant/core/blob/aaa5df9981f38b8e0cae8ee640b0056f9066750c/script/hassfest/manifest.py#L118

What version of Home Assistant Core has the issue?

2024.4

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

No response

Link to integration documentation on our website

No response

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 1 month ago

Hey there @fredrike, mind taking a look at this issue as it has been labeled with an integration (point) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `point` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign point` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

point documentation point source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @fredrike, mind taking a look at this issue as it has been labeled with an integration (tellduslive) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `tellduslive` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign tellduslive` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

tellduslive documentation tellduslive source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @janiversen, mind taking a look at this issue as it has been labeled with an integration (modbus) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `modbus` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign modbus` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

modbus documentation modbus source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @dermotduffy, mind taking a look at this issue as it has been labeled with an integration (hyperion) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `hyperion` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign hyperion` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

hyperion documentation hyperion source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @mdz, mind taking a look at this issue as it has been labeled with an integration (smarttub) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `smarttub` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign smarttub` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

smarttub documentation smarttub source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @onfreund, mind taking a look at this issue as it has been labeled with an integration (risco) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `risco` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign risco` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

risco documentation risco source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @rytilahti, @shenxn, mind taking a look at this issue as it has been labeled with an integration (songpal) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `songpal` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign songpal` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

songpal documentation songpal source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @exxamalte, mind taking a look at this issue as it has been labeled with an integration (gdacs) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `gdacs` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign gdacs` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

gdacs documentation gdacs source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @exxamalte, mind taking a look at this issue as it has been labeled with an integration (geonetnz_quakes) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `geonetnz_quakes` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign geonetnz_quakes` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

geonetnz_quakes documentation geonetnz_quakes source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @marciogranzotto, mind taking a look at this issue as it has been labeled with an integration (nightscout) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `nightscout` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign nightscout` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

nightscout documentation nightscout source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @raman325, mind taking a look at this issue as it has been labeled with an integration (vizio) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `vizio` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign vizio` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

vizio documentation vizio source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @farmio, mind taking a look at this issue as it has been labeled with an integration (fronius) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `fronius` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign fronius` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

fronius documentation fronius source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @tronikos, mind taking a look at this issue as it has been labeled with an integration (google_assistant_sdk) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `google_assistant_sdk` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign google_assistant_sdk` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

google_assistant_sdk documentation google_assistant_sdk source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @chishm, mind taking a look at this issue as it has been labeled with an integration (dlna_dms) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `dlna_dms` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign dlna_dms` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

dlna_dms documentation dlna_dms source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @azogue, mind taking a look at this issue as it has been labeled with an integration (pvpc_hourly_pricing) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `pvpc_hourly_pricing` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign pvpc_hourly_pricing` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

pvpc_hourly_pricing documentation pvpc_hourly_pricing source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @matthewflamm, @kamiyo, mind taking a look at this issue as it has been labeled with an integration (nws) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `nws` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign nws` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

nws documentation nws source _{^{(message by IssueLinks)}}

home-assistant[bot] commented 1 month ago

Hey there @zewelor, @shenxn, @starkillerog, @alexyao2015, mind taking a look at this issue as it has been labeled with an integration (yeelight) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `yeelight` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign yeelight` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

yeelight documentation yeelight source _{^{(message by IssueLinks)}}

MatthewFlamm commented 1 month ago

Is there documentation for the diagnostic platform implementation somewhere? A search for "diagnostics" returns only hits for entity_category or snapshot testing in the dev docs.

rytilahti commented 1 month ago

@MatthewFlamm alas, no, not yet. But the implementation is usually rather simple, especially if one is using data update coordinator, for example: https://github.com/home-assistant/core/blob/dev/homeassistant/components/nam/diagnostics.py shows a very minimal implementation.

It's important to go through the data and scrub the unwanted keys as issue reporters may be asked to upload these files. Another example showing much more data being scrubbed can be seen in tplink/diagnostic.py.

janiversen commented 1 month ago

That link would not work for the modbus integration! Be aware that the modbus protocol do not have diagnostic capabilities, so real diagnostic would be nearly impossible to add.

We could of course make a service call that turns on debug, which is just about as close you can get to "diagnostic".

Please advice if the "should implement" will become a demand. That will exclude quite a number of integrations to maintain a platinum/gold status.

Personally I think the correct way would be to define the diagnostic demands before asking integrations to implement "something".

OnFreund commented 1 month ago

First of all I want to echo @janiversen's sentiment - if implementing diagnostics is a requirement, we should properly document it. In particular, it would be helpful to understand how it's used and what it's used for. This leads me to the second point, which is that without further understanding of diagnostics I don't see how to implement this in the Risco integration. Since it's a security system, almost any piece of data that might be relevant for debugging would be redacted.

janiversen commented 1 month ago

A question can "async_get_config_entry_diagnostics" be used by integrations that are yaml based (I cannot find the platform as a configEntry).

It seems to work at entity level, but e.g. modbus is connection oriented so there no extra information pr entity as to what is already available in the entity state. At connection level (platform) the only diagnostic available is connected/disconnected which the user already knows if all entities are unavailable.

Another example is the fronius integration it uses the modbus integration for communication, so diagnostic what type of diagnostic should that integration provide ?

But of course modbus integration can return a near empty dict, but I highly doubt it will help any users.

And just to be sure, I think implementing diagnostic where available is a splendid idea but the "should" should be modified to "should if possible and it adds additional information".

MatthewFlamm commented 1 month ago

Looking at the how this works async_redact_data uses an opt-out policy for including data, meaning that you have to supply the keys to remove. All other data will be included. The data in my integration has keys with variations like "id" and "@id" that contain potentially sensitive information. And I'm not confident that new ones won't be added by the upstream API, or that the API won't accidentally mess up the data structure leaking sensitive information. This is a security problem. I would be much more confident to add this with an opt-in policy, something like async_redact_data_except where only the keys provided are collected for diagnostics.

In this case where we are unsure that the redaction process is complete, is it sufficient to just add in the config entry data that is completed redacted? This gets to @janiversen comment that this would be useless, but only to satisfy the integration level requirement.

joostlek commented 1 month ago

You can always just compile your own set of data to return. The async_redact_data is there because the majority case is that we have a bunch of uniform data that needs redacting.

tronikos commented 1 month ago

The requirement page at https://developers.home-assistant.io/docs/integration_quality_scale_index/ says: "When communicating with a device or service, the integration implements the diagnostics platform which redacts sensitive information." The Google Assistant SDK integration doesn't add any device or entity so I thought I could skip the diagnostics platform. I cannot think of what I could include in diagnostics. Any recommendations?

joostlek commented 1 month ago

I mean, it still connects to a service. The only thing I could think off would be putting the language code in there.

farmio commented 1 month ago

@janiversen in knx we use it to dump the whole yaml config. Maybe that would be an option for Modbus too. fronius integration doesn't use Modbus btw. but a local HTTP API. Not really sure what to log here though.

joostlek commented 1 month ago

Modbus isn't config entry based, so does diagnostics even work for them?

tronikos commented 1 month ago

Yes Google Assistant SDK still connects to a service but my understanding of: "When communicating with a device or service" refers to https://developers.home-assistant.io/docs/architecture/devices-and-services/ which doesn't apply here. Putting just the language code in diagnostics isn't particularly useful.

janiversen commented 1 month ago

@joostlek that is my point from earlier, the definition is "When communicating with a device or service, the integration implements the diagnostics platform which redacts sensitive information.", and not like the other demands, which have "if available", so it seems it MUST be implemented even if it does not work.

What I will be doing for modbus is to dump state of the entity...but honestly it is only to satisfy the quality demand, as it will not help in diagnosing a problem.

There is also something that I do not understand, the diagnostic platform is targeted at end-users, but it seems the dict is not being translated (I might have missed it, but at least the example tplink/diagnostic.py, does not use strings.json ??

joostlek commented 1 month ago

I think that wording comes from one of the Adr, modbus is a protocol integration like mqtt. And it doesn't connect with a specific device.

janiversen commented 1 month ago

The text says "device or service, modbus communicates with a specific device !! and it was added to this PR so surely someone thinks it should be done.

MatthewFlamm commented 1 month ago

I cannot edit the issue description, but nws is completed in #117587. Thanks for the pointers and quick merge!

epenet commented 1 month ago

I have adjusted the description above. If an integration is unable to implement diagnostic, then a PR should be opened to add a comment against the integration in script/hassfest/manifest.py, explaining why it cannot be implemented. https://github.com/home-assistant/core/blob/d1bdf73bc56a99a230ecb6ae0b3bf3106f80413a/script/hassfest/manifest.py#L118-L136

tronikos commented 3 weeks ago

https://github.com/home-assistant/core/pull/118513 takes care of Google Assistant SDK