Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider timezone when calculate timekey #2054

Merged
merged 8 commits into from
Jul 23, 2018
Merged

Consider timezone when calculate timekey #2054

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6921b47
Consider timezone when calculate timekey
okkez Jul 5, 2018
a5cf3b0
Use with_timezone to set proper timezone during test
okkez Jul 5, 2018
34fb65a
Set proper value "Asia/Tokyo" to TZ
okkez Jul 5, 2018
92225ca
Set TZ properly on Windows
okkez Jul 6, 2018
1e9d52d
Use instance variables instead of `@buffer_config` instance methods
okkez Jul 12, 2018
72339a2
Use Fluent::Timezone.utc_offset to consider `@buffer_config.timekey_z…
okkez Jul 12, 2018
f178b87
Add test case when machine timezone and timekey_zone is different
okkez Jul 17, 2018
54bda48
Add cache for calculating timezone offset
okkez Jul 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions lib/fluent/plugin/output.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,20 +803,17 @@ def metadata(tag, time, record)
if !@chunk_key_time && !@chunk_key_tag
@buffer.metadata()
elsif @chunk_key_time && @chunk_key_tag
time_int = time.to_i
timekey = (time_int - (time_int % @buffer_config.timekey)).to_i
timekey = calculate_timekey(time)
@buffer.metadata(timekey: timekey, tag: tag)
elsif @chunk_key_time
time_int = time.to_i
timekey = (time_int - (time_int % @buffer_config.timekey)).to_i
timekey = calculate_timekey(time)
@buffer.metadata(timekey: timekey)
else
@buffer.metadata(tag: tag)
end
else
timekey = if @chunk_key_time
time_int = time.to_i
(time_int - (time_int % @buffer_config.timekey)).to_i
calculate_timekey(time)
else
nil
end
Expand All @@ -825,6 +822,16 @@ def metadata(tag, time, record)
end
end

def calculate_timekey(time)
time_int = time.to_i
if @buffer_config.timekey_use_utc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accessing object member is slower than accessing instance variable.
@timekey_use_utc = @buffer_config.timekey_use_utc in configure is better.

(time_int - (time_int % @buffer_config.timekey)).to_i
else
offset = Time.at(time_int).utc_offset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should consider timekey_zone value instead of utc_offset?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe yes. How about the following function?

diff --git a/lib/fluent/timezone.rb b/lib/fluent/timezone.rb
index bb6fa868..0e79dd03 100644
--- a/lib/fluent/timezone.rb
+++ b/lib/fluent/timezone.rb
@@ -139,5 +139,17 @@ module Fluent
 
       return nil
     end
+
+    def self.utc_offset(time, timezone)
+      return 0 if timezone.nil?
+
+      case timezone
+      when NUMERIC_PATTERN
+        Time.zone_offset(timezone)
+      when NAME_PATTERN
+        tz = TZInfo::Timezone.get(timezone)
+        tz.period_for_utc(time).utc_total_offset
+      end
+    end
   end
 end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

(time_int - ((time_int + offset)% @buffer_config.timekey)).to_i
end
end

def chunk_for_test(tag, time, record)
require 'fluent/plugin/buffer/memory_chunk'

Expand Down
36 changes: 36 additions & 0 deletions test/plugin/test_out_file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,42 @@ def parse_system(text)
check_gzipped_result(path, formatted_lines * 3)
end

test 'append when JST' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add more test with different time and timezone, e.g. timekey_zone is +09:00 and actual time is +02:00

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will add more test.

with_timezone(Fluent.windows? ? "JST-9" : "Asia/Tokyo") do
time = event_time("2011-01-02 03:14:15+09:00")
formatted_lines = %[2011-01-02T03:14:15+09:00\ttest\t{"a":1}\n] + %[2011-01-02T03:14:15+09:00\ttest\t{"a":2}\n]

write_once = ->(){
d = create_driver %[
path #{TMP_DIR}/out_file_test
compress gz
append true
<buffer>
timekey_use_utc false
timekey_zone Asia/Tokyo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use "+0900" like value?
I'm not sure but test on Windows fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just commit the fix :)

</buffer>
]
d.run(default_tag: 'test'){
d.feed(time, {"a"=>1})
d.feed(time, {"a"=>2})
}
d.instance.last_written_path
}

path = write_once.call
assert_equal "#{TMP_DIR}/out_file_test.20110102.log.gz", path
check_gzipped_result(path, formatted_lines)

path = write_once.call
assert_equal "#{TMP_DIR}/out_file_test.20110102.log.gz", path
check_gzipped_result(path, formatted_lines * 2)

path = write_once.call
assert_equal "#{TMP_DIR}/out_file_test.20110102.log.gz", path
check_gzipped_result(path, formatted_lines * 3)
end
end

test '${chunk_id}' do
time = event_time("2011-01-02 13:14:15 UTC")
formatted_lines = %[2011-01-02T13:14:15Z\ttest\t{"a":1}\n] + %[2011-01-02T13:14:15Z\ttest\t{"a":2}\n]
Expand Down