in_monitor_agent: get_monitor_info: fix NoMethodError

[shuji-koike]

NoMethodError when using in_monitor_agent with out_null plugin #1364

This is my fix for #1364.

---

I thought it may be better to add a resuce e: NoMethodError around here -> https://github.com/shuji-koike/fluentd/blob/be7ffb2/lib/fluent/plugin/in_monitor_agent.rb#L344-L346
If so, I will fix my commit.

[tagomoris]

Code looks good to me.
Adding rescue NoMethodError is good idea. That should have log.error_backtrace to show where the bug is clearly.

[repeatedly]

Good catch :)
And additional commits are welcome.

[shuji-koike]

@tagomoris san

Adding rescue NoMethodError with log.error_backtrace would rather increase logs.
I may want to reduce (suppress) logs for this occasion, because @buffer may be nil in some plugins and (I believe) it is not a bug or an error.

So, may I leave my fix as is?

[tagomoris]

@shuji-koike it makes sense. But it's worth to provide information to make it easy to debug such problems.
So, how about to show warnings at the only first time of rescue such exception? It's a kind of a bit messy code, but it satisfies requirements both of users and developers.

# initialize @first_warn = false # in #initialize
 rescue => e
  log.error # brabrabra
  if !@first_warn
    @first_warn = true
    log.error_stacktrace
  end

[shuji-koike]

@tagomoris san

I've updated my fix -> b38a791

I didn't notice there was a similar issue -> #1335, sorry!

[tagomoris]

Hmm, This fix still have some problems:

• Including the whole logging in unless @first_warn makes that it warns NoMethodError only once (maybe just after Fluentd startup)
• Users may miss just only one warning... we should warn it everytime (without backtrace)
• this patch uses warn about NoMethodError itself, but uses error_backtrace (it shows backtrace in ERROR level)

We should show error level log everytime (out of unless @first_warn clause), and use error_backtrace in unless @first_warn.

[tagomoris]

@shuji-koike ping?

[shuji-koike]

@tagomoris san
Sorry for my late reply, and a Happy New Year!
I updated my fix -> 625752c
Since fluentd will suppress duplicate stacktraces, I removed the @first_warn flag.

[shuji-koike]

I would have to say that I am not really convinced with this fix.

I understand that logging is important.
But users have nothing to do with this error.

I think the real problem is...

In 0.12.x, @buffer is defined only in BufferedOutput plugins.
https://github.com/shuji-koike/fluentd/blob/v0.12.30/lib/fluent/output.rb#L213

in_monitor_agent checks whether @buffer is defined to avoid (suppress) errors.
https://github.com/shuji-koike/fluentd/blob/v0.14.9/lib/fluent/plugin/in_monitor_agent.rb#L253-L254

In 0.14.x, @buffer will be defined in all Output plugins.
https://github.com/shuji-koike/fluentd/blob/v0.14.9/lib/fluent/plugin/output.rb#L185

Therefore, checking instance_variable_defined?(:@buffer) may have become pointless.

I removed the @buffer.nil? check from my fix (when I added the rescue clause), but may I put them back?

[tagomoris]

"suppress repeated stacktrace" option will suppress the stacktraces which is just equal to the previous one. If any error occurs in other place of code, the same stacktrace will be shown in log.
Otherwise, "suppressed same stacktrace" log messages are shown always. It looks a bit messy for me.
So, IMO it's better not to call log.error_backtrace twice and more.

[tagomoris]

Basically, checking @buffer.nil? is good idea, in addition to instance_variable_defined?(:@buffer), because there are still some plugins without @buffer instance variables (in match sections), like Fluent::Plugin::BareOutput.
But it's not the main issue about this pull-request.

This pull-request is to show where the NoMethodError occurs and where the bug hits. This requires stacktrace for further investigation, so we need to handle NoMethodError in special way.

[shuji-koike]

@tagomoris san

Thanks for your advice 😸
I added nil? check "in addition to instance_variable_defined", and brought back the @first_warn flag. -> 919ebb2

[tagomoris]

LGTM

[tagomoris]

@shuji-koike Merged. Thank you for contribution!