-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[internal/exp/metrics] Add functions to merge metrics #32794
Conversation
/label skip-changelog |
If someone can apply the |
@@ -61,3 +65,15 @@ func (m HashMap[T]) Clear() { | |||
type Evictor interface { | |||
Evict() identity.Stream | |||
} | |||
|
|||
type DataPointSlice[DP DataPoint[DP]] interface { |
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.
The changes in this file seemed unrelated to this pr?
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.
It's used in the datapoint merge function: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32794/files#diff-756513e8ec6edd51e0dc85ef3e3756b9674a2929328395fc63e806e9d052da46R113
I'd also like to migrate processor/intervalprocessor
to use this shared definition, instead of having a duplicate interface in multiple places. https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/intervalprocessor/internal/metrics/metrics.go
Any further questions or concerns? |
@RichieSams could you fix the conflicts and failed lint checks? |
Are we ok to merge? @jpkrohling we could always move it to pdatautil in a separate pass, if others agree on that point |
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.
It's OK to have this, especially as it's part of the internal API for this repo. Once we've seen a few places consuming this package, we can determine whether their usage deserves keeping it, or dealing with pdata directly.
Ok we're good to merge now. CI is green 🥳 |
Bump |
Description:
This will merge the metrics in mdB into mdA, trying to re-use resourceMetrics, scopeMetrics, and metric values as possible. This will be used to help implement the new feature for: #32513
Link to tracking Issue: #32513 / #32690
Testing:
I created a unit test which tests various scenarios of how merge behavior should happen
Documentation:
The exported function is documented using standard golang style. And there are comments inside the code to explain what is going on and why