-
Notifications
You must be signed in to change notification settings - Fork 177
chore: Redis Enterprise modern library usage #1538
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: Redis Enterprise modern library usage #1538
Conversation
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.
First pass, no linting errors locally, a few typos and an issue with alerts, everything else is minor.
| redisEnterpriseNodes: | ||
| link.link.new('Redis Enterprise nodes', '/d/' + this.grafana.dashboards['redis-enterprise-node-overview.json'].uid), | ||
| redisEnterpriseDatabases: | ||
| link.link.new('Redis Enterprise databases', '/d/' + this.grafana.dashboards['redis-enterprise-database-overview.jsonn'].uid), |
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.
Typo? .jsonn --> .json
| refresh, | ||
| period, | ||
| ), | ||
| 'redis-enterprise-database-overview.jsonn': |
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.
Typo? .jsonn --> .json
| clusterTotalRequests: { | ||
| name: 'Cluster total requests', | ||
| nameShort: 'Total requests', | ||
| type: 'raw', |
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 also be of type gauge?
| clusterTotalMemoryUsed: { | ||
| name: 'Cluster total memory used', | ||
| nameShort: 'Total memory used', | ||
| type: 'raw', |
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 also be of type gauge?
| clusterTotalConnections: { | ||
| name: 'Cluster total connections', | ||
| nameShort: 'Total connections', | ||
| type: 'raw', |
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 also be of type gauge?
| databaseReadRequests: { | ||
| name: 'Database read requests', | ||
| nameShort: 'Read requests', | ||
| type: 'raw', |
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 also be of type gauge?
| databaseCacheHitRatio: { | ||
| name: 'Database cache hit ratio', | ||
| nameShort: 'Cache hit ratio', | ||
| type: 'raw', |
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 also be of type gauge?
| databaseMemoryUtilization: { | ||
| name: 'Database memory utilization', | ||
| nameShort: 'Memory utilization', | ||
| type: 'raw', |
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 also be of type gauge?
| expr: ||| | ||
| bdb_avg_latency / 1000 > %(alertsDatabaseHighLatencyMs)s | ||
| ||| % $._config, | ||
| bdb_avg_latency{%(filteringSelector)s} / 1000 > %(alertsDatabaseHighLatencyMs)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.
If we divide by 1000 that would convert the value to seconds, but the alert threshold is in milliseconds - alertsDatabaseHighLatencyMs.
| + g.panel.timeSeries.standardOptions.withUnit('percent'), | ||
|
|
||
| nodesNodeCPUUtilizationPanel: | ||
| 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.
What are your thoughts on how this would look versus the commonlib.panels.cpu.timeSeries.base.new? I see that the base is empty in the common lib but it has utilization by core and mode overrides.
This PR refactors the Redis-Enterprise mixin to use the signals API to create its dashboards and follow best practices using g.libsonnet and commonlib.
If you need access for testdata we can provide it at keithschmitty.grafana.net
Screenshots with minimal setup (no loadgen so some metrics will not be populated in these screenshots)
Screenshots
OverviewNodes


Database
Logs

The way my lab was configured didn't allow for logs but it should work