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

Use float instead of Date for protocol types for higher precision. #1737

Merged
merged 9 commits into from
Nov 11, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak commented Sep 23, 2021

📜 Description

Use float instead of Date for protocol types for higher precision.

To retrieve more precise timestamps, we are using System#nanoTime as it's the only method available both on Android and JVM.

When Span is created with given start timestamp (for example with Android app start time), we are not able to measure high precision timestamp, we fall back then to regular timestamp with milliseconds precision.

How it looks in UI:
image

💡 Motivation and Context

Fixes #1689

💚 How did you test it?

Unit & integration tests.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2021

Codecov Report

Merging #1737 (035274a) into 6.x.x (23913ee) will increase coverage by 0.05%.
The diff coverage is 89.39%.

Impacted file tree graph

@@             Coverage Diff              @@
##              6.x.x    #1737      +/-   ##
============================================
+ Coverage     80.93%   80.98%   +0.05%     
- Complexity     2822     2835      +13     
============================================
  Files           205      205              
  Lines         10360    10405      +45     
  Branches       1370     1376       +6     
============================================
+ Hits           8385     8427      +42     
  Misses         1493     1493              
- Partials        482      485       +3     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/SentryTracer.java 88.88% <66.66%> (+0.07%) ⬆️
...ain/java/io/sentry/protocol/SentryTransaction.java 89.60% <80.00%> (-0.50%) ⬇️
...y/src/main/java/io/sentry/protocol/SentrySpan.java 83.70% <88.23%> (-0.43%) ⬇️
sentry/src/main/java/io/sentry/DateUtils.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/Span.java 96.42% <100.00%> (+2.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23913ee...035274a. Read the comment docs.

@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review September 23, 2021 15:42
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Looks so much better in the UI 💯

sentry/src/main/java/io/sentry/Span.java Show resolved Hide resolved
@marandaneto
Copy link
Contributor

marandaneto commented Sep 27, 2021

btw: System#nanoTime

This clock is guaranteed to be monotonic, and is suitable for interval timing when the interval does not span device sleep

that means when the device is in sleep mode, the system does not increase the timer, is it possible that it'd cause off-by-1 sort of problems?

https://developer.android.com/reference/android/os/SystemClock search for nanoTime

@maciejwalkowiak
Copy link
Contributor Author

that means when the device is in sleep mode, the system does not increase the timer, is it possible that it'd cause off-by-1 sort of problems?

Are our transactions meant to span over the system sleep? If so, we should at least check if the endTime - startTime isn't a negative number.

@marandaneto
Copy link
Contributor

that means when the device is in sleep mode, the system does not increase the timer, is it possible that it'd cause off-by-1 sort of problems?

Are our transactions meant to span over the system sleep? If so, we should at least check if the endTime - startTime isn't a negative number.

good question and I don't know the answer right now, have to check if it makes sense or not

@marandaneto
Copy link
Contributor

should we target the v6 branch then?

@maciejwalkowiak
Copy link
Contributor Author

Yes but this PR is anyway on hold until we have JSON serialization merged.

@bruno-garcia
Copy link
Member

JSON PR is getting to its final form so might be time to point this to that branch and merge?

@maciejwalkowiak
Copy link
Contributor Author

Just to avoid double work - once JSON PR is merged to 6.x.x, this branch will be rebased and pointed against 6.x.x.

@bruno-garcia
Copy link
Member

Gson PR merged into branch 6.x.x. Lets get this one merged in there too? We'll release an alpha soon.

@maciejwalkowiak maciejwalkowiak changed the base branch from main to 6.x.x November 2, 2021 23:36
@maciejwalkowiak
Copy link
Contributor Author

6.x.x is now up to date with main, this PR is now pointing to 6.x.x. Ready for the final review.

@marandaneto
Copy link
Contributor

@romtsn would you like to have a look?

@romtsn
Copy link
Member

romtsn commented Nov 4, 2021

@marandaneto checking 👍

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM

@maciejwalkowiak maciejwalkowiak merged commit 3b190d0 into 6.x.x Nov 11, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1689 branch November 11, 2021 09:18
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.

Use float instead of Date for protocol types for higher precision
5 participants