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

fix: timestampNanos nanos precision. #2003

Conversation

rafal-dudek
Copy link

Fix #1996
I hope that testTimestampNanos() is enough. I am testing 2 logs to lower false negative possibility (when all micros and nanos are 0) to 1:1e+12

@rafal-dudek rafal-dudek requested a review from a team as a code owner July 6, 2023 07:54
@google-cla
Copy link

google-cla bot commented Jul 6, 2023

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@rafal-dudek
Copy link
Author

Working on CLA

.map(data -> (Double) data.get(StackdriverTraceConstants.TIMESTAMP_NANOS_ATTRIBUTE))
.collect(Collectors.toList());

assertThat(logTimestampNanos).anySatisfy(nanos -> assertThat(nanos % 1000000).isGreaterThan(0));
Copy link
Member

Choose a reason for hiding this comment

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

Can't nanos be 0 though?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I wanted to create test to ensure that there is nano precision and not just mili.
I am testing 2 logs with any greater than 0 to lower false negative possibility (when all micros and nanos are 0) to 1:1e+12.

Not sure if you like it this way or maybe just simple 1 log with nanos greaterOrEqual(0).

Copy link
Member

Choose a reason for hiding this comment

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

I think the way you have it is fine. As you point out, the likelihood of flakiness due to this is tiny. You might just want to make sure you are getting 2 timestamps in the array. You can even make the number of logs a variable.

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

75.0% 75.0% Coverage
0.0% 0.0% Duplication

@rafal-dudek
Copy link
Author

Sorry, my company can't sign the CLA. Closing.

@rafal-dudek rafal-dudek closed this Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Precision of timestampNanos in logs is miliseconds instead of nanoseconds
2 participants