align metric naming scheme output #46
Conversation
henkjan
force-pushed
the
align-scheme-output
branch
from
7b9546b to
7a9b2f6
Compare
|
Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly. |
|
fixes #47 |
There was a problem hiding this comment.
Overall this looks good, however this causes a breaking change as it changes the metrics in a way that might require a user to need to adjust their config. We need to call this out in the changelog per our guidelines. Also we need to be careful to not leave empty metric path when database is not specified.
| @@ -113,7 +113,7 @@ def lag_compute(res1, res, m_segbytes) # rubocop:disable NestedMethodDefinition | |||
|
|
|||
| # Compute lag | |||
| lag = lag_compute(res1, res, m_segbytes) | |||
| output config[:scheme].to_s, lag | |||
| output "#{config[:scheme]}.replication.#{config[:database]}.to_s", lag | |||
There was a problem hiding this comment.
This is a breaking change as it completely changes the metric output and people may have queries that have alerts on them, this needs to be a major version bump which I will do post acceptance.
| output "#{config[:scheme]}.bgwriter.maxwritten_clean", row['maxwritten_clean'], timestamp | ||
| output "#{config[:scheme]}.bgwriter.buffers_backend", row['buffers_backend'], timestamp | ||
| output "#{config[:scheme]}.bgwriter.buffers_alloc", row['buffers_alloc'], timestamp | ||
| output "#{config[:scheme]}.bgwriter.#{config[:database]}.checkpoints_timed", row['checkpoints_timed'], timestamp |
There was a problem hiding this comment.
what if they don't specify a database name? I think we need to check if it is set and then change the output on it.
There was a problem hiding this comment.
Aside from the comments I left we need to call out the breaking changes in the changelog per our guidelines
|
closing due to inactivity, to see this work over the line either re-open this pull request or open a new one. |
Pull Request Checklist
Is this in reference to an existing issue?
General
Update Changelog following the conventions laid out here
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass
New Plugins
Tests
Add the plugin to the README
Does it have a complete header as outlined here
Purpose
Known Compatibility Issues