Add a parameter for identifying log streams that shall be processed independently #5

thomasschickinger · 2016-12-14T10:34:26Z

No description provided.

…ndependently.

…th the message field.

igorpeshansky · 2016-12-14T17:19:40Z

README.rdoc

@@ -44,7 +44,11 @@ The plugin supports the following parameters:

 [message]  Name of the field in the JSON record that contains the
           single-line log messages that shall be scanned for exceptions.
-           Default: 'message'.
+           If the setting is '', it will automatically be changed to 'message'


If this is set to '', the plugin will try 'message' and 'log', in that order.

igorpeshansky · 2016-12-14T17:20:39Z

README.rdoc

             Default: 1000.

+[max_bytes]  Maximum number of bytes in a detected exception stack trace.
+             If this maximum number is exceeded, the detected exception stack


"detected exception stack trace that has been detected so far" has one too many instances of "detected". I'd lose the first one.

igorpeshansky · 2016-12-14T17:21:36Z

README.rdoc

+             Zero means no limit.
+             Default: 0.
+
+[stream]  Name of a field in the JSON record that contains the name of a


s/a field/the field/.

igorpeshansky · 2016-12-14T17:22:49Z

README.rdoc

+             Default: 0.
+
+[stream]  Name of a field in the JSON record that contains the name of a
+          logical log stream within the 'real' log stream.


Nit: you use single quotes above to delimit field names. Here, 'real' is just quoting a vague concept, so I'd use double quotes instead. Also below.

igorpeshansky · 2016-12-14T17:24:17Z

README.rdoc

@@ -44,7 +44,11 @@ The plugin supports the following parameters:

 [message]  Name of the field in the JSON record that contains the
           single-line log messages that shall be scanned for exceptions.
-           Default: 'message'.
+           If the setting is '', it will automatically be changed to 'message'


This parameter is only applicable to structured (JSON) log streams. Also below.

igorpeshansky · 2016-12-14T17:24:53Z

README.rdoc

+          The exception detection is handled separately for each logical
+          log stream, i.e., exceptions will be detected even if the messages
+          for the logical log streams are interleaved in the 'real' log stream.
+          Consequently, only records that belong to the same stream will be


the same logical stream.

igorpeshansky · 2016-12-14T17:25:38Z

lib/fluent/plugin/out_detect_exceptions.rb

@@ -34,6 +34,8 @@ class DetectExceptionsOutput < Output
    config_param :max_lines, :integer, default: 1000
    desc 'Maximum number of bytes to flush (0 means no limit). Default: 0.'
    config_param :max_bytes, :integer, default: 0
+    desc 'Separate log streams by this field (if set). Default: nil.'


nil or ''?

igorpeshansky · 2016-12-14T17:27:38Z

lib/fluent/plugin/out_detect_exceptions.rb

@@ -84,17 +86,19 @@ def emit(tag, es, chain)

    def process_record(tag, time_sec, record)
      synchronize do
-        unless @accumulators.key?(tag)
+        accumulator_key = [tag]


I'd name it for its logical content, i.e., log_id, rather its intended purpose (i.e., accumulator_key).

igorpeshansky · 2016-12-14T17:35:34Z

test/plugin/test_out_detect_exceptions.rb

+    log_entry
+  end
+
+  def feed_lines(driver, t, messages, stream: nil)


Since stream as a named argument (rather than a positional with a default value), you should be able to keep *messages (they shouldn't conflict) and save yourself a whole lot of square brackets.

Good suggestion. Thanks.

igorpeshansky

Approving with a couple of optional comments.

igorpeshansky · 2016-12-19T16:58:44Z

lib/fluent/plugin/out_detect_exceptions.rb

@@ -34,6 +34,8 @@ class DetectExceptionsOutput < Output
    config_param :max_lines, :integer, default: 1000
    desc 'Maximum number of bytes to flush (0 means no limit). Default: 0.'
    config_param :max_bytes, :integer, default: 0
+    desc "Separate log streams by this field (if set). Default: ''."


[Optional] I'd suggest replacing "if set" with "unless empty" or something. Also, you could avoid the double quotes by saying "Default: empty". In fact, I'm not even sure you need to be explicit about the defaults for any of these in the descriptions -- the only reason it was specified for :languages was because [] isn't immediately associated with "all". The rest are probably superfluous.

Will go with not mentioning the default. This is also consistent with, e.g., remove_tag_prefix.

Might as well clean up the defaults for max_lines and max_bytes...

max_lines has a default that is 'surprising'. I'll thus leave it in (and then for symmetry also the one for max_bytes).

igorpeshansky · 2016-12-19T17:11:24Z

lib/fluent/plugin/out_detect_exceptions.rb

@@ -84,17 +86,19 @@ def emit(tag, es, chain)

    def process_record(tag, time_sec, record)
      synchronize do
-        unless @accumulators.key?(tag)
+        log_id = [tag]
+        log_id.push(record.fetch(stream, '')) unless stream.empty?


[Optional] I think the config parameters manifest as instance fields, so it might pay to be explicit and use @stream instead of just stream (makes code searches much simpler too). Also for the other config parameters -- sorry I didn't pick up on this earlier. Feel free to defer this to a later cleanup.

Cleanup done.

thomasschickinger added 2 commits December 14, 2016 11:32

Add a parameter for identifying log streams that shall be processed i…

5fb2780

…ndependently.

Add description to README and change default to '' for consistency wi…

490724e

…th the message field.

thomasschickinger requested a review from igorpeshansky December 14, 2016 10:47

Improve description in README.

ee0229f

igorpeshansky requested changes Dec 14, 2016

View reviewed changes

Address review comments by igorpeshansky@

f10fc4c

igorpeshansky approved these changes Dec 19, 2016

View reviewed changes

2nd round of comments.

bdf1177

thomasschickinger merged commit e85f69d into master Dec 19, 2016

thomasschickinger deleted the separate_streams branch December 19, 2016 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a parameter for identifying log streams that shall be processed independently #5

Add a parameter for identifying log streams that shall be processed independently #5

thomasschickinger commented Dec 14, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky Dec 14, 2016

thomasschickinger Dec 15, 2016

igorpeshansky left a comment

igorpeshansky Dec 19, 2016

thomasschickinger Dec 19, 2016 •

edited by igorpeshansky

Loading

igorpeshansky Dec 19, 2016

thomasschickinger Dec 19, 2016

igorpeshansky Dec 19, 2016

thomasschickinger Dec 19, 2016

Add a parameter for identifying log streams that shall be processed independently #5

Add a parameter for identifying log streams that shall be processed independently #5

Conversation

thomasschickinger commented Dec 14, 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

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

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasschickinger Dec 19, 2016 • edited by igorpeshansky Loading

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

thomasschickinger Dec 19, 2016 •

edited by igorpeshansky

Loading