-
Notifications
You must be signed in to change notification settings - Fork 177
chore: IBM MQ mixin modernization #1557
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?
Conversation
b01900a to
321be7d
Compare
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.
Couple minor comments here, overall looks great
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.
A few minor changes.
ibm-mq-mixin/alerts.libsonnet
Outdated
| summary: 'There are expired messages, which imply that application resilience is failing.', | ||
| description: | ||
| ( | ||
| 'The number of expired messages in the {{$labels.qmgr}} is {{$labels.value}} which is above the threshold of %(alertsExpiredMessages)s.' |
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.
Shouldn't the description reference the $value of metric? Probably need to format it as well: {{ printf "%.0f" $value }}, WDYT?
ibm-mq-mixin/alerts.libsonnet
Outdated
| summary: 'Stale messages have been detected.', | ||
| description: | ||
| ( | ||
| 'A stale message with an age of {{$labels.value}} has been sitting in the {{$labels.queue}} which is above the threshold of %(alertsStaleMessagesSeconds)ss.' |
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 referencing {{ $value }} instead of {{ $labels.value }}.
ibm-mq-mixin/alerts.libsonnet
Outdated
| summary: 'There is limited disk available for a queue manager.', | ||
| description: | ||
| ( | ||
| 'The amount of disk space available for {{$labels.qmgr}} is at {{$labels.value}}%% which is below the threshold of %(alertsLowDiskSpace)s%%.' |
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 referencing {{ $value }} instead of {{ $labels.value }}.
ibm-mq-mixin/alerts.libsonnet
Outdated
| summary: 'There is a high CPU usage estimate for a queue manager.', | ||
| description: | ||
| ( | ||
| 'The amount of CPU usage for the queue manager {{$labels.qmgr}} is at {{$labels.value}}%% which is above the threshold of %(alertsHighQueueManagerCpuUsage)s%%.' |
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 referencing {{ $value }} instead of {{ $labels.value }}.
ibm-mq-mixin/signals/queue.libsonnet
Outdated
| name: 'Time on queue', | ||
| type: 'gauge', | ||
| description: 'The average time messages spent on the queue.', | ||
| unit: 'µs', |
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.
The metric says _seconds but the unit says micro seconds, can you please change it to 's'?
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 do think this one needs updating though, thanks for the catch 👍
| signals.queueManager.queueOperationsMqput.asTarget(), | ||
| ]) | ||
| + g.panel.timeSeries.panelOptions.withDescription('The number of queue operations of the queue manager.') | ||
| + g.panel.timeSeries.standardOptions.withUnit('operations') |
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 don't think 'operations' is a unit that we support in jsonnet-lib/common-lib. Can we use 'short' instead?
| signals.queue.mqputMqput1Count.asTarget(), | ||
| ]) | ||
| + g.panel.timeSeries.panelOptions.withDescription('The number of queue operations of the queue manager.') | ||
| + g.panel.timeSeries.standardOptions.withUnit('operations') |
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 don't think 'operations' is a unit that we support in jsonnet-lib/common-lib. Can we use 'short' instead?
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.
Several signals have the unit 'operations' and I don't think it is a supported unit in jsonnet-libs/common-lib. Can we use 'short' instead?
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.
Similar to queue signals.
Several signals have the unit 'operations' and I don't think it is a supported unit in jsonnet-libs/common-lib. Can we use 'short' instead?
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.
Similar to the two other signals files.
Several signals have the unit 'operations' and I don't think it is a supported unit in jsonnet-libs/common-lib. Can we use 'short' instead?
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.
operations is preferable here actually. If Grafana doesn't recognise a unit, it's treated as a custom string unit, so it'll be just fine
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.
Couple more comments, overall I'm happy with it!
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.
operations is preferable here actually. If Grafana doesn't recognise a unit, it's treated as a custom string unit, so it'll be just fine
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.
LGTM, all my comments have been addressed, though holding off approval until @aalhour is happy as well :)
Updates the IBM MQ mixin to use more modern libraries
IBM MQ cluster overviewIBM MQ queue manager overviewIBM MQ queue overviewIBM MQ topic overviewIBM MQ logsMetrics should be flowing to the shared Grafana instance on Nov 25 12pm-5pm EST :)