Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Fix bug that can cause the timestamp to be 1 second later. #161

Merged
merged 4 commits into from
Apr 5, 2018

Conversation

bogdandrutu
Copy link

This can happen because time was read twice when $when was null, once to get the usec and once to get the seconds, so if the initial time was 1.999 and second time was 2.001 then the result will be 2.999 which is 1 second later.

Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Good catch, thanks!

} elseif (!$when instanceof \DateTimeInterface) {
throw new \InvalidArgumentException('Invalid date format. Must be a \DateTimeInterface or numeric.');
}
$when->setTimezone(new \DateTimeZone('UTC'));
return $when;
}

/**
* Converst a float timestamp into a \DateTimeInterface object.
Copy link
Member

Choose a reason for hiding this comment

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

typo: Converst -> Convert

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@chingor13
Copy link
Member

Oops, we aren't providing env var credentials to forked builds for CI - investigating.

Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Code style checker found issues (via CI)

To run: vendor/bin/phpcs --standard=phpcs-ruleset.xml

I'll add something to the README/CONTRIBUTING guide on running the tests.

} elseif (!$when instanceof \DateTimeInterface) {
throw new \InvalidArgumentException('Invalid date format. Must be a \DateTimeInterface or numeric.');
}
$when->setTimezone(new \DateTimeZone('UTC'));
return $when;
}

/**
* Converts a float timestamp into a \DateTimeInterface object.
*
Copy link
Member

Choose a reason for hiding this comment

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

PHP code style enforcer found: extra whitespace at end of this line.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

* @param float $when The Unix timestamp to be converted.
* @return \DateTimeInterface
*/
private function formatFloatTimeToDate($when) {
Copy link
Member

Choose a reason for hiding this comment

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

PHP code style enforcer found: opening brace should go on new line.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@bogdandrutu
Copy link
Author

Thanks @chingor13, unfortunately I couldn't find instructions to do this format checks and run tests, etc. and I am super new in php so I have no idea what to do.

Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Whoops, code review fail by me.

list($usec, $sec) = explode(' ', microtime());
$micro = sprintf("%06d", $usec * 1000000);
$when = new \DateTime(date('Y-m-d H:i:s.' . $micro));
$when = formatFloatTimeToDate(microtime(true));
Copy link
Member

Choose a reason for hiding this comment

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

$when = $this->formatFloatTimeToDate(microtime(true));

// Expect that this is a timestamp
$micro = sprintf("%06d", ($when - floor($when)) * 1000000);
$when = new \DateTime(date('Y-m-d H:i:s.'. $micro, (int) $when));
$when = formatFloatTimeToDate($when);
Copy link
Member

Choose a reason for hiding this comment

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

$when = $this->formatFloatTimeToDate($when);

Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks!

@chingor13 chingor13 merged commit f1bc0dd into census-instrumentation:master Apr 5, 2018
@bogdandrutu bogdandrutu deleted the fixbug branch April 5, 2018 20:05
@chingor13 chingor13 mentioned this pull request Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants