Skip to content

Migrate to use v0.14 API#10

Open
cosmo0920 wants to merge 12 commits intoephemeralsnow:masterfrom
cosmo0920:migrate-v0.14
Open

Migrate to use v0.14 API#10
cosmo0920 wants to merge 12 commits intoephemeralsnow:masterfrom
cosmo0920:migrate-v0.14

Conversation

@cosmo0920
Copy link
Copy Markdown

I've tried to migrate to use v0.14 Output and Filter Plugin API.
This PR contains major update change and also includes in breaking changes.
If above questions/problems are acceptable or reasonable for you, could you bump up major version if releasing new version of gem?
And please refer http://docs.fluentd.org/v0.14/articles/plugin-update-from-v12 before release new version.

Thanks in advance.

end
}
new_es
def filter(tag, time, record)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note: In v0.14, #filter is faster than #filter_stream because v0.14 makes filter pipeline: https://github.com/fluent/fluentd/blob/26c4dc4872b71665cd743c1d6b71c774429c750f/lib/fluent/event_router.rb#L214-L232

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

How about replacing the body of the filter method with the body of the filter_record method?

It seems possible to delete redundant error handling.
https://github.com/fluent/fluentd/blob/161e1706273f2e9e179a0cc3d33e88a40dd60198/lib/fluent/plugin/filter.rb#L64-L66

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've replaced it with the body of #filter_record.

@ephemeralsnow
Copy link
Copy Markdown
Owner

I have not thought about the release of the new version yet, but before that, I'd like to think about migrating to a new configuration method.

config_section :filter, multi: true
like as
https://github.com/fluent/fluentd/blob/162031fadf6de19f8f7c0c40766f9c570f607baf/lib/fluent/plugin/filter_grep.rb#L34-L46

Because string with `#split` makes the following a pitfall:

```aconf
requires yaml, time
<filter>
  filter "record.to_yaml; [Time.now.to_i, record]"
</filter>
```

is not valid config.
`requires yaml, time` will be interpreted as
`['yaml', ' time']` and execute:

```ruby
require 'yaml'
require ' time'
```

This is because why string type with `#split` makes a some pitfall.
@cosmo0920
Copy link
Copy Markdown
Author

cosmo0920 commented Oct 3, 2017

I've added commits which are reflected your review.
Please take a look.

@ephemeralsnow
Copy link
Copy Markdown
Owner

Thanks for all your great work.
I'm almost satisfied, but I noticed that there are many filter keywords.
Please give me some time to think about keywords.

<filter **>
  @type eval
  <filter>
    filter "[time, record] if record['status'] == '404'"
  </filter>
  <filter>
    filter "[time, record] if record['status'] == 'POST'"
  </filter>
</filter>

@cosmo0920
Copy link
Copy Markdown
Author

How about using <rule> instead of <filter> in specifying filter config section?

@ephemeralsnow
Copy link
Copy Markdown
Owner

I think that's fine.
However, since configN and filterN are not pairs, I noticed that the current syntax is not appropriate.
I'm sorry but I will think about it for a while.

@cosmo0920
Copy link
Copy Markdown
Author

Oh, I see....

Then, how about the following?

<rule>
  filter [something filtering config]
</rule>
<eval>
  config [something evaluate instances]
</eval>

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants