-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add method Fingerprint to data.Labels #712
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
f827995
to
88e65aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yuri-tceretian, I have some questions, apart from that adding the rest of the @grafana/plugins-platform-backend as reviewer in case they have more feedback.
@@ -80,6 +81,33 @@ func (l Labels) String() string { | |||
return sb.String() | |||
} | |||
|
|||
type Fingerprint uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few reasons:
- standard type is too general and may be confusing to read in the code when passed around in user code.
- simpler to track where the type\value originated.
- add the possibility to extend the type with methods. For example, Stringer interface.
- improves maintainability. If we decide to switch from uint64 to byte array or string, less code will need to be rewritten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I added String() to represent the hash as a sequence of hex numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work! 🎉 I have left one small optional suggestion
Thanks, everyone for the review!! |
What this PR does / why we need it:
This PR adds the method Fingerprint to labels that calculates 64-bit FNV-1 hash of the labels.
Which issue(s) this PR fixes:
Related to grafana/grafana#70998
Special notes for your reviewer: