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.

Sign up for GitHub

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

Jump to bottom

[processor/cumulativetodelta] Convert cumulative Histograms to delta temporality (#12423) #12563

Merged
merged 15 commits into from
Aug 3, 2022
Merged

[processor/cumulativetodelta] Convert cumulative Histograms to delta temporality (#12423) #12563

merged 15 commits into from
Aug 3, 2022

Conversation

mistodon
Copy link
Contributor

Description: Cumulative Histogram metrics will now be converted to delta temporality and are no longer skipped. (This means that consumers who currently rely on Histograms being skipped would need to ensure they are excluded via config.)

Link to tracking Issue: #12423

Testing: Extended unit tests to cover Histogram metrics.

Documentation: Updated README to reflect new capability.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 18, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@mistodon mistodon marked this pull request as ready for review July 19, 2022 12:27
@mistodon mistodon requested a review from a team July 19, 2022 12:27
@mistodon mistodon requested a review from TylerHelmuth as a code owner July 19, 2022 12:27
TylerHelmuth
TylerHelmuth reviewed Jul 19, 2022
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for taking this on.

processor/cumulativetodeltaprocessor/README.md Outdated Show resolved Hide resolved
processor/cumulativetodeltaprocessor/internal/tracking/identity_test.go Show resolved Hide resolved
processor/cumulativetodeltaprocessor/processor.go Outdated Show resolved Hide resolved
processor/cumulativetodeltaprocessor/processor.go Outdated Show resolved Hide resolved
processor/cumulativetodeltaprocessor/processor.go Outdated Show resolved Hide resolved
processor/cumulativetodeltaprocessor/processor.go Show resolved Hide resolved
processor/cumulativetodeltaprocessor/processor.go Outdated Show resolved Hide resolved
unreleased/issue-12423.yaml Show resolved Hide resolved
TylerHelmuth
TylerHelmuth reviewed Jul 21, 2022
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Can you update the README with details about the feature flag? Let's plan to remove it and make its functionality default no sonnet than release 0.60.0.

@mistodon
Copy link
Contributor Author

Done. I wasn't sure how specific you wanted me to be with the timeline, so please feel free to edit/ask me to edit it.

TylerHelmuth
TylerHelmuth approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

One nit, otherwise looks good.

processor/cumulativetodeltaprocessor/README.md Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

@codeboten ready for a second review.

@mistodon mistodon marked this pull request as draft July 22, 2022 16:19
@mistodon
Copy link
Contributor Author

Unfortunately, we think we've found a bug with this implementation. We're planning to investigate more on Monday, but for now, it's probably a good idea not to merge this. Sorry for the abrupt delay.

@mistodon mistodon marked this pull request as ready for review July 26, 2022 08:42
@mistodon
Copy link
Contributor Author

Apologies again - we took a pretty thorough look into the bug and managed to establish that the issue occurs before reaching cumulative-to-delta. And in fact, it's equally possible to trigger the bug with sums and not histograms.

I've opened this for review again. 🙏

@TylerHelmuth
Copy link
Member

If applicable, can you open an issue to track the bug?

@mistodon
Copy link
Contributor Author

Will do, as soon as I can verify that it's actually in the collector and not some other part of our pipeline 🙂

@TylerHelmuth
Copy link
Member

@dashpole ready for a second review

@mistodon
Copy link
Contributor Author

If applicable, can you open an issue to track the bug?

This is the bug in question, for those curious: #12746 - though it's not related to this PR in any specific way.

@TylerHelmuth
Copy link
Member

@open-telemetry/collector-contrib-approvers please review.

@TylerHelmuth
Copy link
Member

@mistodon please resolve conflicts caused by the removal of the Deprecated metrics config.

@mistodon
Copy link
Contributor Author

mistodon commented Aug 3, 2022

Done 🙂

@jpkrohling jpkrohling merged commit 979e0c1 into open-telemetry:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

@codeboten codeboten Awaiting requested review from codeboten

@dashpole dashpole Awaiting requested review from dashpole

Assignees

@codeboten codeboten

@dashpole dashpole

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mistodon @TylerHelmuth @jpkrohling @codeboten @dashpole