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

[internal/exp/metrics] Add functions to merge metrics #32794

Merged
merged 32 commits into from
May 29, 2024

Conversation

RichieSams
Copy link
Contributor

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

@RichieSams RichieSams requested a review from a team as a code owner May 1, 2024 12:43
@RichieSams RichieSams requested a review from songy23 May 1, 2024 12:43
@github-actions github-actions bot requested a review from sh0rez May 1, 2024 12:43
@RichieSams RichieSams changed the title [internal/exp/metrics]: Add functions to merge metrics [internal/exp/metrics] Add functions to merge metrics May 1, 2024
@RichieSams
Copy link
Contributor Author

/label skip-changelog

@RichieSams
Copy link
Contributor Author

If someone can apply the No Changelist label, this should be good for final review and merge

@songy23 songy23 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 1, 2024
@jpkrohling jpkrohling self-requested a review May 7, 2024 15:11
@@ -61,3 +65,15 @@ func (m HashMap[T]) Clear() {
type Evictor interface {
Evict() identity.Stream
}

type DataPointSlice[DP DataPoint[DP]] interface {
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RichieSams
Copy link
Contributor Author

Any further questions or concerns?

@fatsheep9146
Copy link
Contributor

@RichieSams could you fix the conflicts and failed lint checks?

@RichieSams
Copy link
Contributor Author

Are we ok to merge? @jpkrohling we could always move it to pdatautil in a separate pass, if others agree on that point

Copy link
Member

@jpkrohling jpkrohling 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 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.

@RichieSams
Copy link
Contributor Author

Ok we're good to merge now. CI is green 🥳

@RichieSams
Copy link
Contributor Author

Bump

@jpkrohling jpkrohling merged commit f664dbb into open-telemetry:main May 29, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 29, 2024
@RichieSams RichieSams deleted the metrics branch May 29, 2024 15:54
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jul 11, 2024
…#32794)

**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:
open-telemetry#32513


**Link to tracking Issue:**
open-telemetry#32513
/
open-telemetry#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

---------

Co-authored-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants