Skip to content

feat: add support for SummaryDataPoint in delta to cumulative processor #38779

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

Conversation

lightme16
Copy link

@lightme16 lightme16 commented Mar 19, 2025

Description

Adding support for Delta to Cumulatives for summary data points

Link to tracking issue

Fixes #38772

Testing

Added new unit tests for summary

Documentation

@lightme16 lightme16 requested a review from a team as a code owner March 19, 2025 02:29
@lightme16 lightme16 requested a review from edmocosta March 19, 2025 02:29
Copy link

linux-foundation-easycla bot commented Mar 19, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@atoulme
Copy link
Contributor

atoulme commented Mar 19, 2025

Please add tests to cover the new functionality?

@atoulme atoulme marked this pull request as draft March 19, 2025 04:02
@lightme16
Copy link
Author

Please add tests to cover the new functionality?

@atoulme, I can definitely do that. Thank you! However, before I invest further, could you please review the business logic? Are you satisfied with this functionality? Do you agree that sum & count metrics in the summary should be aggregated? I believe they should be, especially for our use case. However, I’m not sure if there are known issues that prevented this from being implemented in the past. If this functionality is indeed missing, we’re happy to add it, but I want to confirm with the community first.

More in the #38772

@lightme16 lightme16 force-pushed the deltatocumulativeprocessor-summary branch from 86cab52 to b356e34 Compare March 28, 2025 01:56
@lightme16
Copy link
Author

@sh0rez, @ArthurSens, and @tedsuo: I just added a feature flag to disable the summary accumulation functionality by default. Please share your thoughts on the merge request. My company needs this to move forward, and it would be great to have it merged. Let me know if anything else broke while processing this for merging.

Thanks!

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 Apr 19, 2025
@lightme16
Copy link
Author

hello, any thoughts on MR - @sh0rez, @ArthurSens, and @tedsuo? thanks

@tombrk
Copy link
Member

tombrk commented Apr 29, 2025

hey @lightme16, I apologize for the much too long waiting time. I was on pto and did not get back to this earlier.

while I do think you are onto a very valid use-case we should absolutely enable, I am not comfortable with deltatocumulative relying on behavior that is not part of the specification.

as per the spec, the Summary data point has no aggregation temporality. your example however shows it absolutely should have such information attached, because .sum and .count can clearly be delta.

ideally, I imagine the following:

  • spec change to make SummaryDataPoint have AggregationTemporality
  • make statsdreceiver set above to delta
  • adapt and merge this PR

are you interested in continuing this work?

@lightme16
Copy link
Author

Hello, @sh0rez I decided to move away from that summaries approach in my system due to this issue and use new native Prometheus histograms instead. I'm not interested in doing any more work on this, but I still believe it would be beneficial to have this patch merged and implemented. Perhaps someone else could take it on

Copy link
Contributor

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

Copy link
Contributor

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

@github-actions github-actions bot closed this May 29, 2025
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.

[deltatocumulativeprocessor] Add summary type support
4 participants