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.

Sign up for GitHub

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

Strip invalid lines #137

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Strip invalid lines #137

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1df0f17
Strip invalid lines
boutetnico Jan 31, 2019
17393f8
Update changelog
boutetnico Feb 1, 2019
6ea82a5
Explore deeper into values instead of dropping them
boutetnico Mar 5, 2019
8739f82
Update bin/metrics-es-node-graphite.rb
majormoses Mar 6, 2019
ecb3318
Update bin/metrics-es-node-graphite.rb
majormoses Mar 6, 2019
6ebb1ad
Update bin/metrics-es-node-graphite.rb
majormoses Mar 6, 2019
f472b38
Update according to feedbacks
boutetnico Mar 6, 2019
06c0a55
Fix RuboCop warnings
boutetnico Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins/community/blob/master/HOW_WE_CHANGELOG.md)

## [Unreleased]
### Changed
- metrics-es-node-graphite.rb: exclude non-numeric data from results (@boutetnico)

## [3.0.0] - 2018-12-17
### Breaking Changes
Expand Down
31 changes: 27 additions & 4 deletions bin/metrics-es-node-graphite.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,9 @@ def run
node['indices'].each do |type, index|
index.each do |k, v|
# #YELLOW
unless k =~ /(_time$)/ || v =~ /\d+/
if k.end_with? 'is_throttled'
metrics["indices.#{type}.#{k}"] = v ? 1 : 0
Copy link
Member

@majormoses majormoses Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Im somewhat confused by this line v is not really a condition unless since its a truthy value. I actually thought it would be a string and therefore not work. Am I understanding that correctly? If that's the case we probably want to either create a helper method to convert string representation to a boolean or comment the above so that I can recall a year later why this works.

Here is a really simple example of doing this with a helper function

# define our function
irb(main):004:0> def true?(obj)
irb(main):005:1> obj.to_s == 'true'
irb(main):006:1> end

# leverage our function
irb(main):008:0> true?('true')
=> true
irb(main):009:0> true?('false')
=> false
irb(main):010:0> true?('foo')
=> false

One thing you will notice in this approach is that it will treat all non true values as false which could be a good or bad thing. The good thing is that this means that if a non string was passed to this it would not break but would be returning false data. The bad thing is that it would be returning false data which could be argued is as bad. We could also flip that around from a true to a false just as easily which while it would return false data it would be returning that it is being throttled which may prompt engineers to look at it and report a bug.

If we wanted to be even safer we could define it like so:

irb(main):011:0> def true?(obj)
irb(main):012:1> if obj.to_s == "true"
irb(main):013:2> true
irb(main):014:2> elsif obj.to_s == "false"
irb(main):015:2> false
irb(main):016:2> else
irb(main):017:2* "#{obj.to_s} is not a truthy value, please open an issue with this output so we can fix it"
irb(main):018:2> end
irb(main):019:1> end
=> :true?

To leverage it we could do something like this:

irb(main):020:0> true?('true')
=> true
irb(main):021:0> true?(true)
=> true
irb(main):022:0> true?('false')
=> false
irb(main):023:0> true?('foo')
=> "foo is not a truthy value, please open an issue with this output so we can fix it"

Here is an example of how we can then leverage the building blocks and decide what to do with it:

irb(main):025:0> if true?('foo').class == String
irb(main):026:1> raise(true?('foo'))
irb(main):027:1> else
irb(main):028:1* true?('true')
irb(main):029:1> end
RuntimeError: foo is not a truthy value, please open an issue with this output so we can fix it
	from (irb):26
	from /home/babrams/.rbenv/versions/2.4.1/bin/irb:11:in `<main>'

I need to think about it more and am open to feedback on what's the "best" thing to do in the face of such an issue. I think we have a couple options:

  • not attempt to handle at all and bail with an ugly stack trace
  • catch the error and return incorrect data if so do we return true or false?
  • catch the error and abort with a meaningful message
  • catch the error and write to STDERR rather than STDOUT, not sure if handlers take both stdout and and stderr input for metric input, I have honestly spent more time writing state checks than metric gathering scripts. I will either hunt through the code on this or reach out and get back to you on this.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this. I have followed your suggestions, can you please have a look at it?

Copy link
Member

Choose a reason for hiding this comment

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

will do I am also gonna see if someone else can review this particular portion and give me some feedback.

Copy link
Member

@jaredledvina jaredledvina Mar 12, 2019 •
edited by majormoses
Loading

Choose a reason for hiding this comment

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

My 2 cents:

not attempt to handle at all and bail with an ugly stack trace

Ugh, I've seen this approach taken, strongly prefer to not doing this as if we know of a failure mode.

catch the error and return incorrect data if so do we return true or false?

My gut says it's better to not return any data versus returning incorrect data. I'd rather someone debug why data isn't there versus using data that's not correct to make a design.

catch the error and abort with a meaningful message

❤️

catch the error and write to STDERR rather than STDOUT...

Hmmm, this is an interesting idea https://github.com/sensu/sensu/blob/master/lib/sensu/client/process.rb#L167 and seem to imply that STDERR would be captured, https://github.com/sensu/sensu/blob/master/lib/sensu/client/process.rb#L198-L200 so that's not crazy in my opinion. If we do do that, I'd like to have someone quickly test what STDERR ends up looking like in the sensu event just to make sure nothing weird happens there (since I've never used that approach).

Overall, I agree the testing for the truthiness of the value and returning it and bailing cleanly if we don't get one is probably the best move here.

Copy link
Member

Choose a reason for hiding this comment

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

I think I am leaning towards the printing to STDERR unless someone has a better idea.

elsif !(k =~ /(_time$)/ || v =~ /\d+/)
metrics["indices.#{type}.#{k}"] = v
end
end
Expand Down Expand Up @@ -306,16 +308,37 @@ def run
if fs_stats
node['fs'].each do |fs, fs_value|
unless fs =~ /(timestamp|data)/
fs_value.each do |k, v|
metrics["fs.#{fs}.#{k}"] = v
metrics_fs = hash_to_dotted_path(fs_value, "#{fs}.")
metrics_fs.each do |k, v|
metrics["fs.#{k}"] = v
end
end
end
end

metrics.each do |k, v|
output([config[:scheme], k].join('.'), v, timestamp)
if v.is_a? Numeric
output([config[:scheme], k].join('.'), v, timestamp)
end
end
ok
end
end

def hash_to_dotted_path(hash, path = "")
boutetnico marked this conversation as resolved.
Show resolved Hide resolved
hash.each_with_object({}) do |(k, v), ret|
key = path + k.to_s
if v.is_a? Hash
ret.merge! hash_to_dotted_path(v, key.to_s + ".")
boutetnico marked this conversation as resolved.
Show resolved Hide resolved
elsif v.is_a? Array
v.each do |element|
if element['device_name']
key2 = key.to_s + "." + element['device_name']
boutetnico marked this conversation as resolved.
Show resolved Hide resolved
ret.merge! hash_to_dotted_path(element, key2.to_s + ".")
boutetnico marked this conversation as resolved.
Show resolved Hide resolved
end
end
else
ret[key] = v
end
end
end