chore: IIS Modernization #1542
chore: IIS Modernization #1542
Conversation
Dasomeone
left a comment
Dasomeone
left a comment
There was a problem hiding this comment.
Only comment so far here is to perhaps reuse the common-lib panels a tad more, but overall I'm happy enough with this as-is.
It's cookie-cutter yes, and I remember that was something we discussed originally as well (same as some panels being "noisy" but we actually want to show 0 values in this one)
I have no real suggestions for improvements here, aside from potentially duplicating some panels as stat panels for a top row, but let's discuss in our sync
microsoft-iis-mixin/panels.libsonnet
Outdated
|
|
||
| // Overview - Requests | ||
| requests: | ||
| commonlib.panels.generic.timeSeries.base.new( |
There was a problem hiding this comment.
Any chance this could be converted to the common requests panel? https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/panels/requests/timeSeries
microsoft-iis-mixin/panels.libsonnet
Outdated
| + g.panel.timeSeries.options.legend.withAsTable(true), | ||
|
|
||
| requestErrors: | ||
| commonlib.panels.generic.timeSeries.base.new( |
There was a problem hiding this comment.
Same here,there's a preset for request errors https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/panels/requests/timeSeries
aalhour
left a comment
aalhour
left a comment
There was a problem hiding this comment.
I have a few requests for changes 🙏
- There is a bug with an existing alert that you didn't touch and I added it to the issue https://github.com/grafana/cloud-onboarding/issues/9972 as an example.
- There are a few instances of
increase(...)function not using the proper interval with offset, i.e.:[$__interval:] offset $__interval - There are a few counter signals missing the
rangeFunctionbut a couple of others have it - There is a potential issue in the
config.libsonnetfile - A few other minor comments
| blockedAsyncIORequests: { | ||
| name: 'Blocked async I/O requests', | ||
| type: 'counter', | ||
| rangeFunction: 'increase', |
| // Requests signals | ||
| requests: { | ||
| name: 'Requests per second', | ||
| type: 'counter', |
There was a problem hiding this comment.
Does this counter signal need a rangeFunction as well, similar to blockedAsyncIORequests?
There was a problem hiding this comment.
By default counters will default to the rate(...)[$__rate_interval] function. It was intentionally not placed but can be explicitly set to rate if you'd like!
|
|
||
| lockedErrors: { | ||
| name: 'Locked errors per second', | ||
| type: 'counter', |
There was a problem hiding this comment.
Does this counter signal need a rangeFunction as well, similar to blockedAsyncIORequests?
|
|
||
| notFoundErrors: { | ||
| name: 'Not found errors per second', | ||
| type: 'counter', |
There was a problem hiding this comment.
Does this counter signal need a rangeFunction as well, similar to blockedAsyncIORequests?
There was a problem hiding this comment.
Same comment about default counter => rate
| // Data transfer signals | ||
| bytesSent: { | ||
| name: 'Bytes sent per second', | ||
| type: 'counter', |
There was a problem hiding this comment.
Does this counter signal need a rangeFunction as well, similar to blockedAsyncIORequests?
| unit: 'percent', | ||
| sources: { | ||
| prometheus: { | ||
| expr: 'increase(windows_iis_server_metadata_cache_hits_total{%(queriesSelector)s}[$__interval:]) / clamp_min(increase(windows_iis_server_metadata_cache_queries_total{%(queriesSelector)s}[$__interval:]),1) * 100', |
There was a problem hiding this comment.
Same comment about [$__interval:], it should be [$__interval:] offset $__interval
| unit: 'percent', | ||
| sources: { | ||
| prometheus: { | ||
| expr: 'increase(windows_iis_server_output_cache_hits_total{%(queriesSelector)s}[$__interval:]) / clamp_min(increase(windows_iis_server_output_cache_queries_total{%(queriesSelector)s}[$__interval:]), 1) * 100', |
There was a problem hiding this comment.
Same comment about [$__interval:], it should be [$__interval:] offset $__interval
| panels.uriCacheHitRatio { gridPos+: { w: 12 } }, | ||
| panels.metadataCacheHitRatio { gridPos+: { w: 12 } }, | ||
| panels.outputCacheHitRatio { gridPos+: { w: 12 } }, | ||
| ]), |
There was a problem hiding this comment.
Is this row used anywhere?
There was a problem hiding this comment.
Ah another line I missed, sincere apologies are in order 👎
microsoft-iis-mixin/panels.libsonnet
Outdated
| + g.panel.timeSeries.standardOptions.withUnit('requests') | ||
| + g.panel.timeSeries.standardOptions.withMin(0) | ||
| + g.panel.timeSeries.options.legend.withPlacement('bottom') | ||
| + g.panel.timeSeries.options.tooltip.withMode('none'), |
There was a problem hiding this comment.
Should this be tooltip.withMode('multi') instead?
There was a problem hiding this comment.
Original had it as none was the original reason that this was set as none
I do however think we should use the commonlib defaults here though as I see no original design decision to modify the default.
| ] | ||
| ) | ||
| + g.panel.timeSeries.panelOptions.withDescription('The number of current connections to an IIS site.') | ||
| + g.panel.timeSeries.options.legend.withPlacement('bottom'), |
There was a problem hiding this comment.
Should this panel also define a withMin(0) similar to the blockedAsyncIORequests panel?
There was a problem hiding this comment.
I actually think we should just drop that override from the commonlib in these cases. Will go ahead and do that as I'm not sure of the scenarios where we are using + g.panel.timeSeries.standardOptions.withMin(0) actually would benefit from the setting.
Dasomeone
left a comment
Dasomeone
left a comment
There was a problem hiding this comment.
Went back to this with fresh eyes and found a couple things, but overall I think structure is fine given what this mixin is
3 participants
Screenshots:
Microsoft IIS overview:
Microsoft IIS applications:
