diff --git a/CHANGELOG.md b/CHANGELOG.md index 55ef1a8f2c..0e0dc71961 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ ### Fixes - Keep resolving the server name after `Sentry.close()` or a re-init. Closing the SDK shut down the shared hostname cache for the life of the process, so `server_name` silently froze at the value it had last resolved ([#6119](https://github.com/getsentry/sentry-java/pull/6119)) +- Order breadcrumbs by the timestamp they carry rather than by when they were created in the current process, so breadcrumbs restored from disk or handed over by a hybrid SDK no longer sort as if they had just happened ([#6097](https://github.com/getsentry/sentry-java/pull/6097)) ### Internal diff --git a/sentry/src/main/java/io/sentry/Breadcrumb.java b/sentry/src/main/java/io/sentry/Breadcrumb.java index fff6954ee5..159e8adddf 100644 --- a/sentry/src/main/java/io/sentry/Breadcrumb.java +++ b/sentry/src/main/java/io/sentry/Breadcrumb.java @@ -73,7 +73,9 @@ public Breadcrumb(final long timestamp) { } Breadcrumb(final @NotNull Breadcrumb breadcrumb) { - this.nanos = System.nanoTime(); + // A clone stands in for the breadcrumb it was copied from, so it inherits its tie-breaker + // instead of taking a fresh one and sorting after everything recorded since. + this.nanos = breadcrumb.nanos; this.timestamp = breadcrumb.timestamp; this.timestampMs = breadcrumb.timestampMs; this.message = breadcrumb.message; @@ -832,6 +834,12 @@ public void setUnknown(@Nullable Map unknown) { @Override @SuppressWarnings("JavaUtilDate") public int compareTo(@NotNull Breadcrumb o) { + final int byTimestamp = getTimestamp().compareTo(o.getTimestamp()); + if (byTimestamp != 0) { + return byTimestamp; + } + // Timestamps are millisecond-granular, so breadcrumbs recorded in the same millisecond tie. + // nanos is only meaningful within a process run, which is all a tie-breaker has to cover. return nanos.compareTo(o.nanos); } diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 0e44812a49..4bba195fee 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -26,7 +26,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.List; import java.util.Map; import org.jetbrains.annotations.ApiStatus; @@ -41,7 +40,6 @@ public final class SentryClient implements ISentryClient { private final @NotNull SentryOptions options; private final @NotNull ITransport transport; - private final @NotNull SortBreadcrumbsByDate sortBreadcrumbsByDate = new SortBreadcrumbsByDate(); private final @NotNull ILoggerBatchProcessor loggerBatchProcessor; private final @NotNull IMetricsBatchProcessor metricsBatchProcessor; @@ -1660,7 +1658,7 @@ private void sortBreadcrumbsByDate( if (sortedBreadcrumbs != null && !breadcrumbs.isEmpty()) { sortedBreadcrumbs.addAll(breadcrumbs); - Collections.sort(sortedBreadcrumbs, sortBreadcrumbsByDate); + Collections.sort(sortedBreadcrumbs); } } @@ -1851,13 +1849,4 @@ private boolean sample() { } return true; } - - private static final class SortBreadcrumbsByDate implements Comparator { - - @SuppressWarnings({"JdkObsolete", "JavaUtilDate"}) - @Override - public int compare(final @NotNull Breadcrumb b1, final @NotNull Breadcrumb b2) { - return b1.getTimestamp().compareTo(b2.getTimestamp()); - } - } } diff --git a/sentry/src/test/java/io/sentry/BreadcrumbTest.kt b/sentry/src/test/java/io/sentry/BreadcrumbTest.kt index f51acca81c..d0341f8e58 100644 --- a/sentry/src/test/java/io/sentry/BreadcrumbTest.kt +++ b/sentry/src/test/java/io/sentry/BreadcrumbTest.kt @@ -1,5 +1,7 @@ package io.sentry +import com.google.common.truth.Truth.assertThat +import java.io.StringReader import java.util.Date import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors @@ -365,6 +367,60 @@ class BreadcrumbTest { } } + @Test + fun `breadcrumbs sharing a timestamp keep the order they were recorded in`() { + val timestamp = Date(1_600_000_000_000) + val first = Breadcrumb(timestamp).apply { message = "first" } + val second = Breadcrumb(timestamp).apply { message = "second" } + val third = Breadcrumb(timestamp).apply { message = "third" } + + val sorted = listOf(third, first, second).sorted().map { it.message } + + assertThat(sorted).containsExactly("first", "second", "third").inOrder() + } + + @Test + fun `a deserialized breadcrumb is ordered by its own timestamp, not by when it was parsed`() { + val live = Breadcrumb(Date(1_600_000_000_000)).apply { message = "live" } + val restored = + Breadcrumb.fromMap( + mapOf( + Breadcrumb.JsonKeys.TIMESTAMP to DateUtils.getTimestamp(Date(1_500_000_000_000)), + Breadcrumb.JsonKeys.MESSAGE to "restored", + ), + SentryOptions(), + ) + + val sorted = listOf(live, restored).sorted().map { it.message } + + assertThat(sorted).containsExactly("restored", "live").inOrder() + } + + @Test + fun `a breadcrumb read back from JSON is ordered by its own timestamp, not by when it was parsed`() { + val live = Breadcrumb(Date(1_600_000_000_000)).apply { message = "live" } + val json = + """{"timestamp":"${DateUtils.getTimestamp(Date(1_500_000_000_000))}","message":"restored"}""" + val restored = + Breadcrumb.Deserializer() + .deserialize(JsonObjectReader(StringReader(json)), NoOpLogger.getInstance()) + + val sorted = listOf(live, restored).sorted().map { it.message } + + assertThat(sorted).containsExactly("restored", "live").inOrder() + } + + @Test + fun `cloning a breadcrumb keeps its position among breadcrumbs sharing its timestamp`() { + val timestamp = Date(1_600_000_000_000) + val first = Breadcrumb(timestamp).apply { message = "first" } + val second = Breadcrumb(timestamp).apply { message = "second" } + + val sorted = listOf(second, Breadcrumb(first)).sorted().map { it.message } + + assertThat(sorted).containsExactly("first", "second").inOrder() + } + class TestKey(val id: Long) { override fun toString(): String = id.toString() }