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

[processor/deltatocumulative]: Sums #30707

Merged
merged 32 commits into from
Feb 15, 2024

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Jan 22, 2024

Description:
Implements major component functionality:

  • metrics identification (metrics.Ident) and stream identification (streams.Ident)
  • abstract data layer data.Point that keeps processing code data type (sum, histogram, exp histogram) agnostic
  • delta.Accumulator stream processor for accumulating any data.Point

Link to tracking Issue: #30705

Testing: Done

Documentation: TODO

sh0rez added 10 commits January 22, 2024 17:12
internal/metrics: primitives for working on a generalized idea of
metrics

internal/streams: primitives for working on streams

internal/delta: delta sum aggregation
emits aggregated metrics on a configured interval to the next consumer.Metrics
drops ooo support for now, which eliminates the need for sophisticated
grouping that was present before.

this radically simplifies the core part of this exporter.

ooo support will be added back by queueing and sorting delta samples instead.
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Please see comments. I think I identified a bug.

Tests would also be welcome, in order to be able to evaluate the code.

@sh0rez sh0rez marked this pull request as ready for review January 29, 2024 15:24
@sh0rez sh0rez requested a review from a team January 29, 2024 15:24
@sh0rez
Copy link
Member Author

sh0rez commented Feb 15, 2024

@RichieSams would you be interested in becoming second CODEOWNER on this component? More than happy to do the same for your intervalprocessor, keeping the current engineering focus split :)

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.

LGTM, only a couple of minor improvements.

@sh0rez
Copy link
Member Author

sh0rez commented Feb 15, 2024

@djaglowski @RichieSams please take a final look :)

@RichieSams
Copy link
Contributor

@RichieSams would you be interested in becoming second CODEOWNER on this component? More than happy to do the same for your intervalprocessor, keeping the current engineering focus split :)

That's a great idea. Let's do both.

@RichieSams
Copy link
Contributor

@djaglowski @RichieSams please take a final look :)

Code / algorithm looks good to me.

Question: When #31017 is merged, do you plan to refactor this code to utilize the new types?

@sh0rez
Copy link
Member Author

sh0rez commented Feb 15, 2024

Question: When #31017 is merged, do you plan to refactor this code to utilize the new types?

yes absolutely, just didn't want to introduce more blocking inter-dependencies for now!

@jpkrohling jpkrohling merged commit c0fe1b8 into open-telemetry:main Feb 15, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 15, 2024
@sh0rez sh0rez deleted the deltatocumulative branch February 15, 2024 15:26
jpkrohling pushed a commit that referenced this pull request Feb 19, 2024
Adds a new internal, _experimental_ package `metrics/identity` which
implements identity types for resource, scope, metric and stream.

This is closely related to work being done in #30707 and #30827.

The package is specifically experimental, as it shall be treated as an
internal component to above processors which may change at any moment as
long as those are under active initial development.

/cc @jpkrohling @djaglowski @RichieSams
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:**
Implements major component functionality:

- [x] metrics identification (`metrics.Ident`) and stream identification
(`streams.Ident`)
- [x] abstract data layer `data.Point` that keeps processing code data
type (sum, histogram, exp histogram) agnostic
- [x] `delta.Accumulator` stream processor for accumulating any
`data.Point`

**Link to tracking Issue:**
open-telemetry#30705

**Testing:**  Done

**Documentation:** <kbd>TODO</kbd>
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
Adds a new internal, _experimental_ package `metrics/identity` which
implements identity types for resource, scope, metric and stream.

This is closely related to work being done in open-telemetry#30707 and open-telemetry#30827.

The package is specifically experimental, as it shall be treated as an
internal component to above processors which may change at any moment as
long as those are under active initial development.

/cc @jpkrohling @djaglowski @RichieSams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants