Workaround otel histogram options #170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The commit works around the disconnect between the OTEL metrics Meter interface for creating histograms and the actual Aggregator used to collect and report on the data. Presently, there is no way to pass bucket definitions to the aggregator except globally for all histograms. This commit resolves that by allowing for the use of a custome AggregatorSelector that caches any options and looks them up when a new Aggregator is requested by the OTEL controller.
slizco
approved these changes
Feb 1, 2022
There was a problem hiding this comment.
I think this makes sense. I wonder if we may also want to provider an exported DefaultBoundaries and/or use those boundaries for NewHistogram. As you mentioned, most histograms are measuring a distribution of request latencies, so they're likely to be in the range of 0-30s, with most data points falling under 1s. We can add that later though if we find ourselves repeating boundary definitions.
Is there anything preventing us from merging this soon? I believe it does not conflict with existing histogram functionality.
1 task
voidlock
force-pushed
the
workaround-otel-histogram-options
branch
from
0d9b9d2
to
c4173e6
Compare
voidlock
force-pushed
the
workaround-otel-histogram-options
branch
from
8ff1e5b
to
8389709
Compare
wchrisjohnson
approved these changes
Feb 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The commit works around the disconnect between the OTEL metrics Meter interface for creating histograms and the actual Aggregator used to collect and report on the data. Presently, there is no way to pass bucket definitions to the aggregator except globally for all histograms.
This commit resolves that by allowing for the use of a custom
AggregatorSelectorthat caches any options and looks them up when a newAggregatoris requested by the OTEL controller.At this point in time, I am leaving everything but the OTEL provider implementation alone. If you use use the
ExplicitHistogramit will simply create an older style Histogram with a bucket count set to the length of the boundaries array.