diff --git a/CHANGELOG.md b/CHANGELOG.md index 429dd549a9..3af6f8886d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,24 @@ ## Unreleased +### Behavioral Changes + +- Collections returned by scope (e.g. `getBreadcrumbs`, `getTags`, `getAttachments`) are shared state and should not be mutated. ([#5541](https://github.com/getsentry/sentry-java/pull/5541)) + - Previously, when going through `CombinedScopeView`, we were returning a copy where mutations didn't show up in the underlying scopes. + - This has now changed in order to reduce SDK overhead. + ### Features - Add experimental `SentrySQLiteDriver` to `sentry-android-sqlite` for instrumenting `androidx.sqlite.SQLiteDriver` ([#5563](https://github.com/getsentry/sentry-java/pull/5563)) - To use it, pass `SQLiteDriver` to `SentrySQLiteDriver.create(...)` - Requires `androidx.sqlite:sqlite` (2.5.0+) on runtime classpath (typically provided by Room or SQLDelight) +### Internal + +- Reduce writer buffer size from 8192 to 512 ([#5544](https://github.com/getsentry/sentry-java/pull/5544)) +- Remove redundant event map copies ([#5536](https://github.com/getsentry/sentry-java/pull/5536)) +- Optimize combined scope by adding an early return if only one scope has data ([#5541](https://github.com/getsentry/sentry-java/pull/5541)) + ## 8.44.0 ### Features @@ -36,11 +48,6 @@ - Fix attachments being duplicated on native events that carry scope attachments ([#5548](https://github.com/getsentry/sentry-java/pull/5548)) - Fix performance collector scheduling many tasks in a row ([#5524](https://github.com/getsentry/sentry-java/pull/5524)) -### Internal - -- Reduce writer buffer size from 8192 to 512 ([#5544](https://github.com/getsentry/sentry-java/pull/5544)) -- Remove redundant event map copies ([#5536](https://github.com/getsentry/sentry-java/pull/5536)) - ## 8.43.2 ### Improvements diff --git a/sentry/src/main/java/io/sentry/CombinedScopeView.java b/sentry/src/main/java/io/sentry/CombinedScopeView.java index f21f8697fa..ea2d752d44 100644 --- a/sentry/src/main/java/io/sentry/CombinedScopeView.java +++ b/sentry/src/main/java/io/sentry/CombinedScopeView.java @@ -171,10 +171,31 @@ public void setFingerprint(@NotNull List fingerprint) { @Override public @NotNull Queue getBreadcrumbs() { + final @NotNull Queue globalBreadcrumbs = globalScope.getBreadcrumbs(); + final @NotNull Queue isolationBreadcrumbs = isolationScope.getBreadcrumbs(); + final @NotNull Queue currentBreadcrumbs = scope.getBreadcrumbs(); + + final boolean hasGlobalBreadcrumbs = !globalBreadcrumbs.isEmpty(); + final boolean hasIsolationBreadcrumbs = !isolationBreadcrumbs.isEmpty(); + final boolean hasCurrentBreadcrumbs = !currentBreadcrumbs.isEmpty(); + + if (!hasGlobalBreadcrumbs && !hasIsolationBreadcrumbs && !hasCurrentBreadcrumbs) { + return getDefaultScopeValue(globalBreadcrumbs, isolationBreadcrumbs, currentBreadcrumbs); + } + if (!hasIsolationBreadcrumbs && !hasCurrentBreadcrumbs) { + return globalBreadcrumbs; + } + if (!hasGlobalBreadcrumbs && !hasCurrentBreadcrumbs) { + return isolationBreadcrumbs; + } + if (!hasGlobalBreadcrumbs && !hasIsolationBreadcrumbs) { + return currentBreadcrumbs; + } + final @NotNull List allBreadcrumbs = new ArrayList<>(); - allBreadcrumbs.addAll(globalScope.getBreadcrumbs()); - allBreadcrumbs.addAll(isolationScope.getBreadcrumbs()); - allBreadcrumbs.addAll(scope.getBreadcrumbs()); + allBreadcrumbs.addAll(globalBreadcrumbs); + allBreadcrumbs.addAll(isolationBreadcrumbs); + allBreadcrumbs.addAll(currentBreadcrumbs); Collections.sort(allBreadcrumbs); final @NotNull Queue breadcrumbs = @@ -224,10 +245,31 @@ public void clear() { @Override public @NotNull Map getTags() { + final @NotNull Map globalTags = globalScope.getTags(); + final @NotNull Map isolationTags = isolationScope.getTags(); + final @NotNull Map currentTags = scope.getTags(); + + final boolean hasGlobalTags = !globalTags.isEmpty(); + final boolean hasIsolationTags = !isolationTags.isEmpty(); + final boolean hasCurrentTags = !currentTags.isEmpty(); + + if (!hasGlobalTags && !hasIsolationTags && !hasCurrentTags) { + return getDefaultScopeValue(globalTags, isolationTags, currentTags); + } + if (!hasIsolationTags && !hasCurrentTags) { + return globalTags; + } + if (!hasGlobalTags && !hasCurrentTags) { + return isolationTags; + } + if (!hasGlobalTags && !hasIsolationTags) { + return currentTags; + } + final @NotNull Map allTags = new ConcurrentHashMap<>(); - allTags.putAll(globalScope.getTags()); - allTags.putAll(isolationScope.getTags()); - allTags.putAll(scope.getTags()); + allTags.putAll(globalTags); + allTags.putAll(isolationTags); + allTags.putAll(currentTags); return allTags; } @@ -243,10 +285,32 @@ public void removeTag(@Nullable String key) { @Override public @NotNull Map getAttributes() { + final @NotNull Map globalAttributes = globalScope.getAttributes(); + final @NotNull Map isolationAttributes = + isolationScope.getAttributes(); + final @NotNull Map currentAttributes = scope.getAttributes(); + + final boolean hasGlobalAttributes = !globalAttributes.isEmpty(); + final boolean hasIsolationAttributes = !isolationAttributes.isEmpty(); + final boolean hasCurrentAttributes = !currentAttributes.isEmpty(); + + if (!hasGlobalAttributes && !hasIsolationAttributes && !hasCurrentAttributes) { + return getDefaultScopeValue(globalAttributes, isolationAttributes, currentAttributes); + } + if (!hasIsolationAttributes && !hasCurrentAttributes) { + return globalAttributes; + } + if (!hasGlobalAttributes && !hasCurrentAttributes) { + return isolationAttributes; + } + if (!hasGlobalAttributes && !hasIsolationAttributes) { + return currentAttributes; + } + final @NotNull Map allAttributes = new ConcurrentHashMap<>(); - allAttributes.putAll(globalScope.getAttributes()); - allAttributes.putAll(isolationScope.getAttributes()); - allAttributes.putAll(scope.getAttributes()); + allAttributes.putAll(globalAttributes); + allAttributes.putAll(isolationAttributes); + allAttributes.putAll(currentAttributes); return allAttributes; } @@ -272,11 +336,32 @@ public void removeAttribute(@Nullable String key) { @Override public @NotNull Map getExtras() { - final @NotNull Map allTags = new ConcurrentHashMap<>(); - allTags.putAll(globalScope.getExtras()); - allTags.putAll(isolationScope.getExtras()); - allTags.putAll(scope.getExtras()); - return allTags; + final @NotNull Map globalExtras = globalScope.getExtras(); + final @NotNull Map isolationExtras = isolationScope.getExtras(); + final @NotNull Map currentExtras = scope.getExtras(); + + final boolean hasGlobalExtras = !globalExtras.isEmpty(); + final boolean hasIsolationExtras = !isolationExtras.isEmpty(); + final boolean hasCurrentExtras = !currentExtras.isEmpty(); + + if (!hasGlobalExtras && !hasIsolationExtras && !hasCurrentExtras) { + return getDefaultScopeValue(globalExtras, isolationExtras, currentExtras); + } + if (!hasIsolationExtras && !hasCurrentExtras) { + return globalExtras; + } + if (!hasGlobalExtras && !hasCurrentExtras) { + return isolationExtras; + } + if (!hasGlobalExtras && !hasIsolationExtras) { + return currentExtras; + } + + final @NotNull Map allExtras = new ConcurrentHashMap<>(); + allExtras.putAll(globalExtras); + allExtras.putAll(isolationExtras); + allExtras.putAll(currentExtras); + return allExtras; } @Override @@ -342,6 +427,23 @@ public void removeContexts(@Nullable String key) { return getSpecificScope(null); } + private @NotNull T getDefaultScopeValue( + final @NotNull T globalValue, + final @NotNull T isolationValue, + final @NotNull T currentValue) { + switch (getOptions().getDefaultScopeType()) { + case CURRENT: + return currentValue; + case ISOLATION: + return isolationValue; + case GLOBAL: + return globalValue; + default: + // calm the compiler + return currentValue; + } + } + IScope getSpecificScope(final @Nullable ScopeType scopeType) { if (scopeType != null) { switch (scopeType) { @@ -373,10 +475,31 @@ IScope getSpecificScope(final @Nullable ScopeType scopeType) { @Override public @NotNull List getAttachments() { + final @NotNull List globalAttachments = globalScope.getAttachments(); + final @NotNull List isolationAttachments = isolationScope.getAttachments(); + final @NotNull List currentAttachments = scope.getAttachments(); + + final boolean hasGlobalAttachments = !globalAttachments.isEmpty(); + final boolean hasIsolationAttachments = !isolationAttachments.isEmpty(); + final boolean hasCurrentAttachments = !currentAttachments.isEmpty(); + + if (!hasGlobalAttachments && !hasIsolationAttachments && !hasCurrentAttachments) { + return getDefaultScopeValue(globalAttachments, isolationAttachments, currentAttachments); + } + if (!hasIsolationAttachments && !hasCurrentAttachments) { + return globalAttachments; + } + if (!hasGlobalAttachments && !hasCurrentAttachments) { + return isolationAttachments; + } + if (!hasGlobalAttachments && !hasIsolationAttachments) { + return currentAttachments; + } + final @NotNull List allAttachments = new CopyOnWriteArrayList<>(); - allAttachments.addAll(globalScope.getAttachments()); - allAttachments.addAll(isolationScope.getAttachments()); - allAttachments.addAll(scope.getAttachments()); + allAttachments.addAll(globalAttachments); + allAttachments.addAll(isolationAttachments); + allAttachments.addAll(currentAttachments); return allAttachments; } diff --git a/sentry/src/test/java/io/sentry/CombinedScopeViewTest.kt b/sentry/src/test/java/io/sentry/CombinedScopeViewTest.kt index d768d6d32d..fd187235a9 100644 --- a/sentry/src/test/java/io/sentry/CombinedScopeViewTest.kt +++ b/sentry/src/test/java/io/sentry/CombinedScopeViewTest.kt @@ -11,6 +11,7 @@ import junit.framework.TestCase.assertTrue import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotNull +import kotlin.test.assertNotSame import kotlin.test.assertNull import kotlin.test.assertSame import org.junit.Assert.assertNotEquals @@ -72,6 +73,74 @@ class CombinedScopeViewTest { assertEquals("current 2", breadcrumbs.poll().message) } + @Test + fun `returns single non-empty breadcrumb queue directly`() { + var combined = fixture.getSut() + fixture.globalScope.addBreadcrumb(Breadcrumb.info("global")) + assertSame(fixture.globalScope.breadcrumbs, combined.breadcrumbs) + + combined = fixture.getSut() + fixture.isolationScope.addBreadcrumb(Breadcrumb.info("isolation")) + assertSame(fixture.isolationScope.breadcrumbs, combined.breadcrumbs) + + combined = fixture.getSut() + fixture.scope.addBreadcrumb(Breadcrumb.info("current")) + assertSame(fixture.scope.breadcrumbs, combined.breadcrumbs) + } + + @Test + fun `returns default write scope breadcrumbs when all scopes are empty`() { + val combined = fixture.getSut(SentryOptions().also { it.defaultScopeType = ScopeType.CURRENT }) + + assertSame(fixture.scope.breadcrumbs, combined.breadcrumbs) + } + + @Test + fun `returns merged breadcrumb copy when multiple scopes have breadcrumbs`() { + val combined = fixture.getSut() + + fixture.globalScope.addBreadcrumb(Breadcrumb.info("global")) + fixture.isolationScope.addBreadcrumb(Breadcrumb.info("isolation")) + + val breadcrumbs = combined.breadcrumbs + + assertNotSame(fixture.globalScope.breadcrumbs, breadcrumbs) + assertNotSame(fixture.isolationScope.breadcrumbs, breadcrumbs) + assertEquals(2, breadcrumbs.size) + } + + @Test + fun `returns single non-empty combined collections directly`() { + val globalScope = mock() + val isolationScope = mock() + val scope = mock() + val combined = CombinedScopeView(globalScope, isolationScope, scope) + + val tags = mapOf("tag" to "value") + whenever(globalScope.tags).thenReturn(emptyMap()) + whenever(isolationScope.tags).thenReturn(emptyMap()) + whenever(scope.tags).thenReturn(tags) + assertSame(tags, combined.tags) + + val attributes = mapOf("attribute" to SentryAttribute.named("attribute", "value")) + whenever(globalScope.attributes).thenReturn(emptyMap()) + whenever(isolationScope.attributes).thenReturn(emptyMap()) + whenever(scope.attributes).thenReturn(attributes) + assertSame(attributes, combined.attributes) + + val extras = mapOf("extra" to "value") + whenever(globalScope.extras).thenReturn(emptyMap()) + whenever(isolationScope.extras).thenReturn(emptyMap()) + whenever(scope.extras).thenReturn(extras) + assertSame(extras, combined.extras) + + val attachments = listOf(createAttachment("attachment.png")) + whenever(globalScope.attachments).thenReturn(emptyList()) + whenever(isolationScope.attachments).thenReturn(emptyList()) + whenever(scope.attachments).thenReturn(attachments) + assertSame(attachments, combined.attachments) + } + @Test fun `oldest breadcrumbs are dropped first`() { val options = SentryOptions().also { it.maxBreadcrumbs = 5 }