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

Add microseconds to error log #9844

Closed
wants to merge 5 commits into from
Closed

Conversation

mikhainin
Copy link
Contributor

It's just a first attempt to complete #9745

Copy link
Contributor

@TysonAndre TysonAndre left a comment

Choose a reason for hiding this comment

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

This appears to implement the functionality being discussed on the mailing list correctly, e.g. u is always 6 digits long, and the args change and the parts of the code being changed seems right.

This PR is properly using u - https://www.php.net/manual/en/datetime.format.php#refsect1-datetime.format-parameters

			case 'u': length = slprintf(buffer, sizeof(buffer), "%06d", (int) floor(t->us)); break;

ext/date/php_date.c Outdated Show resolved Hide resolved
Co-authored-by: Tyson Andre <tyson.andre@uwaterloo.ca>
@mikhainin
Copy link
Contributor Author

Not sure if I'm happy with such a solution (option error_log_usec doesn't look quite elegant, maybe rename it?)
But now we have backward compatibility.

$ ./sapi/cli/php -derror_log=/dev/stdout -r 'error_log("test");'
[11-Nov-2022 13:45:33 UTC] test

$ ./sapi/cli/php -derror_log=/dev/stdout -derror_log_usec=on -r 'error_log("test");'
[11-Nov-2022 13:45:45.926149 UTC] test

Shall I mention this change in the thread? It looked like people were a bit annoyed by me last time...

@mikhainin mikhainin closed this Nov 11, 2022
@mikhainin mikhainin reopened this Nov 11, 2022
Just to keep compatibility with the existing log parsers
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Just some minor things to change.

About the name it's probably best to mentioned in the internals thread. Personally I don't mind this name but maybe someone else might suggest something better.

main/main.c Outdated
Comment on lines 844 to 850
#if HAVE_GETTIMEOFDAY
struct timeval tp = {0}; /* For setting microseconds */

gettimeofday(&tp, NULL);
error_time = tp.tv_sec;
error_time_usec = tp.tv_usec;
#else
Copy link
Member

Choose a reason for hiding this comment

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

It should be in the usec condition where this actually matters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks. If you take a look once again, please?

main/main.c Outdated Show resolved Hide resolved
main/main.c Outdated Show resolved Hide resolved
tests/basic/errorlog_usec.phpt Outdated Show resolved Hide resolved
ext/date/php_date.c Show resolved Hide resolved
main/main.c Show resolved Hide resolved
@derickr
Copy link
Contributor

derickr commented Nov 28, 2022

As I have just explained on the mailinglist, I don't think we should merge this at all. Your operating system already provides all the functionality to show these microseconds: https://news-web.php.net/php.internals/119046

@bukka
Copy link
Member

bukka commented Dec 30, 2022

@negram I think that the discussion on internals didn't reach an agreement so this might need an RFC. Are you prepared to do that? If not, we should close this PR.

@github-actions
Copy link

No feedback was provided. The issue is being suspended because we assume that you are no longer experiencing the problem. If this is not the case and you are able to provide the information that was requested earlier, please do so. Thank you.

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.

None yet

5 participants