-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(dynatraceexporter): provide better estimated summaries for partial histograms #11044
feat(dynatraceexporter): provide better estimated summaries for partial histograms #11044
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial pass, overall looks good! I think we only need to add more tests to make sure the sum
estimation works.
// range for counts[i] is bounds[i-1] to bounds[i] | ||
|
||
// min estimation | ||
if !foundNonEmptyBucket { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum.. not sure I get the motive of this var here? Couldn't we just check if it's i == 0
it will only go inside once..🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first non-empty bucket might not be the first bucket.
The CI failure is in "check-codeowners" but it isn't at all obvious what that check is even doing to me |
Load and stability tests have been running for ~3 hours. is that right? |
nah there is an issue. I recommend taking latest from main to force another round of actions. |
I'm pretty sure the CI failures are unrelated |
Dynatrace exporter converts histograms to stat summary gauges. This PR improves the estimation in the following ways: