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/cumulativetodelta] Convert cumulative Histograms to delta temporality #12423

Closed
mistodon opened this issue Jul 14, 2022 · 5 comments
Closed
Assignees
Labels
priority:p2 Medium processor/cumulativetodelta Cumulative To Delta processor

Comments

@mistodon
Copy link
Contributor

Is your feature request related to a problem? Please describe.
It seems that cumulative-to-delta at the moment only processes Sums, and just skips over other data types. This is a problem for us, as we have Histograms in our pipeline with cumulative temporality that we want to convert.

Describe the solution you'd like
Rather than bypass Histogram metrics, have the cumulative-to-delta processor convert their data points in a similar way to sums.

Describe alternatives you've considered
None, other than continuing to ignore non-Sum metrics.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jul 14, 2022

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?

@TylerHelmuth TylerHelmuth changed the title [cumulativetodeltaprocessor] Convert cumulative Histograms to delta temporality [processor/cumulativetodelta] Convert cumulative Histograms to delta temporality Jul 14, 2022
@TylerHelmuth TylerHelmuth added processor/cumulativetodelta Cumulative To Delta processor priority:p2 Medium labels Jul 14, 2022
@mistodon
Copy link
Contributor Author

Thanks for the quick update! That makes a lot of sense - but given that the new processor isn't looking like it'll be ready super soon, I agree it's worth adding now.

I have started looking at implementing this. I'm not extremely familiar with the codebase or anything, but once I have something, I can open a draft PR for feedback if that sounds good?

@TylerHelmuth
Copy link
Member

Sounds great

@mistodon
Copy link
Contributor Author

There's definitely some things that could be refactored in the above PR, but I want to make sure people are happy with the general approach before I do that.

@TylerHelmuth
Copy link
Member

I think your approach is good. I would say to switch to using a struct instead of a list for the 3 histogram identities, that way we don't have to use any index assumptions.

Ping me when you are ready for a full review. Thanks for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium processor/cumulativetodelta Cumulative To Delta processor
Projects
None yet
Development

No branches or pull requests

2 participants