parser_json: Add stream_buffer_size config param #2381

benwh · 2019-04-14T01:06:16Z

Which issue(s) this PR fixes:
Fixes #609

What this PR does / why we need it:
Allow configuration of the size of the buffer that Yajl uses when parsing streaming input.

The advantage of this is that when using out_exec_filter, and parsing
as JSON, it's now possible to configure this plugin to avoid having to
wait for 8192 bytes of data to be parsed before events are emitted.

Docs Changes: fluent/fluentd-docs#631

Release Note:
Use PR title.

See fluent/fluentd#2381 In addition to documenting the parameter, update the `in_exec` and `out_exec_filter` articles to provide a workaround for users encountering this problem. This note exists in the v0.12 version of the `in_exec` page but was absent in the v1.0 docs, so add the whole note. Closes fluent#624 Signed-off-by: Ben Wheatley <contact@benwh.com>

benwh · 2019-04-14T01:44:27Z

lib/fluent/plugin/out_exec_filter.rb

@@ -95,6 +95,7 @@ class ExecFilterOutput < Output
    COMPAT_PARSE_PARAMS = {
      'out_format' => '@type',
      'out_keys' => 'keys',
+      'out_stream_buffer_size' => 'stream_buffer_size',


The only reason that this was added was to shave another 30s off the tests

benwh · 2019-04-14T01:53:04Z

test/plugin/test_parser_json.rb

@@ -111,4 +111,29 @@ def test_parse_with_keep_time_key_without_time_format(data)
      assert_equal text, record['time']
    end
  end
+
+  data('yajl' => 'yajl')
+  def test_yajl_parse_io_with_buffer_smaller_than_input(data)


I was considering adding a test case for the inverse of this (input smaller than buffer) but didn't do this because it seemed like it would be quite awkward to implement - due to needing to assert on the parser not emitting an event - and probably not provide much value.
Happy to reconsider this if anyone has a suggestion on how this could be done neatly though!

repeatedly · 2019-04-15T01:35:08Z

lib/fluent/plugin/parser_json.rb

+      # The Yajl library defines a default buffer size of 8092 when parsing
+      # from IO streams, so maintain this for backwards-compatibility.
+      # https://www.rubydoc.info/github/brianmario/yajl-ruby/Yajl%2FParser:parse
+      config_param :stream_buffer_size, :integer, default: 8092


Please add :desc parameter to explain the parameter. This is only for yajl.

If you use desc method, not :desc parameter, it should be put on top of config_param.
Currently, desc 'Set the buffer size that Yajl will use when parsing streaming input' is for time_tupe, not stream_buffer_size.
See https://docs.fluentd.org/v1.0/articles/api-plugin-base

Ah, that was a silly mistake, thanks for the pointer! Fixed.

repeatedly · 2019-04-15T01:35:34Z

test/plugin/test_parser_json.rb

@@ -111,4 +111,29 @@ def test_parse_with_keep_time_key_without_time_format(data)
      assert_equal text, record['time']
    end
  end
+
+  data('yajl' => 'yajl')


Don't use data for only 1 pattern.

Good point, sorted :)

benwh · 2019-04-16T16:29:34Z

Hi @repeatedly, those issues are fixed so could you take another look when you get some time? Thanks!

ganmacs · 2019-04-22T03:03:41Z

lib/fluent/plugin/parser_json.rb

@@ -30,6 +30,12 @@ class JSONParser < Parser
      desc 'Set JSON parser'
      config_param :json_parser, :enum, list: [:oj, :yajl, :json], default: :oj

+      # The Yajl library defines a default buffer size of 8092 when parsing


[nits] It may be better a default buffer size is 8192 because of brianmario/yajl-ruby@9d4cca0

FYI: brianmario/yajl-ruby#192

Definitely not a nit; thanks for picking up on this! I did think that 8092 bytes was a bit of a strange size for a buffer, but didn't look into it 🤦‍♂️.
Fixed and I'll update the docs PR too.

ganmacs

LGTM although I left a few nits comments.

ganmacs · 2019-04-22T03:28:51Z

test/plugin/test_parser_json.rb

+                     )
+    text = "100"
+
+    waiting(5) do


Does it need? this test suit seems not to block since L134.

Yes it's not actually required for the test to work, I put it there to protect against any regression in the code being tested (or the test scaffolding) that might cause the test to fail, because if it does then it will block forever which would be quite annoying when it runs in CI, if nothing else.

Allow configuration of the size of the buffer that Yajl uses when parsing streaming input. The advantage of this is that when using `out_exec_filter`, and parsing as JSON, it's now possible to configure this plugin to avoid having to wait for 8192 bytes of data to be parsed before events are emitted. Configuration in the `out_exec_filter` tests has been modified to use this parameter, as it shaves 60 seconds off the test run time. Signed-off-by: Ben Wheatley <contact@benwh.com>

See fluent/fluentd#2381 In addition to documenting the parameter, update the `in_exec` and `out_exec_filter` articles to provide a workaround for users encountering this problem. This note exists in the v0.12 version of the `in_exec` page but was absent in the v1.0 docs, so add the whole note. Closes fluent#624 Signed-off-by: Ben Wheatley <contact@benwh.com>

repeatedly · 2019-04-25T08:01:05Z

Thx!

benwh mentioned this pull request Apr 14, 2019

parser_json: Document new stream_buffer_size param fluent/fluentd-docs#631

Open

benwh commented Apr 14, 2019

View reviewed changes

repeatedly reviewed Apr 15, 2019

View reviewed changes

benwh force-pushed the configurable-json-stream-buffer branch from e6ef491 to 09ae8bc Compare April 15, 2019 09:06

benwh force-pushed the configurable-json-stream-buffer branch from 09ae8bc to 9f9a631 Compare April 17, 2019 18:02

ganmacs reviewed Apr 22, 2019

View reviewed changes

ganmacs approved these changes Apr 22, 2019

View reviewed changes

benwh force-pushed the configurable-json-stream-buffer branch from 9f9a631 to 34567ee Compare April 22, 2019 20:32

benwh force-pushed the configurable-json-stream-buffer branch from 34567ee to 811390d Compare April 22, 2019 20:34

repeatedly merged commit 73a4f01 into fluent:master Apr 25, 2019

benwh deleted the configurable-json-stream-buffer branch February 23, 2020 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser_json: Add stream_buffer_size config param #2381

parser_json: Add stream_buffer_size config param #2381

benwh commented Apr 14, 2019 •

edited

Loading

benwh Apr 14, 2019

benwh Apr 14, 2019

repeatedly Apr 15, 2019

benwh Apr 15, 2019

repeatedly Apr 17, 2019

benwh Apr 17, 2019

repeatedly Apr 15, 2019

benwh Apr 15, 2019

benwh commented Apr 16, 2019

ganmacs Apr 22, 2019

ganmacs Apr 22, 2019

benwh Apr 22, 2019

ganmacs left a comment

ganmacs Apr 22, 2019

benwh Apr 22, 2019

repeatedly commented Apr 25, 2019

parser_json: Add stream_buffer_size config param #2381

parser_json: Add stream_buffer_size config param #2381

Conversation

benwh commented Apr 14, 2019 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwh commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganmacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repeatedly commented Apr 25, 2019

benwh commented Apr 14, 2019 •

edited

Loading