Merge ValuesParser and TypeConverter into Parser #1286
Conversation
… inter-class dependency
And moved ValuesParser to Compat module completely.
The reason of this changes are:
* TypeConverter was old-fashioned module with configuration params ("types_delimiter", "types_label_delimiter") and cannot log anything via PluginLogger
* Many Parser plugins have choosen ValuesParser as superclass, but doesn't use "Values" features (with `@keys`), but uses just TypeConverter features
* "tag_key" feature is now supported by "extract" plugin helper, so parser should not support it
|
@repeatedly could you review this change? |
|
@repeatedly ping? |
| types = {} | ||
| conf['types'].split(delimiter).each do |pair| | ||
| key, value = pair.split(label_delimiter, 2) | ||
| if value.start_with?("time#{label_delimiter}") |
There was a problem hiding this comment.
It should be. I'll add a fix.
| end | ||
|
|
||
| class JSONParser < Fluent::Plugin::JSONParser | ||
| include TypeConverterCompatParameters |
There was a problem hiding this comment.
Yes. It is a subclass of Fluent::Plugin::JSONParser, and it doesn't include it.
There was a problem hiding this comment.
JSONParser doesn't use TypeConverter feature.
Do you have a plan to update JSONParser?
There was a problem hiding this comment.
Yes, I missed to update code of JSONParser. I pushed some commits including one to do it.
|
Idea is good for me and add 2 comments. One concern is regexp parser performance because this parser is used in high-volume environment. |
|
I re-think the |
|
@repeatedly I made a very simple benchmark, and found that multi value return is faster than passing blocks: https://gist.github.com/tagomoris/7bca9fe4fdc3201f691094e6aad3b177 |
|
@repeatedly do you have anything more? |
|
LGTM. If we have a performance issue, we should rewrite parser code in the future. |
This change does:
Many parser plugins (including 3rd party ones) needs features of TypeConverter and time parser.
These features are provided only in ValuesParser, but ValuesParser is originally for parsers which supports fixed number fields, like tsv and csv.
(ltsv parser inherits ValuesParser for TypeConverter, and it has unbelievable implementation...)
This change makes parser plugins to:
hashandarrayto provide safe structured values without additional parametersestimate_current_eventas a configuration parameter (can be overloaded byconfig_set_defaultof 3rd party parser plugins)See
parser_ltsv.rbandparser_regexp.rbto show how this change makes writing plugins understandable.