Migrate exec plugins to v0.14 api #1297

tagomoris · 2016-10-26T09:51:07Z

This change is to migrate out_exec_filter to v0.14 API, and make in/out_exec plugins free from Fluent::ExecUtil::* classes.

This change includes the changes to:

make all (built-in or 3rd party) formatter/parser plugins available in exec plugins
make inject/extract plugin helpers available in exec plugins
add a method to return parser/formatter type for data handling (binary, text or text-per-line)
add methods to pass io or partial data to parsers to parse binary or multiline text data in consistent way
add options of child_process plugin helper to wait process exits and callback for exit status

This change is successor of #1048.

tagomoris · 2016-11-02T14:02:15Z

@repeatedly Could you review this change?

tagomoris · 2016-11-04T10:37:58Z

Rebased on #1305.
@repeatedly ping?

repeatedly · 2016-11-04T11:41:45Z

@tagomoris I will review it. You pushed some changes after previous comment, so I waited to finish your additional commit ;p

tagomoris · 2016-11-04T13:14:47Z

:)
And... currently, CI on Windows is failing. It's problem about just test code, and I'll push some commits for that. Please review the code anyway.

tagomoris · 2016-11-04T13:16:03Z

Hmm, test_out_exec.rb is also unstable a little.

repeatedly · 2016-11-06T23:33:48Z

@sonots Could you also check this PR? This patch is very large so need more reviewers.

repeatedly · 2016-11-06T23:45:38Z

lib/fluent/plugin/formatter_tsv.rb

+    class TSVFormatter < Formatter
+      Plugin.register_formatter('tsv', self)
+
+      desc 'Names of fields included in each lines'


"Field names" is more simpler than "Names of fields"

repeatedly · 2016-11-06T23:57:13Z

lib/fluent/plugin/in_exec.rb


+    def on_record(time, record)
+      tag = nil


This line is not needed.

repeatedly · 2016-11-07T00:01:53Z

lib/fluent/plugin/in_exec.rb

      router.emit(tag, time, record)
    rescue => e
-      log.error "exec failed to emit", error: e, tag: tag, record: Yajl.dump(record)
+      log.error "exec failed to emit", tag: tag, record: Yajl.dump(record), error: e
+      router.emit_error_event(tag, time, record, "exec failed to emit")


4th error argument should be an exception object.

repeatedly · 2016-11-07T04:08:04Z

lib/fluent/plugin/out_exec.rb

-        prog = "#{@command} #{chunk.path}"
+      record = inject_values_to_record(tag, time, record)
+      if @formatter.formatter_type == :text_per_line
+        @formatter.format(tag, time, record).chomp + "\n"


Using constant value for "\n" is better for reducing temporary object allocation.

repeatedly · 2016-11-07T04:12:33Z

lib/fluent/plugin/out_exec.rb

+      prog = if chunk.respond_to?(:path)
+               "#{@command} #{chunk.path}"
+             else
+               tmpfile = Tempfile.new("fluent-plugin-exec-")


"fluent-plugin-out-exec-" is more better.

repeatedly · 2016-11-07T04:23:01Z

lib/fluent/plugin/out_exec.rb

+      chunk_id = chunk.unique_id
+      callback = ->(status){
+        begin
+          if tmpfile


This code should be moved to enusre in try_write instead of here?

No. ensure of #try_write doesn't take care about child process status.
This callback is called after child process plugin helper find the child process's exit status. So here is the only place to delete tmpfile.

Hmm... what happen if exception happens before calling callback?

Of course, temp files will leak.
But all exceptions about child process status must be rescued by child process plugin helper code. Only bugs should be able to raise errors before callback.

repeatedly · 2016-11-07T04:39:51Z

lib/fluent/plugin/out_exec_filter.rb


+    def on_record(time, record)
+      tag = nil


repeatedly · 2016-11-07T04:44:41Z

lib/fluent/plugin/out_exec_filter.rb

+      tag = nil
+      tag = extract_tag_from_record(record)
+      tag = @added_prefix_string + tag if tag && @add_prefix
+      tag ||= @tag


Off topic. We should care extrated result and @tag is nil case.

repeatedly · 2016-11-07T04:45:36Z

lib/fluent/plugin/out_exec_filter.rb

        @next_log_time = Time.now.to_i + @suppress_error_log_interval
      end
+      router.emit_error_event(tag, time, record, "exec_filter failed to emit") if tag && time && record


ditto. 4th argument should be an error object.

repeatedly · 2016-11-07T04:54:36Z

lib/fluent/plugin/parser_msgpack.rb

+      end
+      alias parse_partial_data parse
+
+      def parse_io(io)


should be use parse_io(io, &block) for API signature?

repeatedly · 2016-11-07T05:04:53Z

lib/fluent/plugin_helper/compat_parameters.rb

@@ -77,10 +79,20 @@ module CompatParameters
        "utc" => nil,
      }

+      EXTRACT_PARAMS = {
+        "time_key"      => "time_key",
+        "time_format"   => "time_format",


More spaces than expected.

…to show situation clearly

…w/ v0.12

… & child_respawn

This reverts commit 2ea9540.

tagomoris · 2016-11-08T05:15:23Z

I've pushed commits for review comments.

tagomoris · 2016-11-09T03:18:40Z

@repeatedly ping

repeatedly · 2016-11-09T18:48:47Z

lib/fluent/plugin/out_exec_filter.rb

+      tag = tag_remove_prefix(tag)
+      record = inject_values_to_record(tag, time, record)
+      if @formatter.formatter_type == :text_per_line
+        @formatter.format(tag, time, record).chomp + "\n"


Use NEWLINE = "\n" like above.

repeatedly · 2016-11-09T19:39:18Z

LGTM.

But my code review doesn't guarantee new process handling code has no problem on actual workloads.
So merging it and test by users is better.

tagomoris added the v0.14 label Oct 26, 2016

tagomoris self-assigned this Oct 26, 2016

tagomoris force-pushed the migrate-exec-plugins-to-v0.14-api branch 3 times, most recently from 618069b to a15583e Compare November 1, 2016 11:07

tagomoris force-pushed the migrate-exec-plugins-to-v0.14-api branch from a15583e to 853342e Compare November 2, 2016 13:58

tagomoris changed the title ~~[WIP] Migrate exec plugins to v0.14 api~~ Migrate exec plugins to v0.14 api Nov 2, 2016

tagomoris force-pushed the migrate-exec-plugins-to-v0.14-api branch from 8fcb357 to 046c787 Compare November 4, 2016 10:36

repeatedly reviewed Nov 6, 2016

View reviewed changes

lib/fluent/plugin/in_exec.rb

def on_record(time, record)

tag = nil

Copy link

Member

repeatedly Nov 6, 2016

Choose a reason for hiding this comment

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

This line is not needed.

repeatedly reviewed Nov 7, 2016

View reviewed changes

lib/fluent/plugin/out_exec_filter.rb

def on_record(time, record)

tag = nil

Copy link

Member

repeatedly Nov 7, 2016

Choose a reason for hiding this comment

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

ditto

repeatedly reviewed Nov 7, 2016

View reviewed changes

tagomoris added 5 commits November 7, 2016 17:40

fix to raise error for nonsense #commit_write call

1a99a98

add extract support and fix time handling

323f7e1

fix to wait child processes to run actually, and fix timeout feature …

c7a197c

…to show situation clearly

show plugin helper name explicitly

ac5c6f9

add keep_tag_key/keep_time_key support and warning for compatibility …

d985778

…w/ v0.12

tagomoris added 8 commits November 7, 2016 17:40

add tests for v0.14 style configurations and features of num_children…

6a8b3fa

… & child_respawn

add example configuration

ac37c03

Revert "fix to raise error for nonsense #commit_write call"

ae8f042

This reverts commit 2ea9540.

add error message pattern for ruby 2.1

04a339c

add timezone to timestamps converted into unixtime

8a5faa3

MONOTONIC_RAW is better, but only for Linux

768fb81

fix bug to never break loop

39b9060

make sleep time longer because shorter one fails sometimes on Travis

4686902

tagomoris force-pushed the migrate-exec-plugins-to-v0.14-api branch from 046c787 to 4686902 Compare November 7, 2016 08:40

tagomoris added 5 commits November 8, 2016 14:09

remove useless variable assignment

8c552d7

fix to pass error object into error stream

dd773b0

fix to use constant not to create new string object instances

2ecf4c8

fix to use plugin name

cf1b2e8

fix description, method signature and indentation

21b9a32

make test stable

34b81e6

tagomoris force-pushed the migrate-exec-plugins-to-v0.14-api branch 2 times, most recently from 555e7a2 to d96b08f Compare November 8, 2016 07:20

tagomoris added 2 commits November 8, 2016 16:29

fix to check loosely not to fail on CI env

a9f5300

take care about tests on Windows

9d8b6e7

tagomoris force-pushed the migrate-exec-plugins-to-v0.14-api branch from d96b08f to 9d8b6e7 Compare November 8, 2016 07:30

make tests more stable

ef79463

This was referenced Nov 8, 2016

Migrate out_exec_filter to v0.14 API #1048

Closed

migrating in_http to v0.14 API #1308

Merged

repeatedly reviewed Nov 9, 2016

View reviewed changes

fix to use constant not to assign new objects everytime

8b19791

tagomoris merged commit e3edf3e into master Nov 11, 2016

tagomoris deleted the migrate-exec-plugins-to-v0.14-api branch November 11, 2016 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate exec plugins to v0.14 api #1297

Migrate exec plugins to v0.14 api #1297

tagomoris commented Oct 26, 2016 •

edited

Loading

tagomoris commented Nov 2, 2016

tagomoris commented Nov 4, 2016

repeatedly commented Nov 4, 2016

tagomoris commented Nov 4, 2016

tagomoris commented Nov 4, 2016

repeatedly commented Nov 6, 2016

repeatedly Nov 6, 2016

repeatedly Nov 6, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016

tagomoris Nov 7, 2016

repeatedly Nov 7, 2016

tagomoris Nov 8, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016

repeatedly Nov 7, 2016 •

edited

Loading

tagomoris commented Nov 8, 2016

tagomoris commented Nov 9, 2016

repeatedly Nov 9, 2016

tagomoris Nov 10, 2016

repeatedly commented Nov 9, 2016

Migrate exec plugins to v0.14 api #1297

Migrate exec plugins to v0.14 api #1297

Conversation

tagomoris commented Oct 26, 2016 • edited Loading

tagomoris commented Nov 2, 2016

tagomoris commented Nov 4, 2016

repeatedly commented Nov 4, 2016

tagomoris commented Nov 4, 2016

tagomoris commented Nov 4, 2016

repeatedly commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repeatedly Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

tagomoris commented Nov 8, 2016

tagomoris commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repeatedly commented Nov 9, 2016

tagomoris commented Oct 26, 2016 •

edited

Loading

repeatedly Nov 7, 2016 •

edited

Loading