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

isotime formatter: assume incoming epoch timestamp being in UTC #4668

Merged
merged 4 commits into from
May 9, 2019
Merged

isotime formatter: assume incoming epoch timestamp being in UTC #4668

merged 4 commits into from
May 9, 2019

Conversation

emptywee
Copy link
Contributor

@emptywee emptywee commented May 8, 2019

When the platform where st2 is running is configured with non UTC timezone executions retrieved from st2api have log entries with incorrectly formatted timestamp as shown on this screenshot:

image

After debugging it, turns out that api model is receiving them as unix timestamps, parsing them as local time producing naive datetime object and further assuming it's in UTC timezone, resulting in wrong timestamps provided via API calls.

This PR fixes that, but I am not sure if anything else may get affected. Maybe a proper way would be giving non-formatted timestamp and let API consumers deal with time zones?

@Kami please, take a look and chime in. Others welcome too.

@CLAassistant
Copy link

CLAassistant commented May 8, 2019

CLA assistant check
All committers have signed the CLA.

@Kami Kami self-assigned this May 9, 2019
@Kami Kami added this to the 3.0.1 milestone May 9, 2019
@Kami
Copy link
Member

Kami commented May 9, 2019

Thanks.

The change look good to me, but in such scenarios I would actually start with some unit tests (I will look into adding some) :)

@Kami
Copy link
Member

Kami commented May 9, 2019

I was able to confirm the issue.

It's indeed related to how we handle time objects. For precision reasons we store timestamps as number of microseconds since the UNIX epoch and the format function didn't handle that correctly if a server time was not set to UTC (this is usually not a good practice, but anyway) - it assumed it was in local time while it was actually in UTC.

I will also push tests to this PR.

Thanks again and a good catch 👍

@Kami Kami merged commit 6a5a5e9 into StackStorm:master May 9, 2019
@Kami
Copy link
Member

Kami commented May 9, 2019

Merged, thanks!

@emptywee
Copy link
Contributor Author

emptywee commented May 9, 2019

Good deal! Thanks for looking into this with me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants