Skip to content

Add configuration for extra Unifi device_tracker attributes#15711

Closed
cgarwood wants to merge 2 commits intohome-assistant:devfrom
cgarwood:unifi_attributes
Closed

Add configuration for extra Unifi device_tracker attributes#15711
cgarwood wants to merge 2 commits intohome-assistant:devfrom
cgarwood:unifi_attributes

Conversation

@cgarwood
Copy link
Copy Markdown
Member

@cgarwood cgarwood commented Jul 27, 2018

Description:

The Unifi device tracker adds a lot of extra attributes to devices, such as rx/tx rates, rx/tx bytes, signal strength, etc. which cause a lot of extra state changed events to be fired and logged in the database. This update adds a configuration option, extra_attributes, defaulting to False (disabled). This is technically a breaking change since it defaults to false.

This is my first time working with tests, so I'm not sure I updated the test_unify.py file correctly.

Related issue (if applicable): fixes #14745

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5921

Example entry for configuration.yaml (if applicable):

device_tracker:
  - platform: unifi
    host: 192.168.1.201
    verify_ssl: false
    username: !secret unifi_username
    password: !secret unifi_password
    extra_attributes: true
    new_device_defaults:
      track_new_devices: false

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

ctrl.get_clients.return_value = fake_clients
scanner = unifi.UnifiScanner(ctrl, DEFAULT_DETECTION_TIME, ssid_filter)
scanner = unifi.UnifiScanner(ctrl, DEFAULT_DETECTION_TIME, ssid_filter,
False)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

Copy link
Copy Markdown
Contributor

@awarecan awarecan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to introduce breaking change by make extra_attributes default is True.

client = self._clients.get(device, {})
_LOGGER.debug("Device mac %s attributes %s", device, client)
return client
if (self._extra_attributes):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer guard clause

if not self._extra_attributes:
    return {}

bla bla

@cgarwood
Copy link
Copy Markdown
Member Author

I prefer not to introduce breaking change by make extra_attributes default is True.

I debated on that for a bit myself. My thought is that 90% of people aren't going to use the extra attributes that were introduced, and for those people it's writing a ton of data to the database for no gain, and since a lot of people are on Pi's with SD cards, the less writes the better. Likewise, it would only be a breaking change for people who have written automations/templates utilizing those attributes.

I can definitely change the default to true though, if that's what's preferred.

@awarecan
Copy link
Copy Markdown
Contributor

Alright, let's see what others think.

@jeradM
Copy link
Copy Markdown
Member

jeradM commented Jul 28, 2018

I think a breaking change is ok here. This should have been opt-in from the beginning

@frenck
Copy link
Copy Markdown
Member

frenck commented Jul 29, 2018

Missing documentation PR in the OP.

@cgarwood
Copy link
Copy Markdown
Member Author

Docs added, should be good to go now.

@frenck
Copy link
Copy Markdown
Member

frenck commented Jul 29, 2018

Thanks, @cgarwood! Removing the label.

@dgomes
Copy link
Copy Markdown
Contributor

dgomes commented Jul 29, 2018

I also think that a breaking change is advisable here

@jjlawren
Copy link
Copy Markdown
Contributor

jjlawren commented Aug 3, 2018

As suggested in the linked issue, I agree with a breaking change to exclude this extra data. There are likely others that simply haven’t noticed the negative storage effects yet.

Copy link
Copy Markdown
Contributor

@awarecan awarecan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cv.boolean, cv.isfile),
vol.Optional(CONF_DETECTION_TIME, default=DEFAULT_DETECTION_TIME): vol.All(
cv.time_period, cv.positive_timedelta),
vol.Optional(CONF_EXTRA_ATTRIBUTES, default=DEFAULT_EXTRA_ATTRIBUTES):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default=False is better then default=DEFAULT_EXTRA_ATTRIBUTES

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not be consistent we the remaining DEFAUT_*

@smkelly
Copy link
Copy Markdown

smkelly commented Aug 6, 2018

Have you considered also making this accept a list of attributes you actually do want from Unifi? Then you can collect what you care about and discard the rest. This is similar to telling the darksky sensor what weather data you want.

@cgarwood
Copy link
Copy Markdown
Member Author

cgarwood commented Aug 7, 2018

@smkelly that does seem like a better route to go. I modified the code on my dev box to use monitored_conditions list instead of an extra_attributes boolean config and its working OK on my dev box.

Before I commit it and update this PR, what are your thoughts @awarecan ?

@cgarwood
Copy link
Copy Markdown
Member Author

cgarwood commented Aug 8, 2018

Created a new PR to use a monitored_conditions list instead. #15888

@cgarwood cgarwood closed this Aug 8, 2018
@ghost ghost removed the in progress label Aug 8, 2018
@cgarwood cgarwood deleted the unifi_attributes branch August 13, 2018 22:16
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unifi device_tracker filling up database

9 participants