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

[datadogexporter] Fix cumulative histogram handling in distributions mode #5867

Merged

Conversation

KSerrania
Copy link
Contributor

Description:

Fixes an issue in mapHistogramMetrics where the delta boolean wouldn't be passed to getSketchBuckets when the histograms mode is set to distributions.
Fixes an issue in getSketchBuckets where buckets were not tagged by bounds when getting and setting values in the points cache (in the cumulative histogram case). This led to all buckets of an histogram sharing the same cache key, making the resulting sketch wrong.
Fixes the TestNaNMetrics histogram case, which was missing its ExplicitBounds definition.

Link to tracking Issue: n/a

Testing: Added missing unit tests for the distributions mode to cover the bugged cases.

Documentation: n/a

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor comment (that we can deal with in a general test refactor PR if you want to)

@KSerrania KSerrania marked this pull request as ready for review October 22, 2021 09:01
@KSerrania KSerrania requested review from a team and dmitryax October 22, 2021 09:01
KSerrania added a commit to DataDog/datadog-agent that referenced this pull request Oct 22, 2021
…9623)

Backport of open-telemetry/opentelemetry-collector-contrib#5867 to the `pkg/otlp` package.

Fixes an issue in `mapHistogramMetrics` where the `delta` boolean wouldn't be passed to `getSketchBuckets` when the histograms mode is set to distributions.
Fixes an issue in `getSketchBuckets` where buckets were not tagged by bounds when getting and setting values in the points cache (in the cumulative histogram case). This led to all buckets of an histogram sharing the same cache key, making the resulting sketch wrong.
Fixes the `TestNaNMetrics` histogram case, which was missing its `ExplicitBounds` definition.
Adds missing unit tests for the distributions mode.
KSerrania added a commit to DataDog/datadog-agent that referenced this pull request Oct 22, 2021
…9623)

Backport of open-telemetry/opentelemetry-collector-contrib#5867 to the `pkg/otlp` package.

Fixes an issue in `mapHistogramMetrics` where the `delta` boolean wouldn't be passed to `getSketchBuckets` when the histograms mode is set to distributions.
Fixes an issue in `getSketchBuckets` where buckets were not tagged by bounds when getting and setting values in the points cache (in the cumulative histogram case). This led to all buckets of an histogram sharing the same cache key, making the resulting sketch wrong.
Fixes the `TestNaNMetrics` histogram case, which was missing its `ExplicitBounds` definition.
Adds missing unit tests for the distributions mode.
@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Oct 22, 2021
@bogdandrutu bogdandrutu merged commit 412efc2 into open-telemetry:main Oct 22, 2021
zandrewitte pushed a commit to StackVista/stackstate-agent that referenced this pull request Nov 17, 2022
…ataDog#9623)

Backport of open-telemetry/opentelemetry-collector-contrib#5867 to the `pkg/otlp` package.

Fixes an issue in `mapHistogramMetrics` where the `delta` boolean wouldn't be passed to `getSketchBuckets` when the histograms mode is set to distributions.
Fixes an issue in `getSketchBuckets` where buckets were not tagged by bounds when getting and setting values in the points cache (in the cumulative histogram case). This led to all buckets of an histogram sharing the same cache key, making the resulting sketch wrong.
Fixes the `TestNaNMetrics` histogram case, which was missing its `ExplicitBounds` definition.
Adds missing unit tests for the distributions mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants