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 for Logstash v1 event schema (and improved timestamp handling) #9539

Merged
merged 5 commits into from Jan 3, 2014

Conversation

jcollie
Copy link
Contributor

@jcollie jcollie commented Jan 2, 2014

With version 1.2.0 Logstash introduced a new event schema. Salt is sending the old-style events with the logstash log handler module. While it is possible to deal with old-style events on newer Logstash versions, its better long-term to start sending new-style events. These patches add that support (defaulting to the old-style events). Also, improvements are made to timestamp handling - there's no need for doing anything fancy with pytz and timezones, logstash is perfectly happy with UTC timestamps. Also, these patches reduce the precision of timestamps to milliseconds - the library that logstash uses to deal with time (JodaTime) can't handle anything more, and certain versions will choke if given anything more.

due to limitations in the JodaTime library that logstash uses it is
limited to millisecond accuracy.  Some versions (1.3.2 in particular)
will choke if given anything other than a millisecond timestamp.
if HAS_PYTZ:
return _UTC.localize(timestamp).isoformat()
return '{0}+00:00'.format(timestamp.isoformat())
return datetime.datetime.utcfromtimestamp(record.created).isoformat()[:-3] + 'Z'
Copy link
Member

Choose a reason for hiding this comment

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

Are you really sure? See #9140

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since the timestamp is already in UTC, all that pytz is doing for you is a fancy way to add "+00:00" to the end of the string:

>>> t = time.time()
>>> d=datetime.datetime.utcfromtimestamp(t)
>>> d
datetime.datetime(2014, 1, 3, 2, 25, 16, 197281)
>>> l = pytz.utc.localize(d)
>>> l
datetime.datetime(2014, 1, 3, 2, 25, 16, 197281, tzinfo=<UTC>)
>>> l.isoformat()
'2014-01-03T02:25:16.197281+00:00'
>>> d.isoformat()
'2014-01-03T02:25:16.197281'
>>> datetime.datetime.utcfromtimestamp(t).isoformat()[:-3]+'Z'
'2014-01-03T02:25:16.197Z'

So, I think that it's just simpler to skip pytz. Also, as mentioned in the comments, 1.3.2 has a bug where it won't accept timestamps with more than millisecond precision.

Copy link
Member

Choose a reason for hiding this comment

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

Would you be comfortable with something like the existing fallback which appends +00:00?

It seemed that it would need less configuration on the logstash side...

return '{0}+00:00'-format(datetime.datetime.utcfromtimestamp(record.created).isoformat()[:-3])

We'd still drop pytz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Z and +00:00 are equivalent, but other than saving a few bytes on the wire it doesn't matter to me, it won't make a difference on the logstash side.

http://en.wikipedia.org/wiki/ISO_8601#UTC

Copy link
Member

Choose a reason for hiding this comment

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

Darn!

Yes, correct, the Z is the same 😄

@thatch45
Copy link
Member

thatch45 commented Jan 3, 2014

@s0undt3ch, since this is your domain I will let you decide on the merge

@s0undt3ch
Copy link
Member

Ok, I'm conviced 😄

s0undt3ch added a commit that referenced this pull request Jan 3, 2014
Support for Logstash v1 event schema (and improved timestamp handling)
@s0undt3ch s0undt3ch merged commit cd8ccb9 into saltstack:develop Jan 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants