From d7f8967ff6ca811e18e993e864a4786e58b447f3 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Fri, 5 Mar 2021 17:57:23 +0900 Subject: [PATCH] Add a caching API based on caffeine for use from instrumentation, not just javaagent (#2477) * Add caching API * Finish * javadoc * Extract dep * git add * Drift * Spotbugs * checkstyle * Fix package * Test Caffeine patch --- gradle/dependencies.gradle | 5 + gradle/spotbugs-exclude.xml | 1 + .../instrumentation-api-caching.gradle | 29 +++++ .../caffeine/cache/CacheImplementations.java | 25 ++++ .../instrumentation/api/caching/Cache.java | 23 ++++ .../api/caching/CacheBuilder.java | 46 +++++++ .../api/caching/CaffeineCache.java | 33 +++++ .../src/test/README.md | 1 + .../instrumentation-api.gradle | 3 + .../api/caching/CacheTest.java | 120 ++++++++++++++++++ .../instrumentation/api/BoundedCache.java | 54 -------- .../api/db/SqlStatementSanitizer.java | 8 +- .../instrumentation/api/BoundedCacheTest.java | 104 --------------- .../javaagent-bootstrap-tests.gradle | 10 ++ .../api/caching/PatchCaffeineTest.java | 38 ++++++ .../javaagent-bootstrap.gradle | 12 ++ .../caffeine/cache/BoundedLocalCache.java | 64 ++++++++++ .../javaagent/tooling/AgentInstaller.java | 2 - .../javaagent/tooling/AgentTooling.java | 18 --- .../javaagent/tooling/GuavaBoundedCache.java | 29 ----- .../tooling/GuavaBoundedCacheTest.groovy | 30 ----- settings.gradle | 2 + 22 files changed, 416 insertions(+), 241 deletions(-) create mode 100644 instrumentation-api-caching/instrumentation-api-caching.gradle create mode 100644 instrumentation-api-caching/src/main/java/com/github/benmanes/caffeine/cache/CacheImplementations.java create mode 100644 instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/Cache.java create mode 100644 instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CacheBuilder.java create mode 100644 instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CaffeineCache.java create mode 100644 instrumentation-api-caching/src/test/README.md create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/caching/CacheTest.java delete mode 100644 javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCache.java delete mode 100644 javaagent-api/src/test/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCacheTest.java create mode 100644 javaagent-bootstrap-tests/javaagent-bootstrap-tests.gradle create mode 100644 javaagent-bootstrap-tests/src/test/java/io/opentelemetry/instrumentation/api/caching/PatchCaffeineTest.java create mode 100644 javaagent-bootstrap/src/patch/java/io/opentelemetry/instrumentation/internal/shaded/caffeine/cache/BoundedLocalCache.java delete mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/GuavaBoundedCache.java delete mode 100644 javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/GuavaBoundedCacheTest.groovy diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 217eeeeac1..48e18ffb5d 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -30,6 +30,9 @@ ext { systemLambda : "1.1.0", prometheus : "0.9.0", assertj : '3.19.0', + awaitility : '4.0.3', + // Caffeine 2.x to support Java 8+. 3.x is 11+. + caffeine : '2.9.0', testcontainers : '1.15.2' ] @@ -73,6 +76,7 @@ ext { dependencies.create(group: 'io.prometheus', name: 'simpleclient', version: "${versions.prometheus}"), dependencies.create(group: 'io.prometheus', name: 'simpleclient_httpserver', version: "${versions.prometheus}"), ], + caffeine : "com.github.ben-manes.caffeine:caffeine:${versions.caffeine}", // Testing @@ -97,5 +101,6 @@ ext { coroutines : dependencies.create(group: 'org.jetbrains.kotlinx', name: 'kotlinx-coroutines-core', version: "${versions.coroutines}"), junitApi : "org.junit.jupiter:junit-jupiter-api:${versions.junit5}", assertj : "org.assertj:assertj-core:${versions.assertj}", + awaitility : "org.awaitility:awaitility:${versions.awaitility}" ] } diff --git a/gradle/spotbugs-exclude.xml b/gradle/spotbugs-exclude.xml index b906db7f66..7fe1f97325 100644 --- a/gradle/spotbugs-exclude.xml +++ b/gradle/spotbugs-exclude.xml @@ -10,6 +10,7 @@ + diff --git a/instrumentation-api-caching/instrumentation-api-caching.gradle b/instrumentation-api-caching/instrumentation-api-caching.gradle new file mode 100644 index 0000000000..be008b757c --- /dev/null +++ b/instrumentation-api-caching/instrumentation-api-caching.gradle @@ -0,0 +1,29 @@ +plugins { + id "com.github.johnrengelman.shadow" +} + +group = 'io.opentelemetry.instrumentation' + +apply from: "$rootDir/gradle/java.gradle" +apply from: "$rootDir/gradle/publish.gradle" + +dependencies { + implementation(deps.caffeine) { + exclude group: 'com.google.errorprone', module: 'error_prone_annotations' + exclude group: 'org.checkerframework', module: 'checker-qual' + } +} + +shadowJar { + archiveClassifier.set("") + + relocate "com.github.benmanes.caffeine", "io.opentelemetry.instrumentation.internal.shaded.caffeine" + + minimize() +} + +jar { + enabled = false + + dependsOn shadowJar +} diff --git a/instrumentation-api-caching/src/main/java/com/github/benmanes/caffeine/cache/CacheImplementations.java b/instrumentation-api-caching/src/main/java/com/github/benmanes/caffeine/cache/CacheImplementations.java new file mode 100644 index 0000000000..ce6fa7eb27 --- /dev/null +++ b/instrumentation-api-caching/src/main/java/com/github/benmanes/caffeine/cache/CacheImplementations.java @@ -0,0 +1,25 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.github.benmanes.caffeine.cache; + +// Caffeine uses reflection to load cache implementations based on parameters specified by a user. +// We use gradle-shadow-plugin to minimize the dependency on Caffeine, but it does not allow +// specifying classes to keep, only artifacts. It's a relatively simple workaround for us to use +// this non-public class to create a static link to the required implementations we use. +final class CacheImplementations { + + // Each type of cache has a cache implementation and a node implementation. + + // Strong keys, strong values, maximum size + SSMS ssms; // cache + PSMS psms; // node + + // Weak keys, strong values, maximum size + WSMS wsms; // cache + FSMS fsms; // node + + private CacheImplementations() {} +} diff --git a/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/Cache.java b/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/Cache.java new file mode 100644 index 0000000000..fbcb27a01c --- /dev/null +++ b/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/Cache.java @@ -0,0 +1,23 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.caching; + +import java.util.function.Function; + +/** A cache from keys to values. */ +public interface Cache { + + /** Returns a new {@link CacheBuilder} to configure a {@link Cache}. */ + static CacheBuilder newBuilder() { + return new CacheBuilder(); + } + + /** + * Returns the cached value associated with the provided {@code key}. If no value is cached yet, + * computes the value using {@code mappingFunction}, stores the result, and returns it. + */ + V computeIfAbsent(K key, Function mappingFunction); +} diff --git a/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CacheBuilder.java b/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CacheBuilder.java new file mode 100644 index 0000000000..191579e9e0 --- /dev/null +++ b/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CacheBuilder.java @@ -0,0 +1,46 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.caching; + +import com.github.benmanes.caffeine.cache.Caffeine; +import java.util.concurrent.Executor; + +/** A builder of {@link Cache}. */ +public final class CacheBuilder { + + private final Caffeine caffeine = Caffeine.newBuilder(); + + /** Sets the maximum size of the cache. */ + public CacheBuilder setMaximumSize(long maximumSize) { + caffeine.maximumSize(maximumSize); + return this; + } + + /** + * Sets that keys should be referenced weakly. If used, keys will use identity comparison, not + * {@link Object#equals(Object)}. + */ + public CacheBuilder setWeakKeys() { + caffeine.weakKeys(); + return this; + } + + // Visible for testing + CacheBuilder setExecutor(Executor executor) { + caffeine.executor(executor); + return this; + } + + /** Returns a new {@link Cache} with the settings of this {@link CacheBuilder}. */ + public Cache build() { + @SuppressWarnings("unchecked") + com.github.benmanes.caffeine.cache.Cache delegate = + (com.github.benmanes.caffeine.cache.Cache) caffeine.build(); + return new CaffeineCache(delegate); + } + + CacheBuilder() {} +} diff --git a/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CaffeineCache.java b/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CaffeineCache.java new file mode 100644 index 0000000000..2fca85c156 --- /dev/null +++ b/instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CaffeineCache.java @@ -0,0 +1,33 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.caching; + +import java.util.Set; +import java.util.function.Function; + +final class CaffeineCache implements Cache { + + private final com.github.benmanes.caffeine.cache.Cache delegate; + + CaffeineCache(com.github.benmanes.caffeine.cache.Cache delegate) { + this.delegate = delegate; + } + + @Override + public V computeIfAbsent(K key, Function mappingFunction) { + return delegate.get(key, mappingFunction); + } + + // Visible for testing + Set keySet() { + return delegate.asMap().keySet(); + } + + // Visible for testing + void cleanup() { + delegate.cleanUp(); + } +} diff --git a/instrumentation-api-caching/src/test/README.md b/instrumentation-api-caching/src/test/README.md new file mode 100644 index 0000000000..865db385e2 --- /dev/null +++ b/instrumentation-api-caching/src/test/README.md @@ -0,0 +1 @@ +Tests for this module are in the instrumentation-api project to verify against the shaded artifact. diff --git a/instrumentation-api/instrumentation-api.gradle b/instrumentation-api/instrumentation-api.gradle index c0d0c4177b..e31372aade 100644 --- a/instrumentation-api/instrumentation-api.gradle +++ b/instrumentation-api/instrumentation-api.gradle @@ -4,6 +4,8 @@ apply from: "$rootDir/gradle/java.gradle" apply from: "$rootDir/gradle/publish.gradle" dependencies { + api project(":instrumentation-api-caching") + api deps.opentelemetryApi api deps.opentelemetryContext api deps.opentelemetrySemConv @@ -16,4 +18,5 @@ dependencies { testImplementation project(':testing-common') testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0' testImplementation deps.assertj + testImplementation deps.awaitility } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/caching/CacheTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/caching/CacheTest.java new file mode 100644 index 0000000000..5173831dec --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/caching/CacheTest.java @@ -0,0 +1,120 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.caching; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +class CacheTest { + + @Nested + class StrongKeys { + @Test + void unbounded() { + Cache cache = Cache.newBuilder().build(); + + CaffeineCache caffeineCache = ((CaffeineCache) cache); + assertThat(cache.computeIfAbsent("cat", unused -> "meow")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent("cat", unused -> "bark")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent("dog", unused -> "bark")).isEqualTo("bark"); + assertThat(caffeineCache.keySet()).hasSize(2); + assertThat(cache.computeIfAbsent("cat", unused -> "meow")).isEqualTo("meow"); + } + + @Test + void bounded() { + Cache cache = Cache.newBuilder().setMaximumSize(1).build(); + + CaffeineCache caffeineCache = ((CaffeineCache) cache); + assertThat(cache.computeIfAbsent("cat", unused -> "meow")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent("cat", unused -> "bark")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent("dog", unused -> "bark")).isEqualTo("bark"); + caffeineCache.cleanup(); + assertThat(caffeineCache.keySet()).hasSize(1); + assertThat(cache.computeIfAbsent("cat", unused -> "purr")).isEqualTo("purr"); + } + } + + @Nested + class WeakKeys { + @Test + void unbounded() { + Cache cache = Cache.newBuilder().setWeakKeys().build(); + + CaffeineCache caffeineCache = ((CaffeineCache) cache); + String cat = new String("cat"); + String dog = new String("dog"); + assertThat(cache.computeIfAbsent(cat, unused -> "meow")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent(cat, unused -> "bark")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent(dog, unused -> "bark")).isEqualTo("bark"); + assertThat(caffeineCache.keySet()).hasSize(2); + assertThat(cache.computeIfAbsent(cat, unused -> "meow")).isEqualTo("meow"); + + cat = null; + System.gc(); + // Wait for GC to be reflected. + await() + .untilAsserted( + () -> { + caffeineCache.cleanup(); + assertThat(caffeineCache.keySet()).hasSize(1); + }); + assertThat(cache.computeIfAbsent(dog, unused -> "bark")).isEqualTo("bark"); + dog = null; + System.gc(); + // Wait for GC to be reflected. + await() + .untilAsserted( + () -> { + caffeineCache.cleanup(); + assertThat(caffeineCache.keySet()).isEmpty(); + }); + } + + @Test + void bounded() throws Exception { + Cache cache = Cache.newBuilder().setWeakKeys().setMaximumSize(1).build(); + + CaffeineCache caffeineCache = ((CaffeineCache) cache); + + String cat = new String("cat"); + String dog = new String("dog"); + assertThat(cache.computeIfAbsent(cat, unused -> "meow")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent(cat, unused -> "bark")).isEqualTo("meow"); + assertThat(caffeineCache.keySet()).hasSize(1); + + assertThat(cache.computeIfAbsent(dog, unused -> "bark")).isEqualTo("bark"); + caffeineCache.cleanup(); + assertThat(caffeineCache.keySet()).hasSize(1); + dog = null; + System.gc(); + // Wait for GC to be reflected. + await() + .untilAsserted( + () -> { + caffeineCache.cleanup(); + assertThat(caffeineCache.keySet()).isEmpty(); + }); + } + } +} diff --git a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCache.java b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCache.java deleted file mode 100644 index e2b18b4faa..0000000000 --- a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCache.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.api; - -import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Function; - -/** An LRU cache that has a fixed maximum size. */ -public interface BoundedCache { - - V get(K key, Function mappingFunction); - - static BoundedCache build(long maxSize) { - return Provider.get().build(maxSize); - } - - interface Builder { - BoundedCache build(long maxSize); - } - - class Provider { - /* - The default implementation just delegates to the lookup function and should not normally be used. - It will be replaced at startup by the AgentInstaller. - */ - private static final Builder NEVER_ACTUALLY_CACHES = - new Builder() { - @Override - public BoundedCache build(long maxSize) { - return (key, mappingFunction) -> mappingFunction.apply(key); - } - }; - private static final AtomicReference builderRef = - new AtomicReference<>(NEVER_ACTUALLY_CACHES); - - private Provider() {} - - public static boolean registerIfAbsent(Builder builder) { - return builderRef.compareAndSet(NEVER_ACTUALLY_CACHES, builder); - } - - // Method exists for testing only - static void reset() { - builderRef.set(NEVER_ACTUALLY_CACHES); - } - - public static Builder get() { - return builderRef.get(); - } - } -} diff --git a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/db/SqlStatementSanitizer.java b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/db/SqlStatementSanitizer.java index 715d8e2bee..b84d431af4 100644 --- a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/db/SqlStatementSanitizer.java +++ b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/db/SqlStatementSanitizer.java @@ -7,7 +7,7 @@ package io.opentelemetry.javaagent.instrumentation.api.db; import static io.opentelemetry.javaagent.instrumentation.api.db.StatementSanitizationConfig.isStatementSanitizationEnabled; -import io.opentelemetry.javaagent.instrumentation.api.BoundedCache; +import io.opentelemetry.instrumentation.api.caching.Cache; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -18,14 +18,14 @@ import org.slf4j.LoggerFactory; public final class SqlStatementSanitizer { private static final Logger log = LoggerFactory.getLogger(SqlStatementSanitizer.class); - private static final BoundedCache sqlToStatementInfoCache = - BoundedCache.build(1000); + private static final Cache sqlToStatementInfoCache = + Cache.newBuilder().setMaximumSize(1000).build(); public static SqlStatementInfo sanitize(String statement) { if (!isStatementSanitizationEnabled() || statement == null) { return new SqlStatementInfo(statement, null, null); } - return sqlToStatementInfoCache.get( + return sqlToStatementInfoCache.computeIfAbsent( statement, k -> { log.trace("SQL statement cache miss"); diff --git a/javaagent-api/src/test/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCacheTest.java b/javaagent-api/src/test/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCacheTest.java deleted file mode 100644 index 21e8a7bc72..0000000000 --- a/javaagent-api/src/test/java/io/opentelemetry/javaagent/instrumentation/api/BoundedCacheTest.java +++ /dev/null @@ -1,104 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.api; - -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Function; -import org.jetbrains.annotations.NotNull; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Order; -import org.junit.jupiter.api.Test; - -class BoundedCacheTest { - private AtomicInteger cacheMisses; - - @BeforeEach - void setup() { - cacheMisses = new AtomicInteger(0); - } - - @AfterEach - void reset() { - BoundedCache.Provider.reset(); - } - - String mockLookupFunction(String s) { - cacheMisses.incrementAndGet(); - return s.toUpperCase(); - } - - @Test - @Order(1) - void testCanUseBeforeRegister() { - BoundedCache cache = BoundedCache.build(3); - String result1 = cache.get("foo", this::mockLookupFunction); - String result2 = cache.get("bAr", this::mockLookupFunction); - assertThat(result1).isEqualTo("FOO"); - assertThat(result2).isEqualTo("BAR"); - assertThat(cacheMisses.get()).isEqualTo(2); - } - - @Test - @Order(2) - void testRegisterUsesInstance() { - Map map = new HashMap<>(); - BoundedCache.Builder builder = buildMapBackedBuilder(map); - BoundedCache.Provider.registerIfAbsent(builder); - BoundedCache cache = BoundedCache.build(3); - String result1 = cache.get("foo", this::mockLookupFunction); - String result2 = cache.get("fOo", this::mockLookupFunction); - String result3 = cache.get("foo", this::mockLookupFunction); - assertThat(result1).isEqualTo("FOO"); - assertThat(result2).isEqualTo("FOO"); - assertThat(result3).isEqualTo("FOO"); - assertThat(cacheMisses.get()).isEqualTo(2); // once for "foo" once for "fOo" - assertThat(map.size()).isEqualTo(2); - assertThat(map).containsKey("foo"); - assertThat(map).containsKey("fOo"); - } - - @Test - @Order(3) - void testRegisterMultipleFails() { - Map map = new HashMap<>(); - BoundedCache.Builder builder = buildMapBackedBuilder(map); - assertThat(BoundedCache.Provider.registerIfAbsent(builder)).isTrue(); - assertThat(BoundedCache.Provider.registerIfAbsent(builder)).isFalse(); - } - - @NotNull - private BoundedCache.Builder buildMapBackedBuilder(Map map) { - return new BoundedCache.Builder() { - @Override - public BoundedCache build(long maxSize) { - return new MapBackedCache((Map) map); - } - }; - } - - private static class MapBackedCache implements BoundedCache { - private final Map map; - - public MapBackedCache(Map map) { - this.map = map; - } - - @Override - public V get(K key, Function mappingFunction) { - V v = map.get(key); - if (v == null) { - v = mappingFunction.apply(key); - map.put(key, v); - } - return v; - } - } -} diff --git a/javaagent-bootstrap-tests/javaagent-bootstrap-tests.gradle b/javaagent-bootstrap-tests/javaagent-bootstrap-tests.gradle new file mode 100644 index 0000000000..95baaade30 --- /dev/null +++ b/javaagent-bootstrap-tests/javaagent-bootstrap-tests.gradle @@ -0,0 +1,10 @@ +apply from: "$rootDir/gradle/java.gradle" + +dependencies { + // For testing javaagent-bootstrap's Caffeine patch, we need to compile against our cache API + // but make sure to run against javaagent-bootstrap + testCompileOnly project(':instrumentation-api-caching') + testRuntimeOnly project(":javaagent-bootstrap") + + testImplementation deps.assertj +} \ No newline at end of file diff --git a/javaagent-bootstrap-tests/src/test/java/io/opentelemetry/instrumentation/api/caching/PatchCaffeineTest.java b/javaagent-bootstrap-tests/src/test/java/io/opentelemetry/instrumentation/api/caching/PatchCaffeineTest.java new file mode 100644 index 0000000000..ebadddb0fd --- /dev/null +++ b/javaagent-bootstrap-tests/src/test/java/io/opentelemetry/instrumentation/api/caching/PatchCaffeineTest.java @@ -0,0 +1,38 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.caching; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.ForkJoinTask; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; + +class PatchCaffeineTest { + + @Test + void cleanupNotForkJoinTask() { + AtomicReference errorRef = new AtomicReference<>(); + Cache cache = + Cache.newBuilder() + .setExecutor( + task -> { + try { + assertThat(task).isNotInstanceOf(ForkJoinTask.class); + } catch (AssertionError e) { + errorRef.set(e); + } + }) + .setMaximumSize(1) + .build(); + assertThat(cache.computeIfAbsent("cat", unused -> "meow")).isEqualTo("meow"); + assertThat(cache.computeIfAbsent("dog", unused -> "bark")).isEqualTo("bark"); + AssertionError error = errorRef.get(); + if (error != null) { + throw error; + } + } +} diff --git a/javaagent-bootstrap/javaagent-bootstrap.gradle b/javaagent-bootstrap/javaagent-bootstrap.gradle index d2d5cb28f2..ce2844260b 100644 --- a/javaagent-bootstrap/javaagent-bootstrap.gradle +++ b/javaagent-bootstrap/javaagent-bootstrap.gradle @@ -7,6 +7,18 @@ apply from: "$rootDir/gradle/publish.gradle" minimumBranchCoverage = 0.0 minimumInstructionCoverage = 0.0 +// patch inner class from Caffeine to avoid ForkJoinTask from being loaded too early +sourceSets { + patch { + java {} + } +} +jar { + from(sourceSets.patch.output) { + include 'io/opentelemetry/instrumentation/internal/shaded/caffeine/cache/BoundedLocalCache$PerformCleanupTask.class' + } +} + configurations { // classpath used by the instrumentation muzzle plugin instrumentationMuzzle { diff --git a/javaagent-bootstrap/src/patch/java/io/opentelemetry/instrumentation/internal/shaded/caffeine/cache/BoundedLocalCache.java b/javaagent-bootstrap/src/patch/java/io/opentelemetry/instrumentation/internal/shaded/caffeine/cache/BoundedLocalCache.java new file mode 100644 index 0000000000..0409dcb31d --- /dev/null +++ b/javaagent-bootstrap/src/patch/java/io/opentelemetry/instrumentation/internal/shaded/caffeine/cache/BoundedLocalCache.java @@ -0,0 +1,64 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +// Includes work from: +/* + * Copyright 2017 Datadog, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +/* + * Copyright 2014 Ben Manes. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.opentelemetry.instrumentation.internal.shaded.caffeine.cache; + +import java.lang.ref.WeakReference; + +/** skeleton outer class just for compilation purposes, not included in the final patch. */ +abstract class BoundedLocalCache { + abstract void performCleanUp(Runnable task); + + /** patched to not extend ForkJoinTask as we don't want that class loaded too early. */ + static final class PerformCleanupTask implements Runnable { + private static final long serialVersionUID = 1L; + + final WeakReference> reference; + + PerformCleanupTask(BoundedLocalCache cache) { + reference = new WeakReference>(cache); + } + + @Override + public void run() { + BoundedLocalCache cache = reference.get(); + if (cache != null) { + cache.performCleanUp(/* ignored */ null); + } + } + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java index 7e5698bc4c..53a2572df2 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java @@ -74,8 +74,6 @@ public class AgentInstaller { BootstrapPackagePrefixesHolder.setBoostrapPackagePrefixes(loadBootstrapPackagePrefixes()); // WeakMap is used by other classes below, so we need to register the provider first. AgentTooling.registerWeakMapProvider(); - // Instrumentation can use a bounded cache, so register here. - AgentTooling.registerBoundedCacheProvider(); // this needs to be done as early as possible - before the first Config.get() call ConfigInitializer.initialize(); } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentTooling.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentTooling.java index b9d67c4a10..7584975e4e 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentTooling.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentTooling.java @@ -5,11 +5,8 @@ package io.opentelemetry.javaagent.tooling; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import io.opentelemetry.javaagent.bootstrap.WeakCache; import io.opentelemetry.javaagent.bootstrap.WeakCache.Provider; -import io.opentelemetry.javaagent.instrumentation.api.BoundedCache; import io.opentelemetry.javaagent.instrumentation.api.WeakMap; import io.opentelemetry.javaagent.tooling.bytebuddy.AgentCachingPoolStrategy; import io.opentelemetry.javaagent.tooling.bytebuddy.AgentLocationStrategy; @@ -36,21 +33,6 @@ public class AgentTooling { } } - /** - * Instances of BoundCache are backed by a guava instance that lives in the agent classloader and - * is bridged to user/instrumentation classloader through the BoundedCache.Provider interface. - */ - static void registerBoundedCacheProvider() { - BoundedCache.Provider.registerIfAbsent( - new BoundedCache.Builder() { - @Override - public BoundedCache build(long maxSize) { - Cache cache = CacheBuilder.newBuilder().maximumSize(maxSize).build(); - return new GuavaBoundedCache<>(cache); - } - }); - } - private static Provider loadWeakCacheProvider() { Iterator providers = ServiceLoader.load(Provider.class, AgentInstaller.class.getClassLoader()).iterator(); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/GuavaBoundedCache.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/GuavaBoundedCache.java deleted file mode 100644 index f43c486e40..0000000000 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/GuavaBoundedCache.java +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.tooling; - -import com.google.common.cache.Cache; -import io.opentelemetry.javaagent.instrumentation.api.BoundedCache; -import java.util.concurrent.ExecutionException; -import java.util.function.Function; - -class GuavaBoundedCache implements BoundedCache { - - private final Cache delegate; - - public GuavaBoundedCache(Cache delegate) { - this.delegate = delegate; - } - - @Override - public V get(K key, Function mappingFunction) { - try { - return delegate.get(key, () -> mappingFunction.apply(key)); - } catch (ExecutionException e) { - throw new IllegalStateException("Unexpected cache exception", e); - } - } -} diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/GuavaBoundedCacheTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/GuavaBoundedCacheTest.groovy deleted file mode 100644 index c11152a24f..0000000000 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/GuavaBoundedCacheTest.groovy +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.tooling - -import com.google.common.cache.CacheBuilder -import spock.lang.Specification - -class GuavaBoundedCacheTest extends Specification { - - def "test cache"() { - - when: - - def delegate = CacheBuilder.newBuilder().maximumSize(3).build() - def cache = new GuavaBoundedCache<>(delegate) - def fn = { x -> x.toUpperCase() } - - then: - - cache.get("foo", fn) == "FOO" - cache.get("bar", fn) == "BAR" - cache.get("baz", fn) == "BAZ" - cache.get("fizz", fn) == "FIZZ" - cache.get("buzz", fn) == "BUZZ" - delegate.size() == 3 - } -} diff --git a/settings.gradle b/settings.gradle index 06088cacb8..3c49bab31e 100644 --- a/settings.gradle +++ b/settings.gradle @@ -35,6 +35,7 @@ rootProject.name = 'opentelemetry-java-instrumentation' include ':opentelemetry-api-shaded-for-instrumenting' include ':opentelemetry-ext-annotations-shaded-for-instrumenting' include ':javaagent-bootstrap' +include ':javaagent-bootstrap-tests' include ':javaagent-spi' include ':javaagent-tooling' include ':javaagent' @@ -42,6 +43,7 @@ include ':load-generator' include ':bom-alpha' include ':instrumentation-api' +include ':instrumentation-api-caching' include ':javaagent-api' // misc