Automatically add missing new lines when combining exception stacks and make remove_tag_prefix mandatory by thomasschickinger · Pull Request #10 · GoogleCloudPlatform/fluent-plugin-detect-exceptions

thomasschickinger · 2017-03-09T09:47:10Z

This PR solves the following issues:

Text logs on GCE do not contain a trailing line end. Hence, we need to add it when combining exception stacks.
The fact that remove_tag_prefix is optional is a pitfall for users because re-emitting a log entry with the same tag causes an infinite recursion in fluentd.
Flushing individual text log lines did not work because the first_record field was only populated for JSON logs.

The version number is bumped to 0.0.6

bernii · 2017-04-06T16:24:03Z

I'm getting a following exception when using this branch

2017-04-06 16:00:14 +0000 [warn]: fluent/root_agent.rb:199:handle_emits_error: emit transaction failed: error_class=TypeError error="no implicit conversion of nil into String" tag="raw.coreos-systemd"
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/exception_detector.rb:229:in `block in combined_message'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/exception_detector.rb:228:in `each'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/exception_detector.rb:228:in `combined_message'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/exception_detector.rb:246:in `flush'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/exception_detector.rb:288:in `update_buffer'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/exception_detector.rb:221:in `push'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/out_detect_exceptions.rb:101:in `block in process_record'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/out_detect_exceptions.rb:131:in `synchronize'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/out_detect_exceptions.rb:131:in `synchronize'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/out_detect_exceptions.rb:88:in `process_record'
  2017-04-06 16:00:14 +0000 [warn]: fluent/event_router.rb:81:emit: /usr/local/bundle/gems/fluent-plugin-detect-exceptions-0.0.6/lib/fluent/plugin/out_detect_exceptions.rb:80:in `block in emit'

thomasschickinger · 2017-04-06T16:29:45Z

Judging from the line where this error is reported (exception_detector.rb:229), I'd guess that $RS is not defined in your environment. Could you try replacing it with "\n" and check whether the error disappears?

bernii · 2017-04-06T17:16:02Z

Yeah I was not sure, not a Rubyist here ;-) You're right, that fixes it.
I'm using the official ruby:2.2.0 image from dockerhub, wondering if I should use something different or if I can somehow set $RS in the env?

igorpeshansky · 2017-04-06T19:49:35Z

lib/fluent/plugin/exception_detector.rb

+    def combined_message
+      combined = ''
+      @messages.each do |m|
+        combined << $RS if !combined.empty? && !combined.end_with?("\n")


Can we program defensively and use "\n" if $RS is nil?

Done. Used a member variable for that in order to ensure that this is looked up only once.

This is a static value that should never change. How about making it a class variable instead?

igorpeshansky · 2017-04-06T19:52:07Z

README.rdoc

 {output plugin for fluentd}[http://docs.fluentd.org/articles/output-plugin-overview]
-which scans a log stream text messages or JSON records for multi-line exception
-stack traces: If a consecutive sequence of log messages forms an exception stack
+which scans a log stream text messages or JSON records containing single


a log stream of text messages?

igorpeshansky · 2017-04-06T19:53:00Z

README.rdoc

 are so similar (e.g. because they contain the timestamp of the log entry) that
 this loss of information is irrelevant.

+When combining exception lines, it is ensured that they enter with a line


Sorry, can't parse this. Did you mean s/enter/end/?

Yes, thanks.

igorpeshansky · 2017-04-06T19:53:18Z

README.rdoc

 this loss of information is irrelevant.

+When combining exception lines, it is ensured that they enter with a line
+separator. If they don't, a line separator is added to the original log message.


The second sentence is redundant (implied by "is ensured").

igorpeshansky · 2017-04-06T19:56:00Z

README.rdoc

@@ -53,7 +57,9 @@ The plugin supports the following parameters:
                     a record. A prefix has to be a complete tag part.


Let's change the heading to [remove_tag_prefix (required)] for consistency with other fluentd docs (e.g., in_tail) and make it the first parameter.

igorpeshansky · 2017-04-06T19:57:35Z

README.rdoc

                     tag foo.bar.baz is transformed to bar.baz and the input tag
-                     'foofoo.bar' is not modified. Default: empty string.
+                     'foofoo.bar' is not modified.
+                     Removing a tag prefix is needed to avoid infinite


How about we put it in terms of what the user can/has to do, e.g.:

This must be non-empty to avoid infinite recursion in fluentd.

Speaking of which, do you want to check that this is non-empty and reject the config if it is? Removing the default simply ensures that this is specified by the user, not that it has a non-empty value.

What about the second part of the suggestion (writing code to check that this is non-empty)?

Check and test added.

igorpeshansky · 2017-04-06T19:59:58Z

fluent-plugin-detect-exceptions.gemspec

  gem.required_ruby_version = Gem::Requirement.new('>= 2.0')

-  gem.files         = Dir['**/*'].keep_if { |file| File.file?(file) }
+  gem.files         = Dir['**/*'].keep_if{ |file| File.file?(file) &&


Just curious: why did you have to remove the space before the opening brace?

I didn't. Fixed.

igorpeshansky · 2017-04-06T20:00:17Z

fluent-plugin-detect-exceptions.gemspec


-  gem.files         = Dir['**/*'].keep_if { |file| File.file?(file) }
+  gem.files         = Dir['**/*'].keep_if{ |file| File.file?(file) &&
+                                                  !file.end_with?('gem') }


Nit: should this be !file.end_with?('.gem')?

igorpeshansky · 2017-04-06T20:13:25Z

lib/fluent/plugin/exception_detector.rb

      force_flush if @max_lines > 0 && @messages.length == @max_lines
    end

+    def combined_message


I'm not happy with defining a method for this -- too much Ruby magic. You are essentially injecting newlines after trace lines that don't have them, right? Why not do it as a comprehension, e.g.:

combined_message = @messages.inject([]) {|r, e| r << e; r << $RS unless e.empty? || e.end_with?("\n"); r}.join

thomasschickinger · 2017-04-10T07:22:45Z

All comments addressed. PTAL.

igorpeshansky · 2017-04-13T22:46:12Z

lib/fluent/plugin/exception_detector.rb

+    def combined_message
+      combined = ''
+      @messages.each do |m|
+        combined << $RS if !combined.empty? && !combined.end_with?("\n")


This is a static value that should never change. How about making it a class variable instead?

igorpeshansky · 2017-04-13T22:48:35Z

README.rdoc

                     tag foo.bar.baz is transformed to bar.baz and the input tag
-                     'foofoo.bar' is not modified. Default: empty string.
+                     'foofoo.bar' is not modified.
+                     Removing a tag prefix is needed to avoid infinite


What about the second part of the suggestion (writing code to check that this is non-empty)?

thomasschickinger · 2017-04-18T18:59:33Z

Hi Igor, comments addressed. PTAL.

timoreimann · 2017-09-13T11:13:15Z

we've been running into the same problem this PR fixes recently.

@igorpeshansky is there still anything left blocking from moving forward with this PR?

mirwan · 2017-12-22T10:53:31Z

Hi
About point 2, I proposed PR#20. The combination of remove_tag_prefix and prepend_tag_prefix could prevent from entering an infinite loop.

neeraj9194 · 2018-05-02T05:09:45Z

is this ever going to release???

yanweiguo · 2018-05-25T22:24:59Z

Got the same issue this PR fixes. @igorpeshansky any update?

aplsms · 2018-10-01T23:17:27Z

any updates?

irizzant · 2019-01-21T15:41:04Z

I'm having the same issues with Java exceptions not having and ending new line char.
Any news?

callicles · 2019-08-10T00:16:24Z

Same here @igorpeshansky would be great if that could be merged

jkohen · 2019-08-12T21:03:28Z

@igorpeshansky what does it take to resolve this PR?

Automatically add missing new lines when combining exception stacks.

6bb2a77

thomasschickinger assigned igorpeshansky Mar 9, 2017

thomasschickinger mentioned this pull request Apr 6, 2017

join_separator support for the output log line #14

Closed

igorpeshansky suggested changes Apr 6, 2017

View reviewed changes

thomasschickinger added 2 commits April 10, 2017 09:19

Address review comments.

33bb985

Fix indentation in README.rdoc.

31b8f31

igorpeshansky suggested changes Apr 13, 2017

View reviewed changes

Address second round of comments by igorpeshansky.

b2f4f31

bmoyles0117 force-pushed the master branch from 5654ac1 to b2ddfd5 Compare October 2, 2017 19:49

yanweiguo mentioned this pull request May 25, 2018

Use detect_exceptions for /var/log to combine multi line logs knative/serving#979

Merged

		@@ -53,7 +57,9 @@ The plugin supports the following parameters:
		a record. A prefix has to be a complete tag part.

Comments

Conversation

thomasschickinger commented Mar 9, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernii commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasschickinger commented Apr 6, 2017

Uh oh!

bernii commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasschickinger commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasschickinger commented Apr 18, 2017

Uh oh!

timoreimann commented Sep 13, 2017

Uh oh!

mirwan commented Dec 22, 2017

Uh oh!

neeraj9194 commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanweiguo commented May 25, 2018

Uh oh!

aplsms commented Oct 1, 2018

Uh oh!

irizzant commented Jan 21, 2019

Uh oh!

callicles commented Aug 10, 2019

Uh oh!

jkohen commented Aug 12, 2019

thomasschickinger commented Mar 9, 2017 •

edited by igorpeshansky

Loading

bernii commented Apr 6, 2017 •

edited

Loading

neeraj9194 commented May 2, 2018 •

edited

Loading