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

Add proper buckets for request/response sizes. #73

Merged
merged 2 commits into from
Apr 5, 2022

Conversation

thebaron
Copy link

@thebaron thebaron commented Mar 1, 2022

One of the issues I've noticed is that the request size uses the defaults for prom, which are time based, and not size based. This results in files being bucketed on a range from .0005 bytes to 10 bytes in length ... not what we want. This adds some a simplification for file size calculation and the ability to specify the correct bucketing.

- default buckets in prom are for time, not transfer.
- adjusts code to create proper bucketing for traffic sizes.

    - default buckets in prom are for time, not transfer.
    - adjusts code to create proper bucketing for traffic sizes.
@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #73 (7fd3638) into master (79225fb) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   59.46%   59.43%   -0.03%     
==========================================
  Files           8        8              
  Lines         671      673       +2     
==========================================
+ Hits          399      400       +1     
- Misses        251      252       +1     
  Partials       21       21              
Impacted Files Coverage Δ
prometheus/prometheus.go 51.93% <50.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79225fb...7fd3638. Read the comment docs.

    - adds some tests to help assure us that the buckets have the right
      measurement for the metric (seconds->time, request/response->size).
@thebaron
Copy link
Author

thebaron commented Mar 9, 2022

o/ I see this resulted in a reduction of test coverage by 3/100 of a %. The changes are minor but I tried to come up with a test that might help give us some assurances/confidence that they are being handled correctly. It tests to ensure that the various metrics have the right "unit" bucket and do not have the wrong one. The latter test seems silly, one supposes, but I added it mostly to rule out the possibility of a hungry regex matching on another line's bucket and ensuring it only matches its own le value.
Hopefully this will help with the numbers a bit.

@thebaron
Copy link
Author

Just wanted to follow up one more time on this PR...

@aldas aldas merged commit ed09afe into labstack:master Apr 5, 2022
@aldas
Copy link
Contributor

aldas commented Apr 5, 2022

Sorry for the delay

@thebaron
Copy link
Author

No worries, I just wanted
to make sure that I had done everything possible and wasn't missing something or someone was not waiting on me for something! Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants