Skip to content

fix(grouping): Remove redundant salt wrapper components in legacy grouping #80682

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

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

lobsterkatie
Copy link
Member

In our grouping algorithm, we sometimes include in the hash a constant value (a "salt") alongside the dynamic values generated from the event. (For example, when grouping ExpectCT and Expect-Staple security reports, we include expect-ct or expect-staple alongside the hostname of the server which violated the rule, so that the two types of violations hash into separate groups even when the hostname is the same.)

However, in two places in the legacy grouping algorithm, we're using a salt grouping component not to add an extra value into the hashing but just as an extra wrapper around either an anonymous module name or an anonymous function name. This doesn't change the eventual hash value, but it does violate the thus far unenforced (but soon to be enforced) type signature of both the module and function grouping components, which are meant only to hold actual values, not wrapper components. This fixes the type mismatch by removing the salt component wrappers and placing the module and function name values directly in the module and function grouping components.

Notes:

  • We can see that this doesn't change the hash value (and therefore wouldn't result in a grouping change, even if someone were still on legacy grouping), because the hash values in the affected snapshots don't change.
  • In the case of the module, this change also entailed removing the spot where the ignored module hint used to live. I was going to incorporate it into the remaining hint until I realized that in fact it's been a lie all along - we don't ignore the module! So instead I just got rid of it. (You can see that the module is used because it has a * next to it in the snapshots, meaning it contributes to grouping.)

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 13, 2024
@lobsterkatie lobsterkatie marked this pull request as ready for review November 13, 2024 21:49
@lobsterkatie lobsterkatie requested a review from a team as a code owner November 13, 2024 21:49
Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

It's hard to ignore bugs once we see bugs but as much as it's possible, let's not worry about this code and let's finish moving them off. We need to delete all of this code.

@lobsterkatie
Copy link
Member Author

It's hard to ignore bugs once we see bugs but as much as it's possible, let's not worry about this code and let's finish moving them off. We need to delete all of this code.

Agreed, but in this case the metadata stuff, once typed, won't typecheck without this change.

@JoshFerge
Copy link
Member

It's hard to ignore bugs once we see bugs but as much as it's possible, let's not worry about this code and let's finish moving them off. We need to delete all of this code.

in the future we can always add ignores, and go back in and fix them later.

@lobsterkatie lobsterkatie merged commit 6054329 into master Nov 14, 2024
50 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-remove-salt-wrappers-in-legacy-config branch November 14, 2024 22:02
@github-actions github-actions bot locked and limited conversation to collaborators Nov 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants