Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
strawgate opened this issue Apr 19, 2024 · 5 comments · Fixed by #272
Closed

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

strawgate opened this issue Apr 19, 2024 · 5 comments · Fixed by #272
Milestone

Comments

@strawgate
Copy link
Collaborator

strawgate commented Apr 19, 2024

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
Copy link
Collaborator Author

strawgate commented Apr 19, 2024

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 added a commit to strawgate/homeassistant-elasticsearch that referenced this issue Apr 19, 2024
@strawgate strawgate changed the title Entity/Domain Inclusion/Exclusion precedence is not consistent Entity/Domain Inclusion/Exclusion precedence is not consistent / slightly broken Apr 19, 2024
@strawgate
Copy link
Collaborator Author

strawgate commented Apr 19, 2024

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
Copy link
Owner

legrego commented Apr 23, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

legrego commented Apr 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants