legrego / homeassistant-elasticsearch

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

Entity/Domain Inclusion/Exclusion precedence is not consistent / slightly broken #273

Closed strawgate closed 2 weeks ago

strawgate commented 4 weeks ago

The current code checks entity_exclusion,entity_inclusion,domain_inclusion,domain_exclusion.

This means that exclusion takes precedence for entity but inclusion takes precedence for domains.

recommendation: We should make exclusion take priority over inclusion

strawgate commented 4 weeks ago

Another behavior that I'm not sure makes sense is that inclusion doesn't actually do anything?

If you have an included_entities list and no other settings defined, all entities still get published. As Exclusion takes priority over inclusion, i think this makes include_entities a no-op.

Recommendation: if include_entities or include_domain is provided, it's an allowlist -- i.e. only entities that match either include_entities or include_domain will be published.

strawgate commented 4 weeks ago

I see we have the current behavior documented at least.

As this would be a breaking change, I'm fine leaving it how it is. Perhaps we can include something in the options panel that makes this a little more clear?

Or we could change it for datastreams as switching from legacy to data streams requires reconfiguring anyway.

The logic I'm proposing would be: If not (excluded entity or domain) and (included entity or domain or no inclusion criteria), then publish

legrego commented 3 weeks ago

As this would be a breaking change, I'm fine leaving it how it is. Perhaps we can include something in the options panel that makes this a little more clear?

We could also make this change as part of 1.0, and just note it as a breaking change.

The logic I'm proposing would be: If not (excluded entity or domain) and (included entity or domain or no inclusion criteria), then publish

That is a lot simpler. I was originally trying to support use cases where you might want to exclude an entire domain, except for specific entities. If you only care about 5 sensors, but wanted everything else from all other domains, then the simpler logic wouldn't work. I clearly botched the implementation though 😅

What do you think about following the filter logic used by the first-party Splunk integration? Perhaps without glob support. https://www.home-assistant.io/integrations/splunk/#configure-filter

strawgate commented 3 weeks ago

That looks like include takes precedence over exclude and entity criteria is evaluated before domain criteria?

I can't imagine glob support would be hard to add if we're interested in it, any thoughts?

legrego commented 3 weeks ago

That looks like include takes precedence over exclude and entity criteria is evaluated before domain criteria?

It appears that way. I need to walk through a couple of examples to see if there are edge cases I'm not considering.

I can't imagine glob support would be hard to add if we're interested in it, any thoughts?

My primary concern is that we'd make the UI more confusing. We have very little control over how the UI elements are rendered, and supporting globs might(?) require us to switch from a dropdown list to a freeform text field. That would be a large usability regression IMO, as we wouldn't be able to offer suggestions like we do with the dropdown.

An alternative is to add additional text fields alongside the existing dropdowns if we wanted both globs and the suggestions we provide today. That feels equally bad to me, as we're already exposing a lot of fields for users to have to reason about.

The Splunk integration can get away with this because they only offer YML configuration, and haven't switched to UI-based config.

So in summary - I only support adding globs if we can do so without removing our ability to offer domain/entity suggestions, and without exposing additional complexity to our users. I haven't seen much demand for such a feature, so I don't want to unnecessarily burden the majority with a feature used by the minority. There might be other UI controls available to us, and I'm not opposed to exploring it if you would find the feature useful.