Improve possible date precision to 10 μs

[adinauer]

📜 Description

This especially affects precision when passing in custom start/end dates for spans/transactions.

It also improves timestamp precision where possible instead of relying on a ms precision start timestamp and a fake end timestamp that simply has elapsed ns added to the start timestamp.

We could further improve precision by not using Double in SentrySpan and SentryTransaction. However without changes in relay this would probably only get us to μs precision.

💡 Motivation and Context

Improves #2394 but there's even more room to improve.

💚 How did you test it?

Unit tests, manually

📝 Checklist

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

🔮 Next steps

[adinauer]

This is because a Double cannot offer high enough precision for timestamps.

[adinauer]

@marandaneto you mentioned that doing this might cause problems. I can no longer find the comment but afair it went something like "older versions of android / AGP might run into issues". Do you mind giving this a review 🙏 ?

[marandaneto]

@marandaneto you mentioned that doing this might cause problems. I can no longer find the comment but afair it went something like "older versions of android / AGP might run into issues". Do you mind giving this a review 🙏 ?

@romtsn checked that IIRC, so maybe he can help.

[marandaneto]

H: This likely requires a major bump since it changes the public APIs with a new type and people can rely on either for comparison or even in compilation due to type errors.

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against e9f8cb1

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	312.94 ms	359.82 ms	46.88 ms
Size	1.73 MiB	2.33 MiB	621.15 KiB

Previous results on branch: feat/higher-date-precision

Startup times

Revision	Plain	With Sentry	Diff
904bfec	301.42 ms	339.55 ms	38.13 ms
aab1277	336.48 ms	386.74 ms	50.26 ms
55654bd	331.98 ms	370.85 ms	38.87 ms
da89816	344.00 ms	404.19 ms	60.19 ms
a8de2dd	333.56 ms	398.04 ms	64.48 ms
b818243	264.58 ms	350.81 ms	86.23 ms

App size

Revision	Plain	With Sentry	Diff
904bfec	1.73 MiB	2.33 MiB	619.16 KiB
aab1277	1.73 MiB	2.33 MiB	618.18 KiB
55654bd	1.73 MiB	2.33 MiB	621.17 KiB
da89816	1.73 MiB	2.33 MiB	619.14 KiB
a8de2dd	1.73 MiB	2.33 MiB	621.15 KiB
b818243	1.73 MiB	2.33 MiB	619.15 KiB

[codecov-commenter]

Codecov Report

Base: 80.16% // Head: 80.06% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (cd9b9af) compared to base (b0f62c0).
Patch coverage: 76.57% of modified lines in pull request are covered.

❗ Current head cd9b9af differs from pull request most recent head e9f8cb1. Consider uploading reports for the commit e9f8cb1 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2451      +/-   ##
============================================
- Coverage     80.16%   80.06%   -0.10%     
- Complexity     3882     3902      +20     
============================================
  Files           313      320       +7     
  Lines         14695    14751      +56     
  Branches       1947     1950       +3     
============================================
+ Hits          11780    11811      +31     
- Misses         2154     2169      +15     
- Partials        761      771      +10     

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/Hub.java	76.82% <ø> (-1.22%)	⬇️
sentry/src/main/java/io/sentry/NoOpSpan.java	21.42% <0.00%> (ø)
...entry/src/main/java/io/sentry/NoOpTransaction.java	21.95% <0.00%> (ø)
sentry/src/main/java/io/sentry/SentryOptions.java	79.37% <50.00%> (-0.27%)	⬇️
sentry/src/main/java/io/sentry/util/Platform.java	50.00% <50.00%> (ø)
sentry/src/main/java/io/sentry/DateUtils.java	84.61% <55.55%> (-15.39%)	⬇️
...rc/main/java/io/sentry/SentryAutoDateProvider.java	57.14% <57.14%> (ø)
...ry/src/main/java/io/sentry/SentryNanotimeDate.java	74.07% <74.07%> (ø)
sentry/src/main/java/io/sentry/Span.java	91.66% <80.00%> (-0.58%)	⬇️
sentry/src/main/java/io/sentry/SentryDate.java	83.33% <83.33%> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[adinauer]

H: This likely requires a major bump since it changes the public APIs with a new type and people can rely on either for comparison or even in compilation due to type errors.

@marandaneto I think it should be fine as I only modified / removed things marked internal. Did you see any specific thing that would require the major bump?

[markushi]

Great effort!

I'm wondering if there would be another way of designing the DateProvider.
Instead of overriding the Provider on init we could have a singleton and register more precise implementations on init.
e.g.
SentryDateProvider.now() always provides a SentryDate, probably just ms precision at app start.
Then during init we could register a more precise one:
SentryDateProvider.registerProvider(new SentryInstantDateProvider())
If SentryDateProvider.now() gets called after init, it would provide ns precision.

Although this deviates from our usual options.getXYZ() approach we could get rid of AndroidDateUtils this way (with the downside of only having a lower resolution during app start).

[markushi]

I guess it would be fine to set the lastPausedTime when register() is called using options.getDateProvider().now() - this way it's more consistent

[adinauer]

Not sure about consequences, maybe @romtsn or @marandaneto can confirm?

[romtsn]

@stefanosiano was one implementing it, he might have opinions. But I don't see any problems with moving this into register since it's called shortly after this integration is instantiated anyway.

[adinauer]

Instead of overriding the Provider on init we could have a singleton and register more precise implementations on init.

Sounds good to me. If others agree I'll change it to not use options. Makes even more sense now that it's an internal option anyways.

[adinauer]

we could get rid of AndroidDateUtils this way (with the downside of only having a lower resolution during app start).

If we query the date provider on app start it'll do so before init was executed. So we'd probably still need to use some Android specific util, that doesn't try to use a different SentryDate. Problem otherwise would be to fall back to ms precision whenever we compare e.g. a SentryInstantDate and SentryNanotimeDate. Could also modify SentryInstantDate to also hold a snapshot of System.nanoTime(). I found the Android Util approach to be cleaner.

However this might also be fine as until now we also only offer ms precision when a start date is passed in. With current changes in this PR, precision is improved.

[marandaneto]

H: This likely requires a major bump since it changes the public APIs with a new type and people can rely on either for comparison or even in compilation due to type errors.

@marandaneto I think it should be fine as I only modified / removed things marked internal. Did you see any specific thing that would require the major bump?

@adinauer I guess some classes or methods were without the internal annotation yet or I missed them in the code diff, just skimmed thru, and apparently it's alright, didn't see Public APIs changing, so all good, sorry for the confusion.

[romtsn]

@marandaneto you mentioned that doing this might cause problems. I can no longer find the comment but afair it went something like "older versions of android / AGP might run into issues". Do you mind giving this a review 🙏 ?

So, using Instant in API 26 or above is fine, because it comes as part of the Android SDK already https://developer.android.com/reference/java/time/Instant

In addition, if we wanna support older API versions, we'd need our users to use AGP 4.0 + enabled coreLibraryDesugaring. I think for these cases we could try to access Instant at runtime with a try-catch block and if it doesn't fail on older versions, this means that coreLibraryDesugaring is enabled and we're safe to use the java.lang.* supported APIs.

[romtsn]

Sounds good to me. If others agree I'll change it to not use options. Makes even more sense now that it's an internal option anyways.

If we want users to override the date provider, we should still keep it in the options. But I don't see why they would do it, so I don't have a strong opinion here.

[romtsn]

A few points, but otherwise LGTM 🚀 Agree with Max - great effort, thanks for doing this!

[adinauer]

we could try to access Instant at runtime with a try-catch block and if it doesn't fail on older versions, this means that coreLibraryDesugaring is enabled and we're safe to use the java.lang.* supported APIs.

@romtsn is there varying time source precision? If we use Instant with ms precision, it's worse than SentryNanotimeDate unless I improve some code. This might also happen on non Android. We can always improve in the future, so I'd vote for keeping the code simple for now. I'll do some experiment to see if I can come up with a cleaner approach, if that fails, I'd keep it as is.

If we want users to override the date provider, we should still keep it in the options. But I don't see why they would do it, so I don't have a strong opinion here.

@romtsn @markushi Having the static then makes it easier to use the wrong implementation. For static invocations like appStartTime you have to use AndroidDateUtils over the other static (that would replace the options) as it wouldn't be configured correctly at the time of calling. I'd prefer keeping it in options to avoid this confusion.

[romtsn]

@romtsn is there varying time source precision? If we use Instant with ms precision, it's worse than SentryNanotimeDate unless I improve some code. This might also happen on non Android. We can always improve in the future, so I'd vote for keeping the code simple for now. I'll do some experiment to see if I can come up with a cleaner approach, if that fails, I'd keep it as is.

I believe the desugared version of Instant is still of the ns precision. I mean it's just a simple try-catch in the ctor, doesn't look complex, but up to you :)

I'd prefer keeping it in options to avoid this confusion.

fine for me 👍

[adinauer]

simple try-catch in the ctor

If we expect it to be ns precision then it's easy and I can update the code. The thing I meant was that code would get harder to maintain if every SentryDate had a fallback to System.nanoTime(). I'll add the try/catch for Android but not for Java as it's supposed to only have ms precision despite Instant being available on older versions.

[adinauer]

Just did some testing, try/catch seems to work on older API Levels with desugaring enabled and uses Instant. While testing I noticed, that precision of Instant, on the Android devices I tested on, was only ms so worse than using SentryNanotimeDate. I suggest always using SentryNanotimeDate for Android for now and investigate at a later point whether we can tap a more accurate clock source somehow. @romtsn @markushi how does that sound for you?

An alternative would be to use SentryInstantDate but change it to also have a snapshot of System.nanoTime(). This wouldn't really offer any benefit imo and only increases code complexity.

Haven't found a way to check whether ns precision is actually available. Creating a few dates and checking if any of them differs from ms precision by more than one microsecond would be simple a way to do it. This is to ignore rounding to 999 microseconds when it's actually 0.

[adinauer]

After some more testing I was able to narrow the problem down. When enabling desugaring, I never got high precision timestamps. Without desugaring on the build, API 33 gave high precision timestamps, while 32 and below only gave ms precision. Most of the testing was done on emulators but I also tested on real devices (API 29 and 31) that only had ms precision.

One thing that surprised me was the Android Studio eval dialog Instant.now() offered higher precision on API 33 with desugaring enabled but the code only got ms precision.

[adinauer]

This code could serve to check for high precision availability in the future but for now we decided to keep it simple and stick with SentryNanotimeDate for Android and checking JDK Version for desktop/server.

It's possible desktop/server JDK only offers ms precision on version 9+ or high precision on 8 or lower, depending on the JDK vendor.

package io.sentry.android.core;

import static java.time.temporal.ChronoField.NANO_OF_SECOND;

import android.annotation.SuppressLint;
import io.sentry.SentryDate;
import io.sentry.SentryDateProvider;
import io.sentry.SentryInstantDateProvider;
import io.sentry.SentryNanotimeDateProvider;
import java.time.Instant;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;

/**
 * This {@link SentryDateProvider} will use {@link java.time.Instant} for Android API 26+ or a
 * combination of {@link java.util.Date} and System.nanoTime() on lower versions.
 */
@ApiStatus.Internal
public final class SentryAndroidDateProvider implements SentryDateProvider {

  private @NotNull SentryDateProvider dateProvider;

  public SentryAndroidDateProvider(final @NotNull BuildInfoProvider buildInfoProvider) {
    try {
      if (isHighPrecisionAvailableForInstant()) {
        dateProvider = new SentryInstantDateProvider();
      } else {
        dateProvider = new SentryNanotimeDateProvider();
      }
    } catch (Throwable t) {
      dateProvider = new SentryNanotimeDateProvider();
    }
  }

  @SuppressLint("NewApi")
  private static boolean isHighPrecisionAvailableForInstant() {
    for (int i = 1; i <= 5; i++) {
      if (isHighPrecision(Instant.now())) {
        return true;
      }
    }

    return false;
  }

  @SuppressLint("NewApi")
  private static boolean isHighPrecision(@NotNull final Instant instant) {
    final long nanos = instant.getNano();
    final long nanosUpToMicros = nanos % 1000000;
    final long diff = Math.abs(500000 - nanosUpToMicros);
    return diff < 499000;
  }

  @Override
  public SentryDate now() {
    return dateProvider.now();
  }
}

[markushi]

@adinauer nice investigation efforts!
AS eval likely returns a higher precision as the code being executed does not get desugered (afaik desugering only happens once at build time).
I'm voting for only using SentryNanotimeDateProvider, as it should be enough precision on mobile (e.g. rendering times, network requests and db queries are all in the range of ms)