Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
public final class SentryJCacheWrapper<K, V> implements Cache<K, V> {

private static final String TRACE_ORIGIN = "auto.cache.jcache";
private static final String OPERATION_ATTRIBUTE = "db.operation.name";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated constant belongs in shared SpanDataConvention interface

Low Severity

The OPERATION_ATTRIBUTE = "db.operation.name" constant is independently defined as a private static final in four separate wrapper classes. All four files already import and use SpanDataConvention for CACHE_HIT_KEY and CACHE_KEY_KEY. Adding a DB_OPERATION_NAME_KEY constant to SpanDataConvention would follow the established pattern, eliminate the duplication, and ensure a single source of truth for this attribute name.

Additional Locations (2)
Fix in Cursor Fix in Web

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As db.operation.name does not match any keys listed in the sentry span data convention here: https://develop.sentry.dev/sdk/performance/span-data-conventions/. I think it is fine as is. @adinauer wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move the constant but I first have to clarify internally what the attribute key should be.


private final @NotNull Cache<K, V> delegate;
private final @NotNull IScopes scopes;
Expand All @@ -50,7 +51,7 @@ public SentryJCacheWrapper(final @NotNull Cache<K, V> delegate, final @NotNull I

@Override
public V get(final K key) {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "get");
if (span == null) {
return delegate.get(key);
}
Expand All @@ -70,7 +71,7 @@ public V get(final K key) {

@Override
public Map<K, V> getAll(final Set<? extends K> keys) {
final ISpan span = startSpanForKeys("cache.get", keys);
final ISpan span = startSpanForKeys("cache.get", keys, "getAll");
if (span == null) {
return delegate.getAll(keys);
}
Expand All @@ -97,7 +98,7 @@ public boolean containsKey(final K key) {

@Override
public void put(final K key, final V value) {
final ISpan span = startSpan("cache.put", key);
final ISpan span = startSpan("cache.put", key, "put");
if (span == null) {
delegate.put(key, value);
return;
Expand All @@ -116,7 +117,7 @@ public void put(final K key, final V value) {

@Override
public V getAndPut(final K key, final V value) {
final ISpan span = startSpan("cache.put", key);
final ISpan span = startSpan("cache.put", key, "getAndPut");
if (span == null) {
return delegate.getAndPut(key, value);
}
Expand All @@ -135,7 +136,7 @@ public V getAndPut(final K key, final V value) {

@Override
public void putAll(final Map<? extends K, ? extends V> map) {
final ISpan span = startSpanForKeys("cache.put", map.keySet());
final ISpan span = startSpanForKeys("cache.put", map.keySet(), "putAll");
if (span == null) {
delegate.putAll(map);
return;
Expand Down Expand Up @@ -181,7 +182,7 @@ public V getAndReplace(final K key, final V value) {

@Override
public boolean remove(final K key) {
final ISpan span = startSpan("cache.remove", key);
final ISpan span = startSpan("cache.remove", key, "remove");
if (span == null) {
return delegate.remove(key);
}
Expand All @@ -200,7 +201,7 @@ public boolean remove(final K key) {

@Override
public boolean remove(final K key, final V oldValue) {
final ISpan span = startSpan("cache.remove", key);
final ISpan span = startSpan("cache.remove", key, "remove");
if (span == null) {
return delegate.remove(key, oldValue);
}
Expand All @@ -219,7 +220,7 @@ public boolean remove(final K key, final V oldValue) {

@Override
public V getAndRemove(final K key) {
final ISpan span = startSpan("cache.remove", key);
final ISpan span = startSpan("cache.remove", key, "getAndRemove");
if (span == null) {
return delegate.getAndRemove(key);
}
Expand All @@ -238,7 +239,7 @@ public V getAndRemove(final K key) {

@Override
public void removeAll(final Set<? extends K> keys) {
final ISpan span = startSpanForKeys("cache.remove", keys);
final ISpan span = startSpanForKeys("cache.remove", keys, "removeAll");
if (span == null) {
delegate.removeAll(keys);
return;
Expand All @@ -257,7 +258,7 @@ public void removeAll(final Set<? extends K> keys) {

@Override
public void removeAll() {
final ISpan span = startSpan("cache.flush", null);
final ISpan span = startSpan("cache.flush", null, "removeAll");
if (span == null) {
delegate.removeAll();
return;
Expand All @@ -278,7 +279,7 @@ public void removeAll() {

@Override
public void clear() {
final ISpan span = startSpan("cache.flush", null);
final ISpan span = startSpan("cache.flush", null, "clear");
if (span == null) {
delegate.clear();
return;
Expand Down Expand Up @@ -306,7 +307,7 @@ public void close() {
public <T> T invoke(
final K key, final EntryProcessor<K, V, T> entryProcessor, final Object... arguments)
throws EntryProcessorException {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "invoke");
if (span == null) {
return delegate.invoke(key, entryProcessor, arguments);
}
Expand All @@ -328,7 +329,7 @@ public <T> Map<K, EntryProcessorResult<T>> invokeAll(
final Set<? extends K> keys,
final EntryProcessor<K, V, T> entryProcessor,
final Object... arguments) {
final ISpan span = startSpanForKeys("cache.get", keys);
final ISpan span = startSpanForKeys("cache.get", keys, "invokeAll");
if (span == null) {
return delegate.invokeAll(keys, entryProcessor, arguments);
}
Expand Down Expand Up @@ -400,7 +401,10 @@ public Iterator<Entry<K, V>> iterator() {

// -- span helpers --

private @Nullable ISpan startSpan(final @NotNull String operation, final @Nullable Object key) {
private @Nullable ISpan startSpan(
final @NotNull String operation,
final @Nullable Object key,
final @NotNull String operationName) {
if (!scopes.getOptions().isEnableCacheTracing()) {
return null;
}
Expand All @@ -420,11 +424,14 @@ public Iterator<Entry<K, V>> iterator() {
if (keyString != null) {
span.setData(SpanDataConvention.CACHE_KEY_KEY, Arrays.asList(keyString));
}
span.setData(OPERATION_ATTRIBUTE, operationName);
return span;
}

private @Nullable ISpan startSpanForKeys(
final @NotNull String operation, final @NotNull Set<?> keys) {
final @NotNull String operation,
final @NotNull Set<?> keys,
final @NotNull String operationName) {
if (!scopes.getOptions().isEnableCacheTracing()) {
return null;
}
Expand All @@ -443,6 +450,7 @@ public Iterator<Entry<K, V>> iterator() {
span.setData(
SpanDataConvention.CACHE_KEY_KEY,
keys.stream().map(String::valueOf).collect(Collectors.toList()));
span.setData(OPERATION_ATTRIBUTE, operationName);
return span;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class SentryJCacheWrapperTest {
assertEquals(true, span.getData(SpanDataConvention.CACHE_HIT_KEY))
assertEquals(listOf("myKey"), span.getData(SpanDataConvention.CACHE_KEY_KEY))
assertEquals("auto.cache.jcache", span.spanContext.origin)
assertEquals("get", span.getData("db.operation.name"))
}

@Test
Expand Down Expand Up @@ -98,6 +99,7 @@ class SentryJCacheWrapperTest {
assertEquals(true, span.getData(SpanDataConvention.CACHE_HIT_KEY))
val cacheKeys = span.getData(SpanDataConvention.CACHE_KEY_KEY) as List<*>
assertTrue(cacheKeys.containsAll(listOf("k1", "k2")))
assertEquals("getAll", span.getData("db.operation.name"))
}

@Test
Expand Down Expand Up @@ -127,6 +129,7 @@ class SentryJCacheWrapperTest {
assertEquals("cache.put", span.operation)
assertEquals(SpanStatus.OK, span.status)
assertEquals(listOf("myKey"), span.getData(SpanDataConvention.CACHE_KEY_KEY))
assertEquals("put", span.getData("db.operation.name"))
}

// -- getAndPut --
Expand All @@ -142,6 +145,7 @@ class SentryJCacheWrapperTest {
assertEquals("oldValue", result)
assertEquals(1, tx.spans.size)
assertEquals("cache.put", tx.spans.first().operation)
assertEquals("getAndPut", tx.spans.first().getData("db.operation.name"))
}

// -- putAll --
Expand All @@ -161,6 +165,7 @@ class SentryJCacheWrapperTest {
assertEquals("testCache", span.description)
val cacheKeys = span.getData(SpanDataConvention.CACHE_KEY_KEY) as List<*>
assertTrue(cacheKeys.containsAll(listOf("k1", "k2")))
assertEquals("putAll", span.getData("db.operation.name"))
}

// -- putIfAbsent --
Expand Down Expand Up @@ -236,6 +241,7 @@ class SentryJCacheWrapperTest {
val span = tx.spans.first()
assertEquals("cache.remove", span.operation)
assertEquals(SpanStatus.OK, span.status)
assertEquals("remove", span.getData("db.operation.name"))
}

// -- remove(K, V) --
Expand All @@ -251,6 +257,7 @@ class SentryJCacheWrapperTest {
assertTrue(result)
assertEquals(1, tx.spans.size)
assertEquals("cache.remove", tx.spans.first().operation)
assertEquals("remove", tx.spans.first().getData("db.operation.name"))
}

// -- getAndRemove --
Expand All @@ -266,6 +273,7 @@ class SentryJCacheWrapperTest {
assertEquals("value", result)
assertEquals(1, tx.spans.size)
assertEquals("cache.remove", tx.spans.first().operation)
assertEquals("getAndRemove", tx.spans.first().getData("db.operation.name"))
}

// -- removeAll(Set) --
Expand All @@ -283,6 +291,7 @@ class SentryJCacheWrapperTest {
val span = tx.spans.first()
assertEquals("cache.remove", span.operation)
assertEquals("testCache", span.description)
assertEquals("removeAll", span.getData("db.operation.name"))
}

// -- removeAll() --
Expand All @@ -297,6 +306,7 @@ class SentryJCacheWrapperTest {
verify(delegate).removeAll()
assertEquals(1, tx.spans.size)
assertEquals("cache.flush", tx.spans.first().operation)
assertEquals("removeAll", tx.spans.first().getData("db.operation.name"))
}

// -- clear --
Expand All @@ -314,6 +324,7 @@ class SentryJCacheWrapperTest {
assertEquals("cache.flush", span.operation)
assertEquals(SpanStatus.OK, span.status)
assertNull(span.getData(SpanDataConvention.CACHE_KEY_KEY))
assertEquals("clear", span.getData("db.operation.name"))
}

// -- invoke --
Expand All @@ -330,6 +341,7 @@ class SentryJCacheWrapperTest {
assertEquals("result", result)
assertEquals(1, tx.spans.size)
assertEquals("cache.get", tx.spans.first().operation)
assertEquals("invoke", tx.spans.first().getData("db.operation.name"))
}

// -- invokeAll --
Expand All @@ -348,6 +360,7 @@ class SentryJCacheWrapperTest {
assertEquals(resultMap, result)
assertEquals(1, tx.spans.size)
assertEquals("cache.get", tx.spans.first().operation)
assertEquals("invokeAll", tx.spans.first().getData("db.operation.name"))
}

// -- passthrough operations --
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
public final class SentryCacheWrapper implements Cache {

private static final String TRACE_ORIGIN = "auto.cache.spring";
private static final String OPERATION_ATTRIBUTE = "db.operation.name";

private final @NotNull Cache delegate;
private final @NotNull IScopes scopes;
Expand All @@ -41,7 +42,7 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes

@Override
public @Nullable ValueWrapper get(final @NotNull Object key) {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "get");
if (span == null) {
return delegate.get(key);
}
Expand All @@ -61,7 +62,7 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes

@Override
public @Nullable <T> T get(final @NotNull Object key, final @Nullable Class<T> type) {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "get");
if (span == null) {
return delegate.get(key, type);
}
Expand All @@ -81,7 +82,7 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes

@Override
public @Nullable <T> T get(final @NotNull Object key, final @NotNull Callable<T> valueLoader) {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "get");
if (span == null) {
return delegate.get(key, valueLoader);
}
Expand All @@ -108,7 +109,7 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes

@Override
public @Nullable CompletableFuture<?> retrieve(final @NotNull Object key) {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "retrieve");
if (span == null) {
return delegate.retrieve(key);
}
Expand Down Expand Up @@ -143,7 +144,7 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes
@Override
public <T> CompletableFuture<T> retrieve(
final @NotNull Object key, final @NotNull Supplier<CompletableFuture<T>> valueLoader) {
final ISpan span = startSpan("cache.get", key);
final ISpan span = startSpan("cache.get", key, "retrieve");
if (span == null) {
return delegate.retrieve(key, valueLoader);
}
Expand Down Expand Up @@ -178,7 +179,7 @@ public <T> CompletableFuture<T> retrieve(

@Override
public void put(final @NotNull Object key, final @Nullable Object value) {
final ISpan span = startSpan("cache.put", key);
final ISpan span = startSpan("cache.put", key, "put");
if (span == null) {
delegate.put(key, value);
return;
Expand Down Expand Up @@ -207,7 +208,7 @@ public void put(final @NotNull Object key, final @Nullable Object value) {

@Override
public void evict(final @NotNull Object key) {
final ISpan span = startSpan("cache.remove", key);
final ISpan span = startSpan("cache.remove", key, "evict");
if (span == null) {
delegate.evict(key);
return;
Expand All @@ -226,7 +227,7 @@ public void evict(final @NotNull Object key) {

@Override
public boolean evictIfPresent(final @NotNull Object key) {
final ISpan span = startSpan("cache.remove", key);
final ISpan span = startSpan("cache.remove", key, "evictIfPresent");
if (span == null) {
return delegate.evictIfPresent(key);
}
Expand All @@ -245,7 +246,7 @@ public boolean evictIfPresent(final @NotNull Object key) {

@Override
public void clear() {
final ISpan span = startSpan("cache.flush", null);
final ISpan span = startSpan("cache.flush", null, "clear");
if (span == null) {
delegate.clear();
return;
Expand All @@ -264,7 +265,7 @@ public void clear() {

@Override
public boolean invalidate() {
final ISpan span = startSpan("cache.flush", null);
final ISpan span = startSpan("cache.flush", null, "invalidate");
if (span == null) {
return delegate.invalidate();
}
Expand All @@ -281,7 +282,10 @@ public boolean invalidate() {
}
}

private @Nullable ISpan startSpan(final @NotNull String operation, final @Nullable Object key) {
private @Nullable ISpan startSpan(
final @NotNull String operation,
final @Nullable Object key,
final @NotNull String operationName) {
if (!scopes.getOptions().isEnableCacheTracing()) {
return null;
}
Expand All @@ -301,6 +305,7 @@ public boolean invalidate() {
if (keyString != null) {
span.setData(SpanDataConvention.CACHE_KEY_KEY, Arrays.asList(keyString));
}
span.setData(OPERATION_ATTRIBUTE, operationName);
return span;
}
}
Loading
Loading