Ignore individual options for out copy #303

dterror-zz · 2014-04-22T21:27:08Z

This addresses #296 by adding a fault-tolerant option to OutputChain. With this, it will only raise an exception if all configured outputs raise exceptions. Also added the ignore_individual_errors option to out_copy which simply changes how it configures its OutputChain.

@repeatedly I'll still implement your idea of buffer_ignore_exceeded_chunk, but in a different pull-request, is that ok?

it will only raise an exception if all outputs fail.

Uses a fault tolerant output chain to only fail if all outputs fail.

repeatedly · 2014-04-23T16:11:27Z

lib/fluent/output.rb

+        result = @array[@offset-1].emit(@tag, es, self)
+      rescue Exception => e
+        @errors += 1
+        raise e if not @fault_tolerant or @errors == @array.length


Fluentd code uses ! instead of not.
Ah, also || is popular than or in fluentd.

repeatedly · 2014-04-23T16:50:19Z

lib/fluent/output.rb

+      rescue Exception => e
+        @errors += 1
+        raise e if not @fault_tolerant or @errors == @array.length
+        $log.error e.message, :error => e.to_s


Please add :error_class => e.class.to_s.

repeatedly · 2014-04-23T16:52:51Z

@dterror

I'll still implement your idea of buffer_ignore_exceeded_chunk, but in a different pull-request, is that ok?

I am considering buffer error handling improvement. buffer_ignore_exceeded_chunk is a part of such changes. So please pending it for now.

dterror-zz · 2014-04-23T16:55:36Z

@repeatedly implemented in #306

dterror-zz · 2014-04-23T16:58:47Z

btw, you mentioned you were working on streaming errors to another plugin, that sounds like a very good idea, will it be based on Fluent::Log?

repeatedly · 2014-04-23T17:14:29Z

@dterror It is based on new Engine routing. We need more time to apply this change to current version.

sonots · 2014-04-23T17:16:39Z

lib/fluent/output.rb

+        end
+        @offset += 1
+        result = @array[@offset-1].emit(@tag, es, self)
+      rescue Exception => e


Do not rescue Exception. Exception is super base class of ruby's error including NoMemoryError, SyntaxError. rescue => e should be fine.

repeatedly · 2014-04-23T17:22:05Z

After review the code, I have one idea.
How about ignore_individual_errors [2, 3] instead of ignore_individual_errors true.
The user specify ignore plugin indexes. Above example, second and third plugin errors are ignored.

Or specifying plugin id like ignore_individual_errors ["id2", "id3"]

sonots · 2014-04-23T17:23:23Z

lib/fluent/output.rb

+      rescue Exception => e
+        @errors += 1
+        raise e if !@fault_tolerant || @errors == @array.length
+        $log.error e.message, :error => e.to_s


e.message and e.to_s are the same, so this is not informative.
$log.error :error_class => e.class.to_s, :error => e.to_s is better

how about $log.error :error_class => e.class.to_s, :error => e.message? e.message is more semantic than e.to_s imo

it's fine :-)

Making sure to log all exceptions so that information is not lost.

dterror-zz · 2014-04-23T18:38:29Z

@repeatedly I think that with the implementation of ignore_exceeded_chunk and other error handling improvements we can have a more granular control of what is allowed to fail on a per-plugin config level. With this ignore_individual_errors can be more 'general'. What do you think?

repeatedly · 2014-04-23T20:08:47Z

What does 'general' mean? ignore_indivisual_erros can be applied to another plugins or Fluentd core level?

dterror-zz · 2014-04-23T20:28:24Z

I mean it won't need a granular control such as ignore_individual_errors ["id2", "id3"] because each plugin inside copy would be able to use a no-fail option (like buffer_ignore_exceeded_chunk for example).

repeatedly · 2014-04-23T20:34:10Z

@dterror I see. But only ignore_individual_errors true, the user can't raise an exception which should not be ignored, e.g. S3 plugin on your case.

dterror-zz · 2014-04-23T21:10:16Z

They can ignore ignore_individual_errors in copy and use other no-fail solutions in the ones that can fail.

For example, for my use case in particular I could use:

<match foo.bar.*>
  type copy
  ignore_individual_errors true
  <store>
    type s3
    #...
  </store>
  <store>
    type elasticsearch
    #...
  </store>
</match>

If its okay for either to fail, but not both.

OR

<match foo.bar.*>
  type copy
  <store>
    type s3
    #...
  </store>
  <store>
    type elasticsearch
    buffer_ignore_exceeded_chunk true
    #...
  </store>
</match>

If S3 can't to fail.

It's my belief that buffer_ignore_exceeded_chunk was the only thing that would cause an application error.

repeatedly · 2014-04-23T22:04:11Z

It's my belief that buffer_ignore_exceeded_chunk was the only thing that would cause an application error.

Yeah but other error occurs. Hmm, Fluentd's config parameter can accept any type. I will add array support for other use-cases.

sonots · 2014-04-24T01:34:52Z

But only ignore_individual_errors true, the user can't raise an exception which should not be ignored, e.g. S3 plugin on your case.

Although I am not familiar with S3 plugin, ignore_individual_errors true still raises OutputChainError when one of <store> plugins raises error, so the user can raise an exception.
This looks fine enough for me. Am I missing something?

sonots · 2014-04-24T01:39:04Z

Oh, current implementation raises OutputChainError only when all of plugins raise errors. I think OutputChainError should be raised if one or some of <store> plugins raise error after all <store> plugins are processed.

repeatedly · 2014-04-24T04:25:52Z

@sonots

I think OutputChainError should be raised if one or some of plugins raise error after all plugins are processed.

The proposal of this PR want to ignore the exception which caused by un-important plugin. This is why I propose ignore_indivisual_errors [1, 2] approach to ignore specified plugins.

sonots · 2014-04-24T04:36:07Z

The reason to ignore the exception is (#296)

Raising an exception at this point will break the chain,

So, I thought as the ultimate goal of this PR is to create a option not to break the chain.

Suppressing to raise errors in each plugin, and raising OutputChainError after all plugins are processed achieves the goal.

repeatedly · 2014-04-24T04:43:51Z

Suppressing to raise errors in each plugin, and raising OutputChainError after all plugins are processed achieves the goal.

I think no. This behaviour breaks the transaction. Currently, Input plugin assumes emit is succeeded or raise an error. Your approach is succeeded and raise an error.
For example, in_http returns an error to client when emit failed.
In your approach, the user may get duplicated logs because almost clients have retry or fallback feature.

sonots · 2014-04-24T05:37:14Z

Your approach is succeeded and raise an error.

No, my approach is succeeded (when no plugin fail) or raises an error (when one or some of plugins fails).

repeatedly · 2014-04-24T05:54:34Z

No, my approach is succeeded (when no plugin fail) or raises an error (when one or some of plugins fails).

My 'succeeded' means out_copy processes remaining plugins if error occurred.
From OutputChain user side, Output plugins seems to process the event correctly but raise an error.

If we select your approach, out_copy should keep failed plugin lists for avoiding log duplication.
Your approach seems to good for bulk insert on database but I think hard to use on Fluentd like pluggable system because there are many error reasons.

P.S.
After support error stream, this restriction will be removed or relaxed.

dterror-zz · 2014-04-24T21:15:47Z

I see your point @repeatedly. Covering more cases does make sense. I'll change the implementation.
However, I think ignore_indivisual_errors [1, 2] is a bit weird for referencing plugin indexes. How about adding it as a <store> arg. Something like:

<match foo.bar.*>
  type copy
  <store>
    type s3
    #...
  </store>
  <store ignore-failure>
    type elasticsearch
    #...
  </store>
</match>

repeatedly · 2014-04-24T21:32:04Z

Ouch, I deleted the comment... ;(

@dterror Fluentd plugin has built-in id parameter so using id with ignore_indivisual_errors is easy to implement.

But your approach also seems to easy.
Either way's fine with me :)

@sonots @kiyoto What do you think?

sonots · 2014-04-25T01:44:27Z

<store ignore-failure>

looks nice for me.

repeatedly · 2014-04-25T03:49:47Z

Another one idea.

This issue will be resolved by error stream.
So temporary, releasing out_copy_xxx.rb which includes this feature and add link to out_copy document.

I'm not sure which is the best...

dterror-zz · 2014-04-25T14:15:01Z

How long until error stream is out? what will it look like and is there anything I can do help? If that's the 'proper' way to go, I think it's fair to wait.

dterror-zz · 2014-04-28T20:05:27Z

That is, provided error stream is outlined and is not going to take too long. I still think ignore_individual_options is a very reasonable as a solution.

repeatedly · 2014-04-29T18:48:42Z

In my plan, v1 includes error stream and now working to implement v1 features.
The release date is not fixed but I want to release v1 series in this summer.

sonots · 2014-04-30T06:26:10Z

Since it looks ignore_individual_errors will be obsolete after error stream is implemented, I created another plugin just for the current use. See https://github.com/sonots/fluent-plugin-copy_ex

dterror-zz added 2 commits April 22, 2014 17:13

Adding fault_tolerant option to OutputChain

52d77b1

it will only raise an exception if all outputs fail.

Adding ignore_individual_errors option to out_copy

5f4eccf

Uses a fault tolerant output chain to only fail if all outputs fail.

repeatedly reviewed Apr 23, 2014
View reviewed changes

refactoring for code conventions

a0bb828

repeatedly reviewed Apr 23, 2014
View reviewed changes

sonots reviewed Apr 23, 2014
View reviewed changes

Raising OutputChainError instead of last exception

228de0d

Making sure to log all exceptions so that information is not lost.

dterror-zz mentioned this pull request Apr 25, 2014

Adds buffer_ignore_exceeded_chunk option to Buffer #306

Closed

dterror-zz closed this Apr 30, 2014

repeatedly added the enhancement label Jun 27, 2014

repeatedly mentioned this pull request Aug 20, 2014

Copy plugin delivery control #413

Closed

Misterhex mentioned this pull request Sep 26, 2018

Crash when one of many elasticsearch cluster cannot be connected when fluentd start. uken/fluent-plugin-elasticsearch#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore individual options for out copy #303

Ignore individual options for out copy #303

dterror-zz commented Apr 22, 2014

repeatedly Apr 23, 2014

repeatedly Apr 23, 2014

repeatedly commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

sonots Apr 23, 2014

repeatedly commented Apr 23, 2014

sonots Apr 23, 2014

dterror-zz Apr 23, 2014

sonots Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

sonots commented Apr 24, 2014

sonots commented Apr 24, 2014

repeatedly commented Apr 24, 2014

sonots commented Apr 24, 2014

repeatedly commented Apr 24, 2014

sonots commented Apr 24, 2014

repeatedly commented Apr 24, 2014

dterror-zz commented Apr 24, 2014

repeatedly commented Apr 24, 2014

sonots commented Apr 25, 2014

repeatedly commented Apr 25, 2014

dterror-zz commented Apr 25, 2014

dterror-zz commented Apr 28, 2014

repeatedly commented Apr 29, 2014

sonots commented Apr 30, 2014

Ignore individual options for out copy #303

Ignore individual options for out copy #303

Conversation

dterror-zz commented Apr 22, 2014

repeatedly Apr 23, 2014

Choose a reason for hiding this comment

repeatedly Apr 23, 2014

Choose a reason for hiding this comment

repeatedly commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

sonots Apr 23, 2014

Choose a reason for hiding this comment

repeatedly commented Apr 23, 2014

sonots Apr 23, 2014

Choose a reason for hiding this comment

dterror-zz Apr 23, 2014

Choose a reason for hiding this comment

sonots Apr 23, 2014

Choose a reason for hiding this comment

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

dterror-zz commented Apr 23, 2014

repeatedly commented Apr 23, 2014

sonots commented Apr 24, 2014

sonots commented Apr 24, 2014

repeatedly commented Apr 24, 2014

sonots commented Apr 24, 2014

repeatedly commented Apr 24, 2014

sonots commented Apr 24, 2014

repeatedly commented Apr 24, 2014

dterror-zz commented Apr 24, 2014

repeatedly commented Apr 24, 2014

sonots commented Apr 25, 2014

repeatedly commented Apr 25, 2014

dterror-zz commented Apr 25, 2014

dterror-zz commented Apr 28, 2014

repeatedly commented Apr 29, 2014

sonots commented Apr 30, 2014