Skip to content

[Filtering] Deduplicate semantic duplicate errors - #1802

Closed
timgrein wants to merge 9 commits into
mainfrom
tim/deduplicate-semantic-duplicate-basic-rule-errors
Closed

timgrein wants to merge 9 commits into
mainfrom
tim/deduplicate-semantic-duplicate-basic-rule-errors

Conversation

@timgrein

@timgrein timgrein commented Oct 17, 2023 •

Copy link
Copy Markdown
Contributor

Closes https://github.com/elastic/enterprise-search-team/issues/6062

The FilterValidationError API allows for multiple ids. I've changed the API of SyncRuleValidationResult to allow to also reference multiple ids as one result could correlate to multiple sync rules. I've adapted the constructor of SyncRuleValidationResult that it can still work with a single value (but now also with a list).

This enables the duplication of the semantic duplicate errors messages:

Note: The ids in the box heading require a Kibana change. This change adapts the message inside the box details.

Before (two times the same error just flipped):
image

After:
image

Checklists

Pre-Review Checklist

  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

Comment on lines +33 to +37
# `str` and `bytes` are also iterable
if not isinstance(rule_ids, Iterable) or isinstance(rule_ids, (str, bytes)):
rule_ids = [rule_ids]
else:
rule_ids = list(rule_ids)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is okish for now, as we sometimes use the rule_ids keyword explicitly and sometimes we omit it. So this makes sure that everything is working without scanning through every usage of SyncRuleValidationResult. Maybe we should revisit using type hints again.

@artem-shelkovnikov

Copy link
Copy Markdown
Member

Visually the change still looks non-actionable, is there some follow-up planned for this change?

@artem-shelkovnikov

Copy link
Copy Markdown
Member

Ah I see, there's now one message instead of two, which is nice. We can address the readability of it separately, I guess?

@timgrein

Copy link
Copy Markdown
Contributor Author

Ah I see, there's now one message instead of two, which is nice. We can address the readability of it separately, I guess?

Yes exactly. The heading is set within Kibana, that's a separate change. The message can be adjusted. WDYT about Basic rule 1 (id: xyz) is semantically equivalent to Basic rule 2 (id: abc). Please delete one of those rules.?

…te-basic-rule-errors' into tim/deduplicate-semantic-duplicate-basic-rule-errors
@artem-shelkovnikov

Copy link
Copy Markdown
Member

WDYT about Basic rule 1 (id: xyz) is semantically equivalent to Basic rule 2 (id: abc). Please delete one of those rules.?

I think ids are not very useful - they are not exposed in UI at all. Is it possible to instead specify the rule - e.g. The rule #1 is semantically equal to rule #4 and have #1 and #4 displayed in UI somehow (like the first column, for example)

Comment thread config.yml
Comment on lines +17 to +24
connectors:
-
connector_id: "BdDmZYsBMwlGH8qyGO3r"
service_type: "mongodb"
api_key: "QjlEbVpZc0JNd2xHSDhxeUx1MVc6aXBubndhc1dScC1lbWVYTndZbWFjZw=="
elasticsearch:
host: "http://localhost:9200"
api_key: "QjlEbVpZc0JNd2xHSDhxeUx1MVc6aXBubndhc1dScC1lbWVYTndZbWFjZw=="

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I expect you didn't mean to to commit this

[basic_rule, semantic_duplicate], key=lambda rule: rule.order
)

semantic_duplicate_msg = f"{format(rules_ordered[0], Format.SHORT.value)} is semantically equal to {format(rules_ordered[1], Format.SHORT.value)}."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, all we really need to do for this issue is make the to_string better for rules. If we just make

def __str__(self):
def _format_field(key, value):
if isinstance(value, Enum):
return f"{key}: {value.value}"
return f"{key}: {value}"
formatted_fields = [
_format_field(key, value) for key, value in self.__dict__.items()
]
return "Basic rule: " + ", ".join(formatted_fields)
customer friendly, we can just have this line be:

return f"{rule_one} is semantically equal to {rule_two}"

@artem-shelkovnikov

Copy link
Copy Markdown
Member

@timgrein do you still want to pursue these changes?

@seanstory

Copy link
Copy Markdown
Member

Bump @timgrein can you either close this or get it cleaned up and ready for re-review?

@timgrein

Copy link
Copy Markdown
Contributor Author

Can be closed, there's probably a better solution for this issue

@timgrein timgrein closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants