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

Extend the LogSink interface to be able to pass microseconds #441

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

asekretenko
Copy link
Contributor

The goal of this PR is to solve the problem of passing microseconds into custom LogSink implementations that was described in #307.

I am proposing a slightly different approach, however: instead of modifying the existing send() signature I am adding the second one.
This has an advantage of not breaking all the third-party LogSink implementations immediately (including ours) ... and, unfortunately, all the disadvantages associated with this approach.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@asekretenko
Copy link
Contributor Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jpeach
Copy link

jpeach commented Mar 27, 2019

@asekretenko would it make sense for the usecs parameter to be unsigned?

@asekretenko
Copy link
Contributor Author

@asekretenko would it make sense for the usecs parameter to be unsigned?

Probably it makes sense.
I cannot remember any way to obtain a negative value for (x - static_cast<some_interger_type>(x)) * 1e-6 for an IEEE754-compliant double x if x>0. And you?

@asekretenko
Copy link
Contributor Author

@asekretenko would it make sense for the usecs parameter to be unsigned?

On the other hand, on some crazy (or "slightly buggy") platform we might get a negative value here for some corner case. Seeing ".-000001" in the microseconds would help to understand what the hell is going on much better than seeing seemingly normal values in a strange order. Been there, done that (albeit with another logging system).

Probably it would be better to follow advice from Google's style guide

Do not use an unsigned type merely to assert that a variable is non-negative.

and leave microseconds signed.

@asekretenko
Copy link
Contributor Author

@shinh @sergiud May I ask you to have a look at this PR?

asfgit pushed a commit to apache/mesos that referenced this pull request Mar 29, 2019
Extended the LogSink interface to be able to log microseconds.

This makes possible to solve a problem with modules implementing custom LogSink which currently log 000000 instead of microseconds.

This is a backport of this patch: google/glog#441 to glog 0.3.3

Review: https://reviews.apache.org/r/70334/
@sergiud sergiud merged commit 5792d60 into google:master Oct 31, 2019
St0rmingBr4in pushed a commit to St0rmingBr4in/mesos that referenced this pull request Feb 14, 2020
Extended the LogSink interface to be able to log microseconds.

This makes possible to solve a problem with modules implementing custom LogSink which currently log 000000 instead of microseconds.

This is a backport of this patch: google/glog#441 to glog 0.3.3

Review: https://reviews.apache.org/r/70334/
St0rmingBr4in pushed a commit to St0rmingBr4in/mesos that referenced this pull request Feb 14, 2020
Extended the LogSink interface to be able to log microseconds.

This makes possible to solve a problem with modules implementing custom LogSink which currently log 000000 instead of microseconds.

This is a backport of this patch: google/glog#441 to glog 0.3.3

Review: https://reviews.apache.org/r/70334/
@sergiud sergiud added this to the 0.5 milestone Mar 30, 2021
@sergiud sergiud linked an issue Apr 1, 2021 that may be closed by this pull request
@sergiud sergiud mentioned this pull request May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LogSink::ToString has no microsecond precision.
4 participants