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.

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

Support %iso8601 special case for time_format. fix #1528 #1562

Merged
merged 1 commit into from
May 12, 2017

Conversation

repeatedly
Copy link
Member

Ruby's iso8601 parser is a restrict version and it doesn't support 20170101T120000+0900 like format.
But this parser can handle 2017-01-01T12:00:00+09:00 and 2017-01-01T12:00:00.123+09:00 cases, so supporting it makes configuration simpler for iso8601 logs.

@repeatedly repeatedly requested a review from mururu May 9, 2017 22:19
@repeatedly repeatedly self-assigned this May 9, 2017
@repeatedly repeatedly added enhancement Feature request or improve operations v0.14 labels May 9, 2017
@repeatedly
Copy link
Member Author

@mururu Could you check this patch?

Copy link
Member

@mururu mururu left a comment

Choose a reason for hiding this comment

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

(EDITED) This case doesn't consider timezone/offset. In other words, this format interprets "2017-01-01T12:00:00" as UTC. Is it correct? (I don't know the detail of ISO8601.)

@repeatedly
Copy link
Member Author

Yes. I assume timestamp has a offset because iso8601 defines offset in the spec.

@repeatedly
Copy link
Member Author

If we need to handle timezone seprately, we can add timezone parameter handling later.

@mururu
Copy link
Member

mururu commented May 12, 2017

Sorry, I misunderstood Time.iso8601. When the function gets a timestamp without an offset or "Z", it considers the timezone as (fluentd's) localtime. If we want to handle the timestamp as UTC, we have to add Time.now.localtime.utc_offset to the result of Time.iso8601 like the strptime case.

Yes. I assume timestamp has a offset because iso8601 defines offset in the spec.

You mean the spec says the timestamp must have the offset or "Z"? It sounds good.
I think it is better to write the fact the iso8601 parser accept the timestamp without offset and the timezone is recognized as the same with server's localtime in the document.�

@mururu
Copy link
Member

mururu commented May 12, 2017

anyway, LGTM.

@repeatedly
Copy link
Member Author

Yeah, we should mention the timezone in the document.

@repeatedly repeatedly merged commit a726cd0 into master May 12, 2017
repeatedly added a commit that referenced this pull request May 22, 2017
@repeatedly repeatedly deleted the iso8601-time-format branch May 24, 2017 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or improve operations v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants