Update Varnish Mixin to use modern libraries #1480
Conversation
varnish-mixin/panels.libsonnet
Outdated
| signals.backend.backendConnectionsAccepted.asTarget(), | ||
| signals.backend.backendConnectionsRecycled.asTarget(), | ||
| signals.backend.backendConnectionsReused.asTarget(), | ||
| signals.backend.backendConnectionsBusy.asTarget(), | ||
| signals.backend.backendConnectionsUnhealthy.asTarget(), |
There was a problem hiding this comment.
All these signals previously used the irate() function and I'm not quite sure how useful it is to know rate per second of the connection... I've currently translated as originally implemented but it seemed like we should maybe be displaying using increase. Happy to hear other thoughts, but just figured that knowing a whole number connection value here would be more user friendly/intuitive.
There was a problem hiding this comment.
Agreed, and thank you for calling this out! I believe the later best-practices around the correct offset for increase actually stems from a discussion we had around a bug with the Varnish integration (or feedback related to it at least?) so I'm good with converting these to using increase.
varnish-mixin/panels.libsonnet
Outdated
| signals.sessions.sessionsConnected.asTarget(), | ||
| signals.sessions.sessionsQueued.asTarget(), | ||
| signals.sessions.sessionsDropped.asTarget(), |
There was a problem hiding this comment.
Same thing on usage of irate here which could be useful but feel as though the increase value may be more useful?
There was a problem hiding this comment.
+1, I agree with swapping them.
There was a problem hiding this comment.
Couple comments, this is definitely a mixin that needs a few more things done to it, I remember it was what spawned the desire to establish the best practices doc though I think it only had the first pass applied to it back then so there's a bit more missing :)
| sources: { | ||
| prometheus: { | ||
| expr: 'varnish_main_backend_recycle{%(queriesSelector)s}', | ||
| rangeFunction: 'irate', |
There was a problem hiding this comment.
Like you rightly pointed out, we can convert these to increase() using the proper offset and it should be a little more reliable than irate()
varnish-mixin/rows.libsonnet
Outdated
| // line of stat panels so width is generally small within the grid | ||
| this.grafana.panels.cacheHitRatePanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.frontendRequestsPanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.backendRequestsPanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.sessionsRatePanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.cacheHitsPanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.cacheHitPassPanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.sessionQueueLengthPanel { gridPos+: { w: 3 } }, | ||
| this.grafana.panels.poolsPanel { gridPos+: { w: 3 } }, | ||
|
|
There was a problem hiding this comment.
Please split this into a separate row as we deliberately wanted the stat panels to be 4 units tall, while the current approach ends up being 8 units tall
varnish-mixin/panels.libsonnet
Outdated
| description='Rate of cache hits for pass objects (fulfilled requests that are not cached).' | ||
| ) | ||
| + g.panel.stat.standardOptions.withUnit('/ sec') | ||
| + g.panel.stat.panelOptions.withTransparent(true), |
There was a problem hiding this comment.
Please add this for consistency with the other stat panels
| + g.panel.stat.panelOptions.withTransparent(true), | |
| + g.panel.stat.options.withGraphMode('none') |
| + g.panel.gauge.panelOptions.withTransparent(true), | ||
|
|
||
| // Frontend requests stat | ||
| frontendRequestsPanel: |
There was a problem hiding this comment.
for all the stat panels can you please modify them and the queries to be sums/stats across all instances such that the stat panels do not split into one each? Gets pretty cluttered pretty quickly with multiple instances
This was a complaint we saw with the old Varnish mixin as well so easy to miss as it wasn't something we fixed
| description='Number of failed, created, limited, and current threads.' | ||
| ) | ||
| + g.panel.timeSeries.standardOptions.withUnit('none') | ||
| + g.panel.timeSeries.fieldConfig.defaults.custom.withFillOpacity(0) |
There was a problem hiding this comment.
Reminder here that we want at least withFillOpacity(10), though 20 is perfectly fine as well
Updates the Varnish Mxin to use more modern libaries and signals API
Overview
Logs Overview
Have some questions around why we're using irate and wanted to discuss before merging as of submission of this PR.