Skip to content
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

Sensu Handler shim for Sensu 2.0 event data. #190

Merged
merged 28 commits into from
Sep 13, 2018

Conversation

jspaleta
Copy link
Contributor

Description

Added event_2to1 method to Utils and --enable-2.0-event option to base Handler class. New option makes it possible to use sensu-plugin based handlers with Sensu 2.0 events until handlers provide native 2.0 event support.

Motivation and Context

the event data model changed in Sensu 2.0, so to make it possible to continue to use the community managed handlers a quick mapping back into Sensu 1.4 event JSON representation can be performed.

How Has This Been Tested?

Spot checked with local rebuild of sensu-plugin-logs handler so far.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have updated the Changelog following the conventions laid out on Keep A Changelog

…e Handler class. New option makes it possible to use sensu-plugin based handlers with Sensu 2.0 events until handlers provide native 2.0 event support.
@jspaleta
Copy link
Contributor Author

i need to clean things up for rubocop. I just wanted to get the PR out for people to look at asap

@jspaleta jspaleta changed the title Sensu Handler shim for Sensu 2.0 event data. WIP: Sensu Handler shim for Sensu 2.0 event data. Jul 25, 2018
# until they natively support 2.0
##
def event_2to1
if @event.key?('entity') && @event['client'].empty?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be @event['client'].nil?? Does empty cover nil?

Copy link
Contributor Author

@jspaleta jspaleta Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the normal use case it definitely needs to be empty... as event_read will set client to {} if client doesn't exist. I'll update to check for nil? || empty? to cover all cases.

I don't want to do this as part of event_read, as I want to control when this gets called.. for now just for handlers.

Copy link
Member

@majormoses majormoses Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty? does not cover .nil?

@teekennedy
Copy link

@jspaleta Could the 2to1 method be refactored and/or moved to its own gem so that it can be used for https://github.com/sensu/sensu-1.x-filter-wrapper ?

@jspaleta
Copy link
Contributor Author

jspaleta commented Sep 4, 2018

@Cyphus

okay refactoring this now as a utility function. I'm sure it makes sense as a separate gem right now. It definitely feels like something in the scope of sensu-plugin which provides nice base classes for mutators,checks and handlers.

@jspaleta jspaleta changed the title WIP: Sensu Handler shim for Sensu 2.0 event data. Sensu Handler shim for Sensu 2.0 event data. Sep 5, 2018
@majormoses
Copy link
Member

Could the 2to1 method be refactored and/or moved to its own gem

I agree with @jspaleta that this makes sense to be in the base plugin class as a util method, any GRPC service can depend on the base plugin class.

@jspaleta
Copy link
Contributor Author

jspaleta commented Sep 5, 2018

@Cyphus
I think have it set up now so that if you use this now so that you could for example
create a mutator instance from sensu-plugin in the GRPC and call the method like this:
event_1.x_json=mutator.event_2to1(event_2.x_json)

irb toy example:
require 'sensu-mutator'
mutator = Sensu::Mutator.new
event = JSON.parse("{}")
new_event=mutator.event_2to1(event)

CHANGELOG.md Outdated Show resolved Hide resolved
lib/sensu-plugin/utils.rb Outdated Show resolved Hide resolved
lib/sensu-plugin/utils.rb Outdated Show resolved Hide resolved
lib/sensu-plugin/utils.rb Outdated Show resolved Hide resolved
# will be set to true as a hint to indicate this is a mapped event object.
#
##
def event_2to1(orig_event = nil)
Copy link
Member

@majormoses majormoses Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I'd opt for a longer more descriptive name, I also find that saying 2to1 aloud is confusing. It's unclear if its 221, two to one, or if its combining two events into a single event. Maybe adding the word shim would give it more context which would make it less confusing. I can't say I am coming up with a great method name either and so far only end up with a very verbose name such as shim_v2_event_data_into_v1, event_data_shim_v2, sensu2_event_compatibility, mutate_sensu2_event_data_to_v1, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempt to refactor function name into something more descriptive in later commits.

TRUTHY_VALUES = %w[1 t true yes y].freeze
automap = ENV['SENSU_MAP_V2_EVENT_INTO_V1'].to_s.downcase

if mutator.config[:map_v2_event_into_v1,] || TRUTHY_VALUES.include?(automap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, looks like a mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting that this is allowed syntax by ruby. Fixed in newer commits.

# Deep copy of orig_event
event = Marshal.load(Marshal.dump(orig_event))

# Trigger mapping code iff enity exists and client does not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like an extra f

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

# action used in logs and fluentd plugins handlers
##
state = event['check']['state'] || 'unknown::2.0_event'
event['action'] ||= 'flapping' if state.casecmp('flapping').zero?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could be much simpler and less error prone with a case statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored using a hash mapping, since the states to action are 1 to 1.

if event['check']['history']
legacy_history = []
event['check']['history'].each do |h|
legacy_history << h['status'].to_s || '3'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm if we are not sure what the history is I feel like we should use nil (though that might cause issues?) as not knowing how to translate is not quite the same as the check itself returning an unknown state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also is history really stored as the string when really they are integers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the 1.4 events I have access to, the are stored as strings.
there is at least one mutator in the sensu-plugins collection that makes use of the history, and in fact makes a comment that the history is saved as an array strings.

sensu-plugins-pagerduty/bin/mutator-pagerduty-priority-override.rb

Returning nil instead of '3' may cause weird issues nil.to_i gets translated to 0 which is the 'ok' status. I think if we don't know how to translate the status in the history its better to use the 3 aka 'unknown' to make sure we flag it as a problem that needs to be investigated.

Or it may be better to throw an error here instead of providing a fallback if the status is unmappable to a string.

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @portertech anything else you catch I missed?

@@ -3,6 +3,14 @@ All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]
### Added
- Added map_v2_event_into_v1 method to Utils for all plugin classes to use.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add attribution for you post acceptance or you can do it prior, either way works.

@majormoses majormoses merged commit 552f5fa into sensu-plugins:master Sep 13, 2018
@majormoses
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants