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

Update varnish5_ improving request_rate, transfer_rates #1304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cantoute
Copy link

  • request_rate

    • major reordering. making possible to read pass rates evolutions and rejected requests visible
    • small fix: sess_conn not client_conn in the order
  • transfer_rates

    • using AREA/STACK

- request_rate
  - major reordering. making possible to read pass rates evolutions and rejected requests visible
  - small fix: sess_conn not client_conn in the order

- transfer_rates
  - using AREA/STACK
Copy link
Collaborator

@sumpfralle sumpfralle left a comment

Choose a reason for hiding this comment

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

Thank you for your suggestions!

Please take a look at my comments below.

'min' => '0',
'colour' => '444444',
'colour' => 'FF5D00',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these color changes just your personal preference or objective improvements?
In case of doubt, I would prefer to keep them unchanged.

'args' => '-l 0',
'vlabel' => 'bit/s',
'values' => {
's_resp_hdrbytes' => {
'type' => 'DERIVE',
'label' => 'Header traffic',
'draw' => 'STACK',
'draw' => 'AREA',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change intentional? Or did you mean AREASTACK?

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

I had made a mail explaining a bit more what I did here and I had sent it to the last person what had maintained varnish5_ @kjetilho

On request_rates the change of color is required, the changes are far deeper than color changes.
The graph is the oposite of what it was so it would be posible to visualize passed and missed requests. Instead of having them stacked on top of hits, here I stack hits (big figure) on top of passed and missed (small numbers)

transfer_rates, in the same logic, lets have small numbers to be the first ones stacked, big ones on top.

having small figures stacked on top of big ones (like cache hits) makes them impossible to visualise. Hit rate can go from 1 to 200 in the course of a day, if one stacks on top something that varies between 0.5 and 3, the frange it creates on top of huge mountains isn't readable, and in practice it's the passed and miss we want to monitor, not the hits (hits aren't painful to backend and the hit-rate graph gives us what we need to know on hits, over time one wants to keep an eye on the evolution over time of 'pass' and 'miss')

Perhaps you want to have a look at the result
low trafic
https://munin.adm.ofil.fr/localdomain/localhost.localdomain/varnish5_request_rate.html
https://munin.adm.article3.fr/localdomain/localhost.localdomain/varnish5_request_rate.html

more serious trafic
https://munin.adm.welovetennis.fr/localdomain/localhost.localdomain/varnish5_request_rate.html

Copy link
Author

@cantoute cantoute Mar 29, 2022

Choose a reason for hiding this comment

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

Is this change intentional? Or did you mean AREASTACK?

not sure I'm getting your question.
the plugin used AREA to define first graph and STACK for those that pile on top.
I just swapped them. The header traffic is very small compared to body, stacking it on top makes the graph useless.

Did I miss something ?

Copy link
Author

Choose a reason for hiding this comment

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

hi,
Looks like I had miss-understood sess_conn and working at fixing this.

I'll need a bit more test time and will come back with a new pull request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the plugin used AREA to define first graph and STACK for those that pile on top.

Maybe it helps to simplify the code: you can also use AREASTACK for all fields. The old distinction between the first and the following fields is not necessary anymore.

I'll need a bit more test time and will come back with a new pull request.

Feel free to recycle this PR by force-pushing into it. Or just close this one and open a new one.
Thanks!

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

Successfully merging this pull request may close these issues.

2 participants