-
Notifications
You must be signed in to change notification settings - Fork 103
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
Switch to uint64 hash for identifier #886
Switch to uint64 hash for identifier #886
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 61.03% 62.97% +1.93%
==========================================
Files 56 57 +1
Lines 5903 6012 +109
==========================================
+ Hits 3603 3786 +183
+ Misses 2143 2064 -79
- Partials 157 162 +5 ☔ View full report in Codecov by Sentry. |
FYI: Check an amazing https://pkg.go.dev/golang.org/x/perf/cmd/benchstat tool for easier comparision of those results 🤗 Also you can use the following command for (1) saving to file (2) do enough repetition for
|
@@ -212,27 +212,57 @@ func (c *Cache) gc(shutdown <-chan struct{}, tickerCh <-chan time.Time) bool { | |||
} | |||
|
|||
// Identifier returns the unique string identifier for a metric. | |||
func Identifier(resource *monitoredrespb.MonitoredResource, extraLabels map[string]string, metric pmetric.Metric, attributes pcommon.Map) string { | |||
var b strings.Builder | |||
func Identifier(resource *monitoredrespb.MonitoredResource, extraLabels map[string]string, metric pmetric.Metric, attributes pcommon.Map) (uint64, error) { |
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.
I don't have context on other parts of Otel collector code, but storing those in maps is generally not ideal for efficiency. Especially as those attributes are short enough to be iterated to find items you are looking for etc
This is why Prometheus primary labels code started as array and now it's tightly optimized interned and encoded string 🙈 https://github.com/prometheus/prometheus/blob/main/model/labels/labels_dedupelabels.go#L27
Just ideas for future if even applicable.
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.
This is a solid start 👍🏽 Good work!
7bbdf20
to
da58aa2
Compare
added delimiters to make sure we don't get collisions between the different maps |
I'm still working on testing edge cases to make sure identifiers don't collide.