voxpupuli / puppet-logstash

Puppet module to manage Logstash
https://forge.puppet.com/elastic/logstash
Apache License 2.0
191 stars 299 forks source link

[discussion] Conditional support #107

Closed electrical closed 10 years ago

electrical commented 10 years ago

Hi all,

Conditional support has been added into Logstash 1.2 and I'm trying to implement that in a clean way into the puppet module.

Seeing as puppet has no notion of resource ordering it is something that has to be done custom.

Initially i thought about to simply use numbers to order resources. Sounds very simple but can be hard to maintain when you want to add/remove something.

So, on to an other idea.

below you will find a piece of puppet code and the corresponding result config file for LS.

logstash::input::tcp { 'syslog input':
  port => 9999
}

logstash::filter::grok { 'detect app from syslog':
}

logstash::condition::if { 'if we have type apache':
  expression => “[type] == ‘apache’”
  children   => [ ‘Logstash::Filter::Grok[apache]’, ‘Logstash::Filter::Date[setdate]’ ],
  require    => [ 'Logstash::Filter::Grok[detect app from syslog]' ]
}

logstash::condition::elseif { 'if type is syslog':
  expression => “[type] == ‘syslog’”
  children   => [ 'Logstash::Filter::Grok[syslog]', 'Logstash::Filter::Date[setdate]' ],
  require    => [ 'Logstash::Condition::If[if we have type apache]' ]
}

logstash::condition::else { 'all others':
  children => [ 'Logstash::Filter::Grok[something]', 'Logstash::Filter::Date[other_setdate]' ],
  require  => [ 'Logstash::Condition::Else[if type is syslog]' ]
}

logstash::filter::grok { 'apache':
}

logstash::filter::grok { 'syslog':
}

logstash::filter::grok { 'something':
}

logstash::filter::date { 'setdate':
}

logstash::filter::date { 'other_setdate':
}

logstash::output::elasticsearch { 'send it all to ES':
}

This results in the following config file for LS.

input { # Start inputs

  tcp { # syslog input
  }

} # End inputs

filter { # Start filters

  grok { # detect app from syslog
  }

  if [type] == 'apache' # if we have type apache

    grok { # apache
    }
    date { # setdate
    }

  elseif [type] == 'syslog' # if type is syslog

    grok { # syslog
    }
    date { # setdate
    }

  else

    grok { # other
    }
    date { # other_setdate
    }

  end

} # End  filters

output {  # Start outputs
  elasticsearch { # send it all to ES
  }
} # End outputs

The 'children' part defines which resources are within that conditional block. Ordering within that array is the sequence as they will come out.

The 'require' part defines in which sequence the resources are outside the conditional blocks.

In this case the 'if' conditional requires the first Grok filter. This means that the Grok filter comes first and then the if conditional.

And now the big question.... Is this the way to go?? Can someone think up a better way to do this? Should we do this at all? Should we return to normal config files?

phrawzty commented 10 years ago

As noted in the related gist (https://gist.github.com/electrical/e7a7bf79896b030388b8), using numbers to control sort order has strong precedent from Concat and should therefore be considered a canonical way to deal with this issue. That said, abstracting the LS config vocabulary as Puppet definitions is a clever approach, and has the potential to be more powerful than simple number-sorting.

Concerning the actual syntax, the proposal above seems to translate the name of each conditional definition above to a comment within the generated LS config, which makes sense; however, might there be value in simply using what you've got in expression as the name? This would reduce the number of lines necessary per block while simultaneously creating a declarative line that incorporates the very expression it represents. Example:

logstash::condition::if { “[type] == ‘apache’”:
  children   => [ ‘Logstash::Filter::Grok[apache]’, ‘Logstash::Filter::Date[setdate]’ ],
  require    => [ 'Logstash::Filter::Grok["whatever"]' ]
}
electrical commented 10 years ago

@phrawzty what about the 'else' conditional? what would you set in that one? Beyond that it does make sense and makes it a bit cleaner.

phrawzty commented 10 years ago

what about the 'else' conditional? what would you set in that one?

Well it's an else, it doesn't have a condition beyond failing to trigger previous conditions, so I guess that'd just be a comment. ;)

Honestly, having done mock-ups of both types on my own, I'm not sure if one is clearly superior to the other - they both have positive and negative aspects. Your original proposition does have one important element: the name is always a comment, and if an expression is required, it is stated explicitly. In that respect, while your proposition is more verbose, it's also more consistent.

electrical commented 10 years ago

Decided to implement the conditional support as discussed in this thread. The 'condition' parameter will stay to make things more explicit and verbose.

electrical commented 10 years ago

After trying to implement the conditional support i've decided to completely drop it including all the defines. Will be going back to the old good times of config files.