Skip to content

Commit

Permalink
Set Oj.default_options only once when fluentd is started
Browse files Browse the repository at this point in the history
It shouldn't be set by each plugins, because later loaded plugins will
overwrites it.

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
  • Loading branch information
ashie committed Jul 8, 2021
1 parent beae9f7 commit 3217fe4
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 18 deletions.
2 changes: 1 addition & 1 deletion lib/fluent/env.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module Fluent
DEFAULT_PLUGIN_DIR = ENV['FLUENT_PLUGIN'] || '/etc/fluent/plugin'
DEFAULT_SOCKET_PATH = ENV['FLUENT_SOCKET'] || '/var/run/fluent/fluent.sock'
DEFAULT_BACKUP_DIR = ENV['FLUENT_BACKUP_DIR'] || '/tmp/fluent'
DEFAULT_OJ_OPTIONS = Fluent::OjOptions.get_options
DEFAULT_OJ_OPTIONS = Fluent::OjOptions.load_env
DEFAULT_DIR_PERMISSION = 0755
DEFAULT_FILE_PERMISSION = 0644

Expand Down
20 changes: 20 additions & 0 deletions lib/fluent/oj_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,26 @@ class OjOptions
'use_to_json': true
}

@@available = false

def self.available?
@@available

This comment has been minimized.

Copy link
@cosmo0920

cosmo0920 Jul 9, 2021

Contributor

Sorry for being late but how about using Fluent::VariableStore for storing available states.
I think that we needn't use VariableStore but I feel that potential reloading prevention is occurred by this change.

end

def self.load_env
options = self.get_options
begin
require 'oj'
Oj.default_options = options
@@available = true
rescue LoadError
@@available = false
end
options
end

private

def self.get_options
options = {}
DEFAULTS.each { |key, value| options[key] = value }
Expand Down
16 changes: 9 additions & 7 deletions lib/fluent/plugin/formatter_json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#

require 'fluent/plugin/formatter'
require 'fluent/env'
require 'fluent/oj_options'

module Fluent
module Plugin
Expand All @@ -30,12 +30,14 @@ class JSONFormatter < Formatter
def configure(conf)
super

begin
raise LoadError unless @json_parser == 'oj'
require 'oj'
Oj.default_options = Fluent::DEFAULT_OJ_OPTIONS
@dump_proc = Oj.method(:dump)
rescue LoadError
if @json_parser == 'oj'
if Fluent::OjOptions.available?
@dump_proc = Oj.method(:dump)
else
log.info "Oj isn't installed, fallback to Yajl as json parser"
@dump_proc = Yajl.method(:dump)
end
else
@dump_proc = Yajl.method(:dump)
end

Expand Down
5 changes: 2 additions & 3 deletions lib/fluent/plugin/parser_json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
#

require 'fluent/plugin/parser'
require 'fluent/env'
require 'fluent/time'
require 'fluent/oj_options'

require 'yajl'
require 'json'
Expand Down Expand Up @@ -50,8 +50,7 @@ def configure(conf)
def configure_json_parser(name)
case name
when :oj
require 'oj'
Oj.default_options = Fluent::DEFAULT_OJ_OPTIONS
raise LoadError unless Fluent::OjOptions.available?
[Oj.method(:load), Oj::ParseError]
when :json then [JSON.method(:load), JSON::ParserError]
when :yajl then [Yajl.method(:load), Yajl::ParseError]
Expand Down
4 changes: 2 additions & 2 deletions test/test_event_time.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ class EventTimeTest < Test::Unit::TestCase

test 'Oj.dump' do
time = Fluent::EventTime.new(100)
require 'fluent/env'
Oj.default_options = Fluent::DEFAULT_OJ_OPTIONS
require 'fluent/oj_options'
Fluent::OjOptions.load_env
assert_equal('{"time":100}', Oj.dump({'time' => time}))
assert_equal('["tag",100,{"key":"value"}]', Oj.dump(["tag", time, {"key" => "value"}], mode: :compat))
end
Expand Down
26 changes: 21 additions & 5 deletions test/test_oj_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
require 'fluent/oj_options'

class OjOptionsTest < ::Test::Unit::TestCase
begin
require 'oj'
@@oj_is_avaibale = true
rescue LoadError
@@oj_is_avaibale = false
end

setup do
@orig_env = {}
ENV.each do |key, value|
Expand All @@ -15,25 +22,34 @@ class OjOptionsTest < ::Test::Unit::TestCase
@orig_env.each { |key, value| ENV[key] = value }
end

sub_test_case "OjOptions" do
test "available?" do
assert_equal(@@oj_is_avaibale, Fluent::OjOptions.available?)
end

sub_test_case "set by environment variable" do
test "when no env vars set, returns default options" do
ENV.delete_if { |key| key.start_with?("FLUENT_OJ_OPTION_") }
assert_equal Fluent::OjOptions::DEFAULTS, Fluent::OjOptions.get_options
defaults = Fluent::OjOptions::DEFAULTS
assert_equal(defaults, Fluent::OjOptions.load_env)
assert_equal(defaults, Oj.default_options.slice(*defaults.keys)) if @@oj_is_avaibale
end

test "valid env var passed with valid value, default is overridden" do
ENV["FLUENT_OJ_OPTION_BIGDECIMAL_LOAD"] = ":bigdecimal"
assert_equal :bigdecimal, Fluent::OjOptions.get_options[:bigdecimal_load]
assert_equal(:bigdecimal, Fluent::OjOptions.load_env[:bigdecimal_load])
assert_equal(:bigdecimal, Oj.default_options[:bigdecimal_load]) if @@oj_is_avaibale
end

test "valid env var passed with invalid value, default is not overriden" do
ENV["FLUENT_OJ_OPTION_BIGDECIMAL_LOAD"] = ":conor"
assert_equal :float, Fluent::OjOptions.get_options[:bigdecimal_load]
assert_equal(:float, Fluent::OjOptions.load_env[:bigdecimal_load])
assert_equal(:float, Oj.default_options[:bigdecimal_load]) if @@oj_is_avaibale
end

test "invalid env var passed, nothing done with it" do
ENV["FLUENT_OJ_OPTION_CONOR"] = ":conor"
assert_equal nil, Fluent::OjOptions.get_options[:conor]
assert_equal(nil, Fluent::OjOptions.load_env[:conor])
assert_equal(nil, Oj.default_options[:conor]) if @@oj_is_avaibale
end
end
end

0 comments on commit 3217fe4

Please sign in to comment.