legrego / homeassistant-elasticsearch

Publish Home-Assistant events to Elasticsearch
https://legrego.github.io/homeassistant-elasticsearch/
MIT License
145 stars 38 forks source link

Add included_domains and included_entities #50

Closed ooii closed 4 years ago

ooii commented 5 years ago

Please check the CONFIG_SCHEMA. The logic behind what I've done is the following: If included_domains is not empty, then only the domains in it are included, all the others are excluded. The same logic is applied to entities (given that its domain is included).

Closes #49

legrego commented 5 years ago

@ooii thanks so much for putting this together! I'll try to take a closer look at this tomorrow and give you any feedback I have.

legrego commented 5 years ago

Sorry I haven't looked at this yet. I haven't forgotten!

ooii commented 5 years ago

Sorry for the diff.

legrego commented 5 years ago

@ooii let me know if you're still interested in working on this. No hard feelings if not, I can finish this up if needed!

ooii commented 5 years ago

Hello,

Sorry @legrego for the response delay. It was the rush before leaving for vacation where I'm now. But I'm still interested to participating to that.

One thing I'd still like to see is making the include. and exclude. configuration entries mutually exclusive. We shouldn't allow users to specify both include and exclude configurations when we will only respect one of them.

Actually, I think a user could include/exclude a domain and its entities without any issue. Example:

  1. Include binary_sensor domain: that will include all the binary sensors.
    1. If the user includes explicitly a binary_sensor, it won't change anything.
    2. If the user excludes explicitly a binary_sensor, then this latter is excluded while all others are included.
  2. Exclude binary_sensor domain: that will exclude all the binary sensors.
    1. If the user excludes explicitly a binary_sensor, it won't change anything.
    2. If the user includes explicitly a binary_sensor, then this latter is the only binary_sensor which is included.

Note that 2.i could be obtained if the user only includes the specific binary_sensor, while 1.i should not be obtained if the user only excludes the specific binary_sensor (I'm not sure that excluding a specific entity should include all other entities of the domain).

We shouldn't allow users to specify both include and exclude configurations when we will only respect one of them

If you mean that including and excluding the same domain/entity should not be accepted, then I agree.

legrego commented 5 years ago

Sorry @legrego for the response delay. It was the rush before leaving for vacation where I'm now. But I'm still interested to participating to that.

No worries at all, and please don't feel rushed!

Actually, I think a user could include/exclude a domain and its entities without any issue

Thanks for explaining your viewpoint here with examples, this is very helpful! I agree that it's useful to allow both include and exclude for the scenarios you listed above. I think we would want to prevent both exclude.domains and include.domains from being specified, and likewise, prevent both exclude.entities and include.entities from being specified.

Once include.domains or include.entities is specified, it declares a whitelist, so specifying the corresponding exclude configuration is a no-op, and might lead to confusion.

So in summary:

exclude.domains exclude.entities include.domains include.entities Allowed?
- - - - Yes
Set - - - Yes
- Set - - Yes
- - Set - Yes
- - - Set Yes
Set Set - - Yes
Set - Set - No
Set - - Set Yes
- Set Set - Yes
- Set - Set No
- - Set Set Yes
ooii commented 5 years ago

I completely agree with that. I'd suggest also to have the ability to use regex in include/exclude of entities.

legrego commented 5 years ago

I'd suggest also to have the ability to use regex in include/exclude of entities.

I'm not opposed to this, as I think it'd be a pretty powerful feature. My only request is that we make it possible to easily specify entities using normal string notation (e.g., switch.living_room_lights) without having to escape regex characters (e.g., switch\.living_room_lights). Were you thinking of having a separate configuration entry to specify regex vs strings, or did you have something else in mind?

If it's easier, we can always add regex support in a followup PR too, if the string based approach isn't powerful enough.

ooii commented 5 years ago

My only request is that we make it possible to easily specify entities using normal string notation (e.g., switch.living_room_lights) without having to escape regex characters (e.g., switch.living_room_lights).

Totally agreed with that.

Were you thinking of having a separate configuration entry to specify regex vs strings, or did you have something else in mind?

Yes, I'm having this in mind this. We should replace (for excluded_domains)

if (not self._included_domains and state.domain in self._excluded_domains):

by

regex_excluded_domains = "(" + ")|(".join(self._excluded_domains) + ")"
if (not self._included_domains and re.search(regex_excluded_domains, state.domain)):
legrego commented 5 years ago

@ooii that sounds like a plan to me!

ooii commented 5 years ago

@legrego are you on it or do you want me to send a new PR?

legrego commented 5 years ago

@ooii I'll leave that up to you. I haven't done any work on this yet, so if you're still interested in working on this, feel free! Just let me know your plans 😄

ooii commented 5 years ago

That's fine for me. But I'd like to let you manage the part about include. and exclude. configuration entries mutually exclusive. Fine?

legrego commented 5 years ago

good with me!

legrego commented 4 years ago

I'm going to close this since it is over a year behind master. Please rebase and reopen if this is something you're still interested in working on. Thanks!