chore: Discourse Mixin Modernization #1539
Conversation
|
Currently working on runbook for this but think the Mixin should be ready for some review |
Dasomeone
left a comment
Dasomeone
left a comment
There was a problem hiding this comment.
Couple comments, thanks Keith!
| latestMedianRequestTime: { | ||
| name: 'Latest median request time', | ||
| type: 'gauge', | ||
| unit: 's', | ||
| description: 'The median amount of time for "latest" page requests.', | ||
| sources: { | ||
| prometheus: { | ||
| expr: 'discourse_http_duration_seconds{%(queriesSelector)s,quantile="0.5",action="latest"}', | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| topicMedianRequestTime: { | ||
| name: 'Topic median request time', | ||
| type: 'gauge', | ||
| unit: 's', | ||
| description: 'The median amount of time for "topics show" requests.', | ||
| sources: { | ||
| prometheus: { | ||
| expr: 'discourse_http_duration_seconds{%(queriesSelector)s,quantile="0.5",controller="topics"}', | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| latest99thPercentileRequestTime: { | ||
| name: 'Latest 99th percentile request time', | ||
| type: 'gauge', | ||
| unit: 's', | ||
| description: 'The 99th percentile amount of time for "latest" page requests.', | ||
| sources: { | ||
| prometheus: { | ||
| expr: 'discourse_http_duration_seconds{%(queriesSelector)s,quantile="0.99",action="latest"}', | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| topic99thPercentileRequestTime: { | ||
| name: 'Topic 99th percentile request time', | ||
| type: 'gauge', | ||
| unit: 's', | ||
| description: 'The 99th percentile amount of time for "topics show" requests.', | ||
| sources: { | ||
| prometheus: { | ||
| expr: 'discourse_http_duration_seconds{%(queriesSelector)s,quantile="0.99",controller="topics"}', | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Given that we have proper histogram data from the looks of it, what do you think of replacing these with histograms? Or perhaps in addition to the 99th quantile
There was a problem hiding this comment.
Seems like a good idea 👍 will look into it 👀
discourse-mixin/panels.libsonnet
Outdated
| sidekiqJobDuration: | ||
| commonlib.panels.generic.stat.base.new('Sidekiq job duration', targets=[signals.jobs.sidekiqJobDuration.asTarget()]) | ||
| + g.panel.stat.panelOptions.withDescription('Time spent in Sidekiq jobs broken out by job name.') | ||
| + g.panel.stat.standardOptions.withUnit('s'), | ||
|
|
||
| scheduledJobDuration: | ||
| commonlib.panels.generic.stat.base.new('Scheduled job duration', targets=[signals.jobs.scheduledJobDuration.asTarget()]) | ||
| + g.panel.stat.panelOptions.withDescription('Time spent in scheduled jobs broken out by job name.') | ||
| + g.panel.stat.standardOptions.withUnit('s'), |
There was a problem hiding this comment.
I'd love to see these two panels also have right-aligned table legends, with an additional column for max/mode/current duration
schmikei
force-pushed
the
chore/discourse-modernization
branch
from
468dbe1 to
8778033
Compare
schmikei
force-pushed
the
chore/discourse-modernization
branch
from
8778033 to
52beb8e
Compare
aalhour
left a comment
aalhour
left a comment
There was a problem hiding this comment.
A few comments and requests for changes.
| + g.panel.timeSeries.options.legend.withAsTable(true) | ||
| + g.panel.timeSeries.options.legend.withPlacement('right') | ||
| + g.panel.timeSeries.panelOptions.withDescription('Time spent in Sidekiq jobs broken out by job name.') | ||
| + g.panel.timeSeries.standardOptions.withUnit('s'), |
There was a problem hiding this comment.
I wonder if this panel would look better with stacking enabled? Since we want to show cumulative job duration and breakdown by job.
There was a problem hiding this comment.
Not sure that'd apply here. Generally speaking we only want to stack when it's very clear we're showing a total value
| + g.panel.timeSeries.options.legend.withAsTable(true) | ||
| + g.panel.timeSeries.options.legend.withPlacement('right') | ||
| + g.panel.timeSeries.panelOptions.withDescription('Time spent in scheduled jobs broken out by job name.') | ||
| + g.panel.timeSeries.standardOptions.withUnit('s'), |
There was a problem hiding this comment.
Similar question to the sidekiqJobDuration.
discourse-mixin/jsonnetfile.json
Outdated
| } | ||
| }, | ||
| "version": "master" | ||
| } |
There was a problem hiding this comment.
Do we want to add a dependency to grafonnet-v11.4.0?
| targets=[signals.overview.pageViews.asTarget()] | ||
| ) | ||
| + g.panel.timeSeries.panelOptions.withDescription('Rate of pageviews for the entire application. Grouped by type and service.') | ||
| + g.panel.timeSeries.standardOptions.withUnit('none'), |
There was a problem hiding this comment.
Do we want to change the unit to something more semantic like views/sec?
There was a problem hiding this comment.
[panel-units-rule] 'Discourse overview': Dashboard 'Discourse overview', panel 'Page views' has no or invalid units defined: 'views/sec'
It fails linting :/
There was a problem hiding this comment.
My bad, I was asking if there is something as meaningful as views/sec, not specifically that unit.
| g.panel.histogram.new('Latest request time') | ||
| + g.panel.histogram.queryOptions.withTargets([signals.overview.latest99thPercentileRequestTime.asTarget()]) | ||
| + g.panel.histogram.panelOptions.withDescription('The 99th percentile amount of time for "latest" page requests for the selected site.') | ||
| + g.panel.histogram.standardOptions.withUnit('s'), |
There was a problem hiding this comment.
I don't know whether these should be timeseries or histograms. Since the datasource is not a histogram but a precalculated quantile value this might be broken. I am in favor of unifying both. What do you think?
There was a problem hiding this comment.
I can make it a histogram!
| g.panel.histogram.new('Topic show request time') | ||
| + g.panel.histogram.queryOptions.withTargets([signals.overview.topic99thPercentileRequestTime.asTarget()]) | ||
| + g.panel.histogram.panelOptions.withDescription('The amount of time for "topics show" requests for the selected site.') | ||
| + g.panel.histogram.standardOptions.withUnit('s'), |
There was a problem hiding this comment.
Similar comment to the histogram above it.
3 participants
This PR updates the discourse-mixin to use commonlib/g.libsonnet to generate its dashboards. It also employs the signals API to help manage the different prometheus targets which can be extended into the future.