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

enable periodic flushing #36

Merged
merged 2 commits into from
Jun 7, 2018
+9 −1
Merged

enable periodic flushing #36

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3baefc1
enable periodic flushing
colinsurprenant Jun 6, 2018
94fd5b4
bump version to 4.0.5
colinsurprenant Jun 7, 2018
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
4 changes: 4 additions & 0 deletions lib/logstash/filters/elapsed.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ class LogStash::Filters::Elapsed < LogStash::Filters::Base
# to the "end event"; if it's set to `true` a new "match event" is created.
config :new_event_on_match, :validate => :boolean, :required => false, :default => false

# This filter must have its flush function called periodically to be able to purge
# expired stored start events.
config :periodic_flush, :validate => :boolean, :default => true
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this require a docs change to set the default value of this field to true?

https://www.elastic.co/guide/en/logstash/current/plugins-filters-elapsed.html#plugins-filters-elapsed-periodic_flush

Copy link
Contributor Author

@colinsurprenant colinsurprenant Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

oh, I guess so. The tricky part here is that this doc part seems to be common for these common "base" options so I am not sure how we can override it in this filter doc.
Maybe @karenzone can help here?

Copy link

Choose a reason for hiding this comment

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

Not TOO tricky when we can consult @dedemorton about advanced magic such as this.
From DeDe: "We can conditionally code the common docs to show different defaults...just need to know conditions where the default is different. We will also need to create a new version of the file for the versioned plugin docs...but Colin and Rob don’t need to worry about that bit."

Sooooo, we just need to know conditions where the default is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenzone as per this PR, the periodic_flush option is now defaulted to true in this plugin and I don't see why anyone would want to change it actually.

config :periodic_flush, :validate => :boolean, :default => true

Copy link
Contributor

Choose a reason for hiding this comment

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

The common options are defined in a shared file here: https://github.com/elastic/logstash/blob/master/docs/include/filter.asciidoc#plugins-{type}s-{plugin}-periodic_flush. Looks like the default value in the shared file is false.

To set the default to true, we'll need to add some conditional coding to the common file.

Are there other filter plugins that set periodic_flush to true by default, or just this one? If there are multiple plugins, we might want to introduce an asciidoc attribute to use in the condition. You can see how we've done something similar with the codec option here:

https://raw.githubusercontent.com/elastic/logstash/master/docs/include/input.asciidoc

ifdef::default_codec[]
  * Default value is +"{default_codec}"+
endif::[]
ifndef::default_codec[]
  * Default value is `"plain"`
endif::[]

We introduced an attribute called default_codec that overrides the setting in the common file when the default_codec attribute is defined in a plugin's attributes. For example, here you'll see :default_codec: line: https://raw.githubusercontent.com/elastic/logstash-docs/master/docs/plugins/inputs/stdin.asciidoc.

Using an asciidoc attribute is a good approach because it doesn't affect the versioned plugin reference (meaning that we won't have to create a new version of the file to work with 6.3+).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with that @colinsurprenant - ok with you too @karenzone?

Copy link

Choose a reason for hiding this comment

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

Works for me. I've added it to my list for tracking: elastic/logstash#6693

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we also need conditional coding on the description to indicate when the option is not configurable. @karenzone I can help with that when you're ready to work on the doc changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenzone I just looked at elastic/logstash#6693 and note that this also impacts the metrics, aggregate, multilines and throttle filters (per my quick search ^^)

Copy link

Choose a reason for hiding this comment

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

@colinsurprenant I have added the expanded list of impacted filters to my tracking list, and will do a thorough search for additional when I correct the issue. Thank you for keeping me in the loop


public
def register
@mutex = Mutex.new
Expand Down