Fix bug that can cause the timestamp to be 1 second later. by bogdandrutu · Pull Request #161 · census-instrumentation/opencensus-php · GitHub
Skip to content
This repository was 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
chingor13 merged 4 commits into
census-instrumentation:masterfrom
bogdandrutu:fixbug
Apr 5, 2018
Merged

Fix bug that can cause the timestamp to be 1 second later.#161
chingor13 merged 4 commits into
census-instrumentation:masterfrom
bogdandrutu:fixbug

Conversation

@bogdandrutu

Copy link
Copy Markdown

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.

@chingor13 chingor13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch, thanks!

Comment thread src/Trace/Span.php Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@chingor13 chingor13 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/Trace/Span.php Outdated

/**
* Converts a float timestamp into a \DateTimeInterface object.
*

Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Comment thread src/Trace/Span.php Outdated
* @param float $when The Unix timestamp to be converted.
* @return \DateTimeInterface
*/
private function formatFloatTimeToDate($when) {

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@chingor13 chingor13 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/Trace/Span.php Outdated
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
Copy Markdown
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));

Comment thread src/Trace/Span.php Outdated

Copy link
Copy Markdown
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);

@chingor13 chingor13 left a comment

Copy link
Copy Markdown
Member

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