-
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
[pkg/translate/promremotewrite] downscale exponential histograms for prometheus #24026
[pkg/translate/promremotewrite] downscale exponential histograms for prometheus #24026
Conversation
|
@krajorama please sing the CLA |
This comment was marked as outdated.
This comment was marked as outdated.
After some optimizations I got this benchmark compared to baseline:
Looking at the profile, half the time is spent in reallocating the deltas and probably spans slices. That could be something to look at , although for small numbers it's probably fine. |
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.
lgtm!
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.
I verified the tests, checking that the changes aren't breaking.
CI is failing with an unrelated change, I'll rebase once the fix is merged. |
Signed-off-by: György Krajcsovits <[email protected]>
Add unit test to check that upscaling is still rejected and downscaling happens above scale 8. Test edges 8 and 9. Further detailed tests to follow in TestConvertBucketsLayout. Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Add a new parameter that will be used to tell the function how many buckets to merge when converting the bucket layouts. Not use yet. Add disabled test cases. Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Saves about 10%. Signed-off-by: György Krajcsovits <[email protected]>
We don't need to carry nextBucketIdx outside the loop since we can easily recalculate its value. Signed-off-by: György Krajcsovits <[email protected]>
Co-authored-by: Ruslan Kovalov <[email protected]>
70ae915
to
af9e160
Compare
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.
Couple nits and a question. I'd approve but for the question about why apparently unrelated test changes seem to have been made.
Co-authored-by: Anthony Mirabella <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Undo previous mess. Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Hoping this will resolve our issues with prometheus histograms 🙏 Putting the error we have seen here for visibility.
|
…usremotewrite Get open-telemetry/opentelemetry-collector-contrib#24026 Signed-off-by: György Krajcsovits <[email protected]>
* vendor update opentelemetry-collector-contrib/pkg/translator/prometheusremotewrite Get open-telemetry/opentelemetry-collector-contrib#24026 Signed-off-by: György Krajcsovits <[email protected]> * Update CHANGELOG.md Co-authored-by: Charles Korn <[email protected]> --------- Signed-off-by: György Krajcsovits <[email protected]>
Description:
Implement down-scaling of exponential histograms to Prometheus native histograms in Prometheus remote writer.
Configuration of down-scaling is TBD.
Link to tracking Issue:
Fixes: #17565
Testing:
Unit tests.
Documentation:
TBD