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

delta-to-cumulative-processor: initial #28910

Conversation

0x006EA1E5
Copy link

Description:

Added initial draft of Delta to Cumulative Processor

Allows converting delta metrics to cumulative metrics

Link to tracking Issue: TODO

Testing: TODO

Documentation: TODO

@0x006EA1E5 0x006EA1E5 requested review from a team and TylerHelmuth November 6, 2023 18:05
@0x006EA1E5 0x006EA1E5 marked this pull request as draft November 7, 2023 09:04
@0x006EA1E5
Copy link
Author

@djaglowski

@atoulme
Copy link
Contributor

atoulme commented Nov 10, 2023

Please open an issue for this new component. New components require sponsors. Please read CONTRIBUTING.md for more information.

@djaglowski
Copy link
Member

@0x006EA1E5, thanks for the PR. I have this on my list to give you some initial feedback, but as mentioned above, you'll need to formerly propose the component before we can proceed with anything more than that. I will sponsor the component.

Comment on lines +29 to +32
- `identity_mode`: How to create a metrics identity signature. Options are
- `SCOPE_AND_RESOURCE_ATTRIBUTES`: Use both the metrics `scope` and `resource` attributes
- `SCOPE_ATTRIBUTES_ONLY`: Use only the metrics `scope` attributes
- n.b., if metrics will be exported wit the `prometheus` or `prometheus remote write` exporters, this should be set to work will with the exporter's `resource_to_telemetry_conversion` configuration
Copy link
Member

Choose a reason for hiding this comment

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

I'm having a hard time grappling with this part of the component. Typically, I would expect that we aggregate without changing anything at all about the resource or scope. i.e. We always use both the resource and scope attributes to uniquely identify the set of timeseries we're aggregating.

Comment on lines +21 to +22
// resourceAttrs contain the resource attributes. They are used to output instance and job labels.
resourceAttrs pcommon.Map
Copy link
Member

Choose a reason for hiding this comment

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

Related to my previous comment, I would expect we maintain both resource and scope attributes separately.

for i := 0; i < dps.Len(); i++ {
ip := dps.At(i)

signature := timeseriesSignature(a.identifyMode, il.Name(), metric, ip.Attributes(), resourceAttrs)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than create a signature, would it make sense to define a hierarchical data type which mirrors the result we will emit? A little oversimplified, but roughly:

type timeseriesManager struct {
    resources map[pcommon.Map] resource // resource attributes => resource
}

type resource struct {
    lastUpdated time.Time
    attributes pcommon.Map
    scopes map[pcommon.Map]scope // scope attributes => scope
}

type scope struct {
    lastUpdated time.Time
    attributes pcommon.Map
    name string
    metrics map[string]metric // metric name => metric 
}

type metric struct {
    lastUpdated time.Time
    pmetric.Metric // stored as reference so we can recreate the metric as cumulative
    timeseries map[pcommon.Map]timeseries // data point attributes => timeseries
}

type timeseries {
    lastUpdated time.Time
    // accumulated data points
}

Copy link
Contributor

github-actions bot commented Dec 1, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 1, 2023
@cwegener
Copy link
Contributor

I may not be aware of all recent discussions around the cumulativetodelta processor (which this new one seems to be very closely related to), but there was this comment a while back:

Yes I would love for this capability to exist. The reason it hasn't been implemented yet is that this processor will hopefully be replaced some day by a stateful version of the transform processor. I've been hesitant to make changes to it because I'd like to deprecate it; any functionality added is functionality that has to be moved over to the new processor.

In this case tho, I feel like this is an important enough feature (and no one has started on a stateful transform processor yet 😭) that I think we could add it now to the cumulativetodelta processor. Is it something you plan to work on?

#12423 (comment)

@github-actions github-actions bot removed the Stale label Dec 12, 2023
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 26, 2023
Copy link
Contributor

github-actions bot commented Jan 9, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 9, 2024
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.

5 participants