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

Hash collision risk of metric data aggregation #3060

Open
alangy98 opened this issue Sep 12, 2024 · 1 comment
Open

Hash collision risk of metric data aggregation #3060

alangy98 opened this issue Sep 12, 2024 · 1 comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@alangy98
Copy link

alangy98 commented Sep 12, 2024

Hello, I have concerns regarding the hash collision issue with the current implementation of metric data aggregation, related to this PR #1993.

std::unordered_map<size_t, std::pair<MetricAttributes, std::unique_ptr<Aggregation>>> hash_map_;

The aggregation map uses size_t as the key type, and the hash code of the attribute set is treated as a unique identifier. However, if two different attribute sets happen to generate the same hash code, it could lead to collisions, and I haven’t seen any handling for this scenario in the current logic.

Why not consider using the MetricAttributes instead of its hash code as the key of the unordered_map?

Thanks

@alangy98 alangy98 added the bug Something isn't working label Sep 12, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 12, 2024
@lalitb
Copy link
Member

lalitb commented Sep 17, 2024

@alangy98 The original purpose of the custom hash function was to guarantee that we generate the same hash code regardless of the order in which the user provides the attributes, without needing to sort them. Additionally, it was designed to retain the hash code to prevent recalculating it repeatedly. I agree that we should address the collision handling logic here. If you're interested in contributing, feel free to proceed; otherwise, I can take a look at this later.

@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

3 participants