-
-
Notifications
You must be signed in to change notification settings - Fork 472
chore(android-sqlite): Repair start times of spans generated by SentrySQLiteDriver #5543
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
base: feat/no-double-wrapping-sqlite-driver
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| package io.sentry.sqlite | ||
|
|
||
| import io.sentry.IScopes | ||
| import io.sentry.ISpan | ||
| import io.sentry.Instrumenter | ||
| import io.sentry.ScopesAdapter | ||
| import io.sentry.SentryDate | ||
| import io.sentry.SentryLongDate | ||
| import io.sentry.SentryNanotimeDate | ||
| import io.sentry.SentryStackTraceFactory | ||
| import io.sentry.SpanDataConvention | ||
| import io.sentry.SpanStatus | ||
|
|
@@ -28,36 +30,44 @@ internal class SQLiteSpanInstrumentation( | |
| */ | ||
| fun startTimestamp(): SentryDate = scopes.options.dateProvider.now() | ||
|
|
||
| /** Records a `db.sql.query` span from [startTimestamp] to the moment of invocation. */ | ||
| /** Records a `db.sql.query` span from [startTimestamp] to [startTimestamp] + [durationNanos]. */ | ||
| fun recordSpan( | ||
| sql: String, | ||
| startTimestamp: SentryDate, | ||
| durationNanos: Long, | ||
| status: SpanStatus, | ||
| throwable: Throwable? = null, | ||
| ) { | ||
| recordSpan(sql, startTimestamp, endTimestamp = null, status, throwable) | ||
| val parent = scopes.span ?: return | ||
| val nanoPrecisionStart = startTimestamp.repairPrecision(baseline = parent.startDate) | ||
| val endTimestamp = SentryLongDate(nanoPrecisionStart.nanoTimestamp() + durationNanos) | ||
| parent.recordChild(sql, nanoPrecisionStart, endTimestamp, status, throwable) | ||
| } | ||
|
|
||
| /** Records a `db.sql.query` span from [startTimestamp] to [startTimestamp] + [durationNanos]. */ | ||
| fun recordSpan( | ||
| /** | ||
| * Records a `db.sql.query` span from [startTimestamp] to the moment of invocation. | ||
| * | ||
| * "Coarse" in that it doesn't ensure nanosecond precision for [SentryNanotimeDate] | ||
| * [startTimestamp]s. | ||
| */ | ||
| fun recordCoarseSpan( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chime in if you can think of a better name. Not in love with this, but it'll do if it has to... |
||
| sql: String, | ||
| startTimestamp: SentryDate, | ||
| durationNanos: Long, | ||
| status: SpanStatus, | ||
| throwable: Throwable? = null, | ||
| ) { | ||
| val endTimestamp = SentryLongDate(startTimestamp.nanoTimestamp() + durationNanos) | ||
| recordSpan(sql, startTimestamp, endTimestamp, status, throwable) | ||
| val parent = scopes.span ?: return | ||
| parent.recordChild(sql, startTimestamp, endTimestamp = null, status, throwable) | ||
| } | ||
|
|
||
| private fun recordSpan( | ||
| private fun ISpan.recordChild( | ||
| sql: String, | ||
| startTimestamp: SentryDate, | ||
| endTimestamp: SentryDate?, | ||
| status: SpanStatus, | ||
| throwable: Throwable?, | ||
| ) { | ||
| scopes.span?.startChild("db.sql.query", sql, startTimestamp, Instrumenter.SENTRY)?.apply { | ||
| startChild("db.sql.query", sql, startTimestamp, Instrumenter.SENTRY).apply { | ||
| spanContext.origin = SQLITE_TRACE_ORIGIN | ||
| throwable?.let { this.throwable = it } | ||
|
|
||
|
|
@@ -74,6 +84,38 @@ internal class SQLiteSpanInstrumentation( | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Repairs the receiver's [nanoTimestamp][SentryDate.nanoTimestamp] if needed so that it actually | ||
| * has nanosecond precision. | ||
| * | ||
| * Designed for use with spans whose start timestamps are [SentryNanotimeDate]s. Without repair, | ||
| * those timestamps will be aligned to the same millisecond at transport, and the Sentry UI will | ||
| * arbitrarily reorder them: | ||
| * ``` | ||
| * Parent span ├█████████████┤ | ||
| * END TRANSACTION ├███┤ 0.18 ms ← (Wrong order) | ||
| * BEGIN IMMEDIATE TRANSACTION ├████┤ 0.25 ms | ||
| * INSERT INTO `my_db` … ├██┤ 0.10 ms | ||
| * ↑ | ||
| * (All spans share the same ms baseline | ||
| * even though their execution was staggered) | ||
| * ``` | ||
| * | ||
| * Repair ensures proper ordering and lets the spans stagger: | ||
| * ``` | ||
| * Parent span ├█████████████┤ | ||
| * BEGIN IMMEDIATE TRANSACTION ├████┤ 0.25 ms | ||
| * INSERT INTO `my_db` … ├██┤ 0.10 ms | ||
| * END TRANSACTION ├███┤ 0.18 ms | ||
| * ``` | ||
| */ | ||
| private fun SentryDate.repairPrecision(baseline: SentryDate?): SentryDate = | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This arguably belongs on [1] adding a ...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 [2] adding a ...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 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. |
||
| if (baseline is SentryNanotimeDate) { | ||
| SentryLongDate(baseline.laterDateNanosTimestampByDiff(this)) | ||
| } else { | ||
| this | ||
| } | ||
|
|
||
| companion object { | ||
|
|
||
| fun fromDatabaseName(databaseName: String?, scopes: IScopes = ScopesAdapter.getInstance()) = | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)