-
Notifications
You must be signed in to change notification settings - Fork 178
chore: IIS Modernization #1542
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
base: master
Are you sure you want to change the base?
chore: IIS Modernization #1542
Conversation
| "uid": "${prometheus_datasource}" | ||
| }, | ||
| "expr": "rate(windows_iis_files_sent_total{job=~\"$job\", instance=~\"$instance\", site=~\"$site\"}[$__rate_interval])", | ||
| "expr": "increase(windows_iis_files_sent_total{job=\"integrations/iis\",job=~\"$job\",instance=~\"$instance\", site=~\"$site\"}[$__interval:] offset -$__interval)", |
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.
Changed this to increase as a file/seconds seems a tad less useful that just knowing the files transferred
Dasomeone
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 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', |
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 like this one!
| // Requests signals | ||
| requests: { | ||
| name: 'Requests per second', | ||
| type: 'counter', |
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.
Does this counter signal need a rangeFunction as well, similar to blockedAsyncIORequests?
|
|
||
| lockedErrors: { | ||
| name: 'Locked errors per second', | ||
| type: 'counter', |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this counter signal need a rangeFunction as well, similar to blockedAsyncIORequests?
| // Data transfer signals | ||
| bytesSent: { | ||
| name: 'Bytes sent per second', | ||
| type: 'counter', |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this row used anywhere?
| + 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be tooltip.withMode('multi') instead?
| ] | ||
| ) | ||
| + 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this panel also define a withMin(0) similar to the blockedAsyncIORequests panel?
Screenshots:
Microsoft IIS overview:


Microsoft IIS applications:

