chore(android-sqlite): Repair start times of spans generated by SentrySQLiteDriver#5543
Conversation
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| * "Coarse" in that it doesn't provide a nanosecond precision for [SentryNanotimeDate] | ||
| * [startTimestamp]s. | ||
| */ | ||
| fun recordCoarseSpan( |
There was a problem hiding this comment.
Chime in if you can think of a better name. Not in love with this, but it'll do if it has to...
| * END TRANSACTION ├███┤ 0.18 ms | ||
| * ``` | ||
| */ | ||
| private fun SentryDate.repairPrecision(baseline: SentryDate?): SentryDate = |
There was a problem hiding this comment.
This arguably belongs on SentryDate, especially as the latter's API is already enmeshed in the broken precision of SentryDate.nanoTimestamp. But I struggled to get it to work. I considered:
[1] adding a SentryDate.laterDateByDiff() that returns a SentryLongDate (declared as a SentryDate).
...but that seemed heavyweight for a single call site (new public method, tests against all implementing classes), esp as the only thing it buys us is a replacement for the already minimal line 112. Plus, bumping the minSdkVersion to 26 will let us move off of SentryNanotimeDate, so any investments in it will be short-lived.
[2] adding a repairPrecision() method solely to SentryNanotimeDate (allowing us to relocate all of the repair method).
...but that'd also be a new public API, and it'd mean we'd have to cast at the call site, which would be against the grain of how we typically use SentryDate.
So I left things as you see them, figuring we could shuffle bits around if we ever get a second call site.
Let me know if you have any opinions.
0xadam-brown
force-pushed
the
chore/offset-sqlite-start-timestamps
branch
from
70c2b17 to
1e20263
Compare
…ySQLiteDriver (JAVA-275) Repairs the nanoTimetamp of the SentryNanotimeDates used as start times for the spans generated by SentrySQLiteDriver. Without those repairs, all spans within a given wall clock millisecond are displayed by Sentry UI as starting at that same millisecond and are re-ordered arbitrarily. Often that's quite confusing as actual BEGIN -> EXECUTE STATEMENT -> END sequences can appear as EXECUTE STATEMENT -> END -> BEGIN (etc.). For more details, see the discussion [here](#5504 (comment)).
0xadam-brown
force-pushed
the
chore/offset-sqlite-start-timestamps
branch
from
1e20263 to
d29d3f8
Compare
| ) { | ||
| recordSpan(sql, startTimestamp, endTimestamp = null, status, throwable) | ||
| val parent = scopes.span ?: return | ||
| val nanoPrecisionStart = startTimestamp.repairPrecision(baseline = parent.startDate) |
There was a problem hiding this comment.
Repairing the incoming timestamp is the only interesting change this PR makes. (The diff looks a bit bigger because I switched the order of the first and second methods and renamed the second recordCoarseSpan.)
0xadam-brown
requested review from
adinauer,
markushi,
romtsn and
runningcode
as code owners
1 participant
📜 Description
Repairs the
nanoTimetampof theSentryNanotimeDates used as start times for the spans generated bySentrySQLiteDriver.For more details, see the discussion here. This PR implements approach [1] ("Offset SQLite spans based nano duration between parent and child start times").
💡 Motivation and Context
Without those repairs, all spans within a given wall clock millisecond are displayed by Sentry UI as starting at that same millisecond and are re-ordered arbitrarily. Often that's quite confusing as actual BEGIN -> EXECUTE STATEMENT -> END sequences can appear as EXECUTE STATEMENT -> END -> BEGIN (etc.).
JAVA-275
Screenshots
Before
(Link)
After
(Link)
Before
(Link)
After
(Link)
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Consider implementing nesting of spans as mentioned here.