sensu-plugins / sensu-plugins-graphite

This plugin provides native Graphite instrumentation for monitoring, including: replication status, various Graphite data queries, mutators, and handlers
http://sensu-plugins.io
MIT License
22 stars 45 forks source link

Fix compatibility with newer versions of Graphite #64

Closed jwatroba closed 6 years ago

jwatroba commented 6 years ago

Pull Request Checklist

Fixes: https://github.com/sensu-plugins/sensu-plugins-graphite/issues/63

Before:

{"command":"handler-graphite-event.rb","type":"pipe","filters":[],"severities":["ok","critical"],"handle_flapping":false,"handle_silenced":false,"name":"graphite_event"},"output":["{\"what\":\"sensu_event\",\"tags\":\"tag1,tag2,tag3\",\"data\":\"ok\",\"when\":{timestamp}}\n"]}

After (with --tags-as-array): {"command":"handler-graphite-event.rb --tags-as-array","type":"pipe","filters":[],"severities":["ok","critical"],"handle_flapping":false,"handle_silenced":false,"name":"graphite_event"},"output":["{\"what\":\"sensu_event\",\"tags\":[\"tag1,tag2,tag3\"],\"data\":\"ok\",\"when\":{timestamp}}\n"]}

After (without --tags-as array) -- No change from original behavior: {"command":"handler-graphite-event.rb","type":"pipe","filters":[],"severities":["ok","critical"],"handle_flapping":false,"handle_silenced":false,"name":"graphite_event"},"output":["{\"what\":\"sensu_event\",\"tags\":\"tag1,tag2,tag3\",\"data\":\"ok\",\"when\":{timestamp}}\n"]}

General

New Plugins

Purpose

Known Compatablity Issues

majormoses commented 6 years ago

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

majormoses commented 6 years ago

@jwatroba does this break backwards compatibility on older graphite versions?

jwatroba commented 6 years ago

Actually, might not be. According to the release notes on 1.0 of Graphite:

The events API now requires tags to be an array when creating tagged events. Previous versions only accepted string attributes. Tags are also serialized as arrays.

Might need a switch depending on the users version. Sorry, I originally thought it was backwards compatible.

majormoses commented 6 years ago

Yup, looks like we need to detect the version of graphite and conditionally make it an array. I have added the label as a reminder until we find a solution for backwards compatibility.

majormoses commented 6 years ago

@jwatroba I have not really seen a good way of detecting graphite version remotely what do you think about making it an argument? We could default for now on the old one to prevent breaking anyone and allow those that need it to move forward and later on we can change the default to the newer tag methods.

jwatroba commented 6 years ago

@majormoses I updated the pull request to default to the legacy version of Graphite and added '-l' to switch to 'latest' compatibility. I've tested in on post 1.0 Graphite and it works.

majormoses commented 6 years ago

Hmm I don't know that we want to have an argument that implies it will always work with latest what about naming it something like --tags-as-arrays? This is more descriptive and at some point we can change the default to true, and eventually remove the extra code.

majormoses commented 6 years ago

Can you please also add a changelog entry per our guidelines and a testing artifact?

majormoses commented 6 years ago

Also looks like there is an extra newline if you can kill that it will appease the rubocop.

jwatroba commented 6 years ago

Ok, I've updated the handler to use --tags-as-array or -t, updated the Changelog and added some before/after log entries to the pull request.

majormoses commented 6 years ago

released: https://rubygems.org/gems/sensu-plugins-graphite/versions/3.1.0