From 89254351f6db9219450551fb80cc55a8b6f9a45c Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 2 Mar 2026 16:45:16 +0100 Subject: [PATCH 1/3] feat(spring): [Cache Tracing 1] Add SentryCacheWrapper and SentryCacheManagerWrapper Co-Authored-By: Claude Opus 4.6 --- sentry-spring-7/api/sentry-spring-7.api | 21 ++ .../cache/SentryCacheManagerWrapper.java | 37 +++ .../spring7/cache/SentryCacheWrapper.java | 229 ++++++++++++++++ .../cache/SentryCacheManagerWrapperTest.kt | 48 ++++ .../spring7/cache/SentryCacheWrapperTest.kt | 259 ++++++++++++++++++ sentry/api/sentry.api | 2 + .../java/io/sentry/SpanDataConvention.java | 2 + 7 files changed, 598 insertions(+) create mode 100644 sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java create mode 100644 sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java create mode 100644 sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt create mode 100644 sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt diff --git a/sentry-spring-7/api/sentry-spring-7.api b/sentry-spring-7/api/sentry-spring-7.api index 3a57c13e835..41514d95552 100644 --- a/sentry-spring-7/api/sentry-spring-7.api +++ b/sentry-spring-7/api/sentry-spring-7.api @@ -104,6 +104,27 @@ public final class io/sentry/spring7/SpringSecuritySentryUserProvider : io/sentr public fun provideUser ()Lio/sentry/protocol/User; } +public final class io/sentry/spring7/cache/SentryCacheManagerWrapper : org/springframework/cache/CacheManager { + public fun (Lorg/springframework/cache/CacheManager;Lio/sentry/IScopes;)V + public fun getCache (Ljava/lang/String;)Lorg/springframework/cache/Cache; + public fun getCacheNames ()Ljava/util/Collection; +} + +public final class io/sentry/spring7/cache/SentryCacheWrapper : org/springframework/cache/Cache { + public fun (Lorg/springframework/cache/Cache;Lio/sentry/IScopes;)V + public fun clear ()V + public fun evict (Ljava/lang/Object;)V + public fun evictIfPresent (Ljava/lang/Object;)Z + public fun get (Ljava/lang/Object;)Lorg/springframework/cache/Cache$ValueWrapper; + public fun get (Ljava/lang/Object;Ljava/lang/Class;)Ljava/lang/Object; + public fun get (Ljava/lang/Object;Ljava/util/concurrent/Callable;)Ljava/lang/Object; + public fun getName ()Ljava/lang/String; + public fun getNativeCache ()Ljava/lang/Object; + public fun invalidate ()Z + public fun put (Ljava/lang/Object;Ljava/lang/Object;)V + public fun putIfAbsent (Ljava/lang/Object;Ljava/lang/Object;)Lorg/springframework/cache/Cache$ValueWrapper; +} + public abstract interface annotation class io/sentry/spring7/checkin/SentryCheckIn : java/lang/annotation/Annotation { public abstract fun heartbeat ()Z public abstract fun monitorSlug ()Ljava/lang/String; diff --git a/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java new file mode 100644 index 00000000000..5a52734756b --- /dev/null +++ b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java @@ -0,0 +1,37 @@ +package io.sentry.spring7.cache; + +import io.sentry.IScopes; +import java.util.Collection; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.springframework.cache.Cache; +import org.springframework.cache.CacheManager; + +/** Wraps a Spring {@link CacheManager} to return Sentry-instrumented caches. */ +@ApiStatus.Internal +public final class SentryCacheManagerWrapper implements CacheManager { + + private final @NotNull CacheManager delegate; + private final @NotNull IScopes scopes; + + public SentryCacheManagerWrapper( + final @NotNull CacheManager delegate, final @NotNull IScopes scopes) { + this.delegate = delegate; + this.scopes = scopes; + } + + @Override + public @Nullable Cache getCache(final @NotNull String name) { + final Cache cache = delegate.getCache(name); + if (cache == null) { + return null; + } + return new SentryCacheWrapper(cache, scopes); + } + + @Override + public @NotNull Collection getCacheNames() { + return delegate.getCacheNames(); + } +} diff --git a/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java new file mode 100644 index 00000000000..068435bf800 --- /dev/null +++ b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java @@ -0,0 +1,229 @@ +package io.sentry.spring7.cache; + +import io.sentry.IScopes; +import io.sentry.ISpan; +import io.sentry.SpanDataConvention; +import io.sentry.SpanOptions; +import io.sentry.SpanStatus; +import java.util.Arrays; +import java.util.concurrent.Callable; +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.springframework.cache.Cache; + +/** Wraps a Spring {@link Cache} to create Sentry spans for cache operations. */ +@ApiStatus.Internal +public final class SentryCacheWrapper implements Cache { + + private static final String TRACE_ORIGIN = "auto.cache.spring"; + + private final @NotNull Cache delegate; + private final @NotNull IScopes scopes; + + public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes scopes) { + this.delegate = delegate; + this.scopes = scopes; + } + + @Override + public @NotNull String getName() { + return delegate.getName(); + } + + @Override + public @NotNull Object getNativeCache() { + return delegate.getNativeCache(); + } + + @Override + public @Nullable ValueWrapper get(final @NotNull Object key) { + final ISpan span = startSpan("cache.get", key); + if (span == null) { + return delegate.get(key); + } + try { + final ValueWrapper result = delegate.get(key); + span.setData(SpanDataConvention.CACHE_HIT_KEY, result != null); + span.setStatus(SpanStatus.OK); + return result; + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public @Nullable T get(final @NotNull Object key, final @Nullable Class type) { + final ISpan span = startSpan("cache.get", key); + if (span == null) { + return delegate.get(key, type); + } + try { + final T result = delegate.get(key, type); + span.setData(SpanDataConvention.CACHE_HIT_KEY, result != null); + span.setStatus(SpanStatus.OK); + return result; + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public @Nullable T get(final @NotNull Object key, final @NotNull Callable valueLoader) { + final ISpan span = startSpan("cache.get", key); + if (span == null) { + return delegate.get(key, valueLoader); + } + try { + final T result = delegate.get(key, valueLoader); + // valueLoader is called on miss, so the method always returns a value + span.setData(SpanDataConvention.CACHE_HIT_KEY, true); + span.setStatus(SpanStatus.OK); + return result; + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public void put(final @NotNull Object key, final @Nullable Object value) { + final ISpan span = startSpan("cache.put", key); + if (span == null) { + delegate.put(key, value); + return; + } + try { + delegate.put(key, value); + span.setStatus(SpanStatus.OK); + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public @Nullable ValueWrapper putIfAbsent( + final @NotNull Object key, final @Nullable Object value) { + final ISpan span = startSpan("cache.put", key); + if (span == null) { + return delegate.putIfAbsent(key, value); + } + try { + final ValueWrapper result = delegate.putIfAbsent(key, value); + span.setStatus(SpanStatus.OK); + return result; + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public void evict(final @NotNull Object key) { + final ISpan span = startSpan("cache.remove", key); + if (span == null) { + delegate.evict(key); + return; + } + try { + delegate.evict(key); + span.setStatus(SpanStatus.OK); + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public boolean evictIfPresent(final @NotNull Object key) { + final ISpan span = startSpan("cache.remove", key); + if (span == null) { + return delegate.evictIfPresent(key); + } + try { + final boolean result = delegate.evictIfPresent(key); + span.setStatus(SpanStatus.OK); + return result; + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public void clear() { + final ISpan span = startSpan("cache.flush", null); + if (span == null) { + delegate.clear(); + return; + } + try { + delegate.clear(); + span.setStatus(SpanStatus.OK); + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + @Override + public boolean invalidate() { + final ISpan span = startSpan("cache.flush", null); + if (span == null) { + return delegate.invalidate(); + } + try { + final boolean result = delegate.invalidate(); + span.setStatus(SpanStatus.OK); + return result; + } catch (Throwable e) { + span.setStatus(SpanStatus.INTERNAL_ERROR); + span.setThrowable(e); + throw e; + } finally { + span.finish(); + } + } + + private @Nullable ISpan startSpan(final @NotNull String operation, final @Nullable Object key) { + final ISpan activeSpan = scopes.getSpan(); + if (activeSpan == null || activeSpan.isNoOp()) { + return null; + } + + final SpanOptions spanOptions = new SpanOptions(); + spanOptions.setOrigin(TRACE_ORIGIN); + final ISpan span = activeSpan.startChild(operation, getName(), spanOptions); + if (key != null) { + span.setData(SpanDataConvention.CACHE_KEY_KEY, Arrays.asList(String.valueOf(key))); + } + return span; + } +} diff --git a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt new file mode 100644 index 00000000000..ca56be7fbc9 --- /dev/null +++ b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt @@ -0,0 +1,48 @@ +package io.sentry.spring7.cache + +import io.sentry.IScopes +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull +import kotlin.test.assertTrue +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.springframework.cache.Cache +import org.springframework.cache.CacheManager + +class SentryCacheManagerWrapperTest { + + private val scopes: IScopes = mock() + private val delegate: CacheManager = mock() + + @Test + fun `getCache wraps returned cache in SentryCacheWrapper`() { + val cache = mock() + whenever(delegate.getCache("test")).thenReturn(cache) + + val wrapper = SentryCacheManagerWrapper(delegate, scopes) + val result = wrapper.getCache("test") + + assertTrue(result is SentryCacheWrapper) + } + + @Test + fun `getCache returns null when delegate returns null`() { + whenever(delegate.getCache("missing")).thenReturn(null) + + val wrapper = SentryCacheManagerWrapper(delegate, scopes) + val result = wrapper.getCache("missing") + + assertNull(result) + } + + @Test + fun `getCacheNames delegates to underlying cache manager`() { + whenever(delegate.cacheNames).thenReturn(listOf("cache1", "cache2")) + + val wrapper = SentryCacheManagerWrapper(delegate, scopes) + val result = wrapper.cacheNames + + assertEquals(listOf("cache1", "cache2"), result) + } +} diff --git a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt new file mode 100644 index 00000000000..8187b487e8b --- /dev/null +++ b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt @@ -0,0 +1,259 @@ +package io.sentry.spring7.cache + +import io.sentry.IScopes +import io.sentry.SentryOptions +import io.sentry.SentryTracer +import io.sentry.SpanDataConvention +import io.sentry.SpanStatus +import io.sentry.TransactionContext +import java.util.concurrent.Callable +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertTrue +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.springframework.cache.Cache + +class SentryCacheWrapperTest { + + private lateinit var scopes: IScopes + private lateinit var delegate: Cache + + @BeforeTest + fun setup() { + scopes = mock() + delegate = mock() + whenever(scopes.options).thenReturn(SentryOptions()) + whenever(delegate.name).thenReturn("testCache") + } + + private fun createTransaction(): SentryTracer { + val tx = SentryTracer(TransactionContext("tx", "op"), scopes) + whenever(scopes.span).thenReturn(tx) + return tx + } + + // -- get(Object key) -- + + @Test + fun `get with ValueWrapper creates span with cache hit true on hit`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + val valueWrapper = mock() + whenever(delegate.get("myKey")).thenReturn(valueWrapper) + + val result = wrapper.get("myKey") + + assertEquals(valueWrapper, result) + assertEquals(1, tx.spans.size) + val span = tx.spans.first() + assertEquals("cache.get", span.operation) + assertEquals("testCache", span.description) + assertEquals(SpanStatus.OK, span.status) + assertEquals(true, span.getData(SpanDataConvention.CACHE_HIT_KEY)) + assertEquals(listOf("myKey"), span.getData(SpanDataConvention.CACHE_KEY_KEY)) + assertEquals("auto.cache.spring", span.spanContext.origin) + } + + @Test + fun `get with ValueWrapper creates span with cache hit false on miss`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.get("myKey")).thenReturn(null) + + val result = wrapper.get("myKey") + + assertNull(result) + assertEquals(1, tx.spans.size) + assertEquals(false, tx.spans.first().getData(SpanDataConvention.CACHE_HIT_KEY)) + } + + // -- get(Object key, Class) -- + + @Test + fun `get with type creates span with cache hit true on hit`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.get("myKey", String::class.java)).thenReturn("value") + + val result = wrapper.get("myKey", String::class.java) + + assertEquals("value", result) + assertEquals(1, tx.spans.size) + assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT_KEY)) + } + + @Test + fun `get with type creates span with cache hit false on miss`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.get("myKey", String::class.java)).thenReturn(null) + + val result = wrapper.get("myKey", String::class.java) + + assertNull(result) + assertEquals(1, tx.spans.size) + assertEquals(false, tx.spans.first().getData(SpanDataConvention.CACHE_HIT_KEY)) + } + + // -- get(Object key, Callable) -- + + @Test + fun `get with callable creates span with cache hit true`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + val callable = Callable { "loaded" } + whenever(delegate.get("myKey", callable)).thenReturn("loaded") + + val result = wrapper.get("myKey", callable) + + assertEquals("loaded", result) + assertEquals(1, tx.spans.size) + assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT_KEY)) + } + + // -- put -- + + @Test + fun `put creates cache put span`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + + wrapper.put("myKey", "myValue") + + verify(delegate).put("myKey", "myValue") + assertEquals(1, tx.spans.size) + val span = tx.spans.first() + assertEquals("cache.put", span.operation) + assertEquals(SpanStatus.OK, span.status) + assertEquals(listOf("myKey"), span.getData(SpanDataConvention.CACHE_KEY_KEY)) + } + + // -- putIfAbsent -- + + @Test + fun `putIfAbsent creates cache put span`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.putIfAbsent("myKey", "myValue")).thenReturn(null) + + wrapper.putIfAbsent("myKey", "myValue") + + assertEquals(1, tx.spans.size) + assertEquals("cache.put", tx.spans.first().operation) + } + + // -- evict -- + + @Test + fun `evict creates cache remove span`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + + wrapper.evict("myKey") + + verify(delegate).evict("myKey") + assertEquals(1, tx.spans.size) + val span = tx.spans.first() + assertEquals("cache.remove", span.operation) + assertEquals(SpanStatus.OK, span.status) + } + + // -- evictIfPresent -- + + @Test + fun `evictIfPresent creates cache remove span`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.evictIfPresent("myKey")).thenReturn(true) + + val result = wrapper.evictIfPresent("myKey") + + assertTrue(result) + assertEquals(1, tx.spans.size) + assertEquals("cache.remove", tx.spans.first().operation) + } + + // -- clear -- + + @Test + fun `clear creates cache flush span`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + + wrapper.clear() + + verify(delegate).clear() + assertEquals(1, tx.spans.size) + val span = tx.spans.first() + assertEquals("cache.flush", span.operation) + assertEquals(SpanStatus.OK, span.status) + assertNull(span.getData(SpanDataConvention.CACHE_KEY_KEY)) + } + + // -- invalidate -- + + @Test + fun `invalidate creates cache flush span`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.invalidate()).thenReturn(true) + + val result = wrapper.invalidate() + + assertTrue(result) + assertEquals(1, tx.spans.size) + assertEquals("cache.flush", tx.spans.first().operation) + } + + // -- no span when no active transaction -- + + @Test + fun `does not create span when there is no active transaction`() { + whenever(scopes.span).thenReturn(null) + val wrapper = SentryCacheWrapper(delegate, scopes) + whenever(delegate.get("myKey")).thenReturn(null) + + wrapper.get("myKey") + + verify(delegate).get("myKey") + } + + // -- error handling -- + + @Test + fun `sets error status and throwable on exception`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + val exception = RuntimeException("cache error") + whenever(delegate.get("myKey")).thenThrow(exception) + + assertFailsWith { wrapper.get("myKey") } + + assertEquals(1, tx.spans.size) + val span = tx.spans.first() + assertEquals(SpanStatus.INTERNAL_ERROR, span.status) + assertEquals(exception, span.throwable) + } + + // -- delegation -- + + @Test + fun `getName delegates to underlying cache`() { + val wrapper = SentryCacheWrapper(delegate, scopes) + assertEquals("testCache", wrapper.name) + } + + @Test + fun `getNativeCache delegates to underlying cache`() { + val nativeCache = Object() + whenever(delegate.nativeCache).thenReturn(nativeCache) + val wrapper = SentryCacheWrapper(delegate, scopes) + + assertEquals(nativeCache, wrapper.nativeCache) + } +} diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 4399b191d21..1835f7043ca 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4277,6 +4277,8 @@ public final class io/sentry/SpanContext$JsonKeys { public abstract interface class io/sentry/SpanDataConvention { public static final field BLOCKED_MAIN_THREAD_KEY Ljava/lang/String; + public static final field CACHE_HIT_KEY Ljava/lang/String; + public static final field CACHE_KEY_KEY Ljava/lang/String; public static final field CALL_STACK_KEY Ljava/lang/String; public static final field CONTRIBUTES_TTFD Ljava/lang/String; public static final field CONTRIBUTES_TTID Ljava/lang/String; diff --git a/sentry/src/main/java/io/sentry/SpanDataConvention.java b/sentry/src/main/java/io/sentry/SpanDataConvention.java index c4329f6dcad..8a31a7c70ff 100644 --- a/sentry/src/main/java/io/sentry/SpanDataConvention.java +++ b/sentry/src/main/java/io/sentry/SpanDataConvention.java @@ -26,4 +26,6 @@ public interface SpanDataConvention { String HTTP_START_TIMESTAMP = "http.start_timestamp"; String HTTP_END_TIMESTAMP = "http.end_timestamp"; String PROFILER_ID = "profiler_id"; + String CACHE_HIT_KEY = "cache.hit"; + String CACHE_KEY_KEY = "cache.key"; } From da5bde079338d48f6fe0a9d9c8bfd6f421e599dd Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 9 Mar 2026 09:06:23 +0100 Subject: [PATCH 2/3] fix(cache): Fix span description, putIfAbsent, and Callable hit detection - Use cache key as span description instead of cache name, matching the spec and other SDKs (Python, JavaScript) - Skip instrumentation for putIfAbsent since we cannot know if a write actually occurred; override to bypass default get()+put() delegation - Wrap valueLoader Callable in get(key, Callable) to detect cache hit/miss instead of always reporting hit=true - Update tests to match new behavior Co-Authored-By: Claude --- .../spring7/cache/SentryCacheWrapper.java | 40 +++++++++---------- .../spring7/cache/SentryCacheWrapperTest.kt | 37 ++++++++++++----- 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java index 068435bf800..f2ed581793f 100644 --- a/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java +++ b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java @@ -7,6 +7,7 @@ import io.sentry.SpanStatus; import java.util.Arrays; import java.util.concurrent.Callable; +import java.util.concurrent.atomic.AtomicBoolean; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -83,9 +84,15 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes return delegate.get(key, valueLoader); } try { - final T result = delegate.get(key, valueLoader); - // valueLoader is called on miss, so the method always returns a value - span.setData(SpanDataConvention.CACHE_HIT_KEY, true); + final AtomicBoolean loaderInvoked = new AtomicBoolean(false); + final T result = + delegate.get( + key, + () -> { + loaderInvoked.set(true); + return valueLoader.call(); + }); + span.setData(SpanDataConvention.CACHE_HIT_KEY, !loaderInvoked.get()); span.setStatus(SpanStatus.OK); return result; } catch (Throwable e) { @@ -116,24 +123,14 @@ public void put(final @NotNull Object key, final @Nullable Object value) { } } + // putIfAbsent is not instrumented — we cannot know ahead of time whether the put + // will actually happen, and emitting a cache.put span for a no-op would be misleading. + // This matches sentry-python and sentry-javascript which also skip conditional puts. + // We must override to bypass the default implementation which calls this.get() + this.put(). @Override public @Nullable ValueWrapper putIfAbsent( final @NotNull Object key, final @Nullable Object value) { - final ISpan span = startSpan("cache.put", key); - if (span == null) { - return delegate.putIfAbsent(key, value); - } - try { - final ValueWrapper result = delegate.putIfAbsent(key, value); - span.setStatus(SpanStatus.OK); - return result; - } catch (Throwable e) { - span.setStatus(SpanStatus.INTERNAL_ERROR); - span.setThrowable(e); - throw e; - } finally { - span.finish(); - } + return delegate.putIfAbsent(key, value); } @Override @@ -220,9 +217,10 @@ public boolean invalidate() { final SpanOptions spanOptions = new SpanOptions(); spanOptions.setOrigin(TRACE_ORIGIN); - final ISpan span = activeSpan.startChild(operation, getName(), spanOptions); - if (key != null) { - span.setData(SpanDataConvention.CACHE_KEY_KEY, Arrays.asList(String.valueOf(key))); + final String keyString = key != null ? String.valueOf(key) : null; + final ISpan span = activeSpan.startChild(operation, keyString, spanOptions); + if (keyString != null) { + span.setData(SpanDataConvention.CACHE_KEY_KEY, Arrays.asList(keyString)); } return span; } diff --git a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt index 8187b487e8b..2d59bdc8710 100644 --- a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt +++ b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt @@ -13,6 +13,8 @@ import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNull import kotlin.test.assertTrue +import org.mockito.kotlin.any +import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -52,7 +54,7 @@ class SentryCacheWrapperTest { assertEquals(1, tx.spans.size) val span = tx.spans.first() assertEquals("cache.get", span.operation) - assertEquals("testCache", span.description) + assertEquals("myKey", span.description) assertEquals(SpanStatus.OK, span.status) assertEquals(true, span.getData(SpanDataConvention.CACHE_HIT_KEY)) assertEquals(listOf("myKey"), span.getData(SpanDataConvention.CACHE_KEY_KEY)) @@ -103,19 +105,36 @@ class SentryCacheWrapperTest { // -- get(Object key, Callable) -- @Test - fun `get with callable creates span with cache hit true`() { + fun `get with callable creates span with cache hit true on hit`() { val tx = createTransaction() val wrapper = SentryCacheWrapper(delegate, scopes) - val callable = Callable { "loaded" } - whenever(delegate.get("myKey", callable)).thenReturn("loaded") + // Simulate cache hit: delegate returns value without invoking the loader + whenever(delegate.get(eq("myKey"), any>())).thenReturn("cached") - val result = wrapper.get("myKey", callable) + val result = wrapper.get("myKey", Callable { "loaded" }) - assertEquals("loaded", result) + assertEquals("cached", result) assertEquals(1, tx.spans.size) assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT_KEY)) } + @Test + fun `get with callable creates span with cache hit false on miss`() { + val tx = createTransaction() + val wrapper = SentryCacheWrapper(delegate, scopes) + // Simulate cache miss: delegate invokes the loader callable + whenever(delegate.get(eq("myKey"), any>())).thenAnswer { invocation -> + val loader = invocation.getArgument>(1) + loader.call() + } + + val result = wrapper.get("myKey", Callable { "loaded" }) + + assertEquals("loaded", result) + assertEquals(1, tx.spans.size) + assertEquals(false, tx.spans.first().getData(SpanDataConvention.CACHE_HIT_KEY)) + } + // -- put -- @Test @@ -136,15 +155,15 @@ class SentryCacheWrapperTest { // -- putIfAbsent -- @Test - fun `putIfAbsent creates cache put span`() { + fun `putIfAbsent delegates without creating span`() { val tx = createTransaction() val wrapper = SentryCacheWrapper(delegate, scopes) whenever(delegate.putIfAbsent("myKey", "myValue")).thenReturn(null) wrapper.putIfAbsent("myKey", "myValue") - assertEquals(1, tx.spans.size) - assertEquals("cache.put", tx.spans.first().operation) + verify(delegate).putIfAbsent("myKey", "myValue") + assertEquals(0, tx.spans.size) } // -- evict -- From 0b1b83af471e6929a21f8e72dafe7a83c2de3286 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 10 Mar 2026 14:10:46 +0100 Subject: [PATCH 3/3] fix(spring7): Avoid double-wrapping caches in SentryCacheManagerWrapper Co-Authored-By: Claude Opus 4.6 --- .../spring7/cache/SentryCacheManagerWrapper.java | 4 ++-- .../spring7/cache/SentryCacheManagerWrapperTest.kt | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java index 5a52734756b..97ac313bd91 100644 --- a/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java +++ b/sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheManagerWrapper.java @@ -24,8 +24,8 @@ public SentryCacheManagerWrapper( @Override public @Nullable Cache getCache(final @NotNull String name) { final Cache cache = delegate.getCache(name); - if (cache == null) { - return null; + if (cache == null || cache instanceof SentryCacheWrapper) { + return cache; } return new SentryCacheWrapper(cache, scopes); } diff --git a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt index ca56be7fbc9..dbc5992b7e0 100644 --- a/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt +++ b/sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheManagerWrapperTest.kt @@ -4,6 +4,7 @@ import io.sentry.IScopes import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNull +import kotlin.test.assertSame import kotlin.test.assertTrue import org.mockito.kotlin.mock import org.mockito.kotlin.whenever @@ -36,6 +37,18 @@ class SentryCacheManagerWrapperTest { assertNull(result) } + @Test + fun `getCache does not double-wrap SentryCacheWrapper`() { + val innerCache = mock() + val alreadyWrapped = SentryCacheWrapper(innerCache, scopes) + whenever(delegate.getCache("test")).thenReturn(alreadyWrapped) + + val wrapper = SentryCacheManagerWrapper(delegate, scopes) + val result = wrapper.getCache("test") + + assertSame(alreadyWrapped, result) + } + @Test fun `getCacheNames delegates to underlying cache manager`() { whenever(delegate.cacheNames).thenReturn(listOf("cache1", "cache2"))