Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a caching API based on caffeine for use from instrumentation, not just javaagent #2477

Merged
merged 12 commits into from
Mar 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
]

Expand Down Expand Up @@ -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

Expand All @@ -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}"
]
}
1 change: 1 addition & 0 deletions gradle/spotbugs-exclude.xml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<Or>
<Class name="io.opentelemetry.instrumentation.test.utils.GcUtils"/>
<Class name="io.opentelemetry.javaagent.util.GcUtils"/>
<Class name="~io.opentelemetry.instrumentation.api.caching.CacheTest.*"/>
</Or>
<Bug pattern="DM_GC"/>
</Match>
Expand Down
29 changes: 29 additions & 0 deletions instrumentation-api-caching/instrumentation-api-caching.gradle
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -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() {}
}
Original file line number Diff line number Diff line change
@@ -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<K, V> {

/** 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<? super K, ? extends V> mappingFunction);
}
Original file line number Diff line number Diff line change
@@ -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 <K, V> Cache<K, V> build() {
@SuppressWarnings("unchecked")
com.github.benmanes.caffeine.cache.Cache<K, V> delegate =
(com.github.benmanes.caffeine.cache.Cache<K, V>) caffeine.build();
Comment on lines +39 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Why not add <K, V> as type parameters of CacheBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah Caffeine has them - since we don't have loading cache at least for now I couldn't find a reason to have them on the builder. This is the same trick Caffeine uses to convert Caffeine<Object, Object> to Cache<K, V>.

return new CaffeineCache<K, V>(delegate);
}

CacheBuilder() {}
}
Original file line number Diff line number Diff line change
@@ -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<K, V> implements Cache<K, V> {

private final com.github.benmanes.caffeine.cache.Cache<K, V> delegate;

CaffeineCache(com.github.benmanes.caffeine.cache.Cache<K, V> delegate) {
this.delegate = delegate;
}

@Override
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
return delegate.get(key, mappingFunction);
}

// Visible for testing
Set<K> keySet() {
return delegate.asMap().keySet();
}

// Visible for testing
void cleanup() {
delegate.cleanUp();
}
}
1 change: 1 addition & 0 deletions instrumentation-api-caching/src/test/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Tests for this module are in the instrumentation-api project to verify against the shaded artifact.
3 changes: 3 additions & 0 deletions instrumentation-api/instrumentation-api.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Original file line number Diff line number Diff line change
@@ -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<String, String> 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<String, String> 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();
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering: what does cleanup() exactly do here, does it remove values over max size?
Does this test pass without it? Would adding it to unbounded() make that test fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup it ensures values over max are removed since Caffeine does that asynchronously. Without it out fails, but for non evictions it's not needed

Choose a reason for hiding this comment

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

If you prefer, set the executor to Runnable::run for a direct call. The async isn’t needed for the cache’s own maintenance as that is fast, but we have optional callbacks to user code so it’s protected if that is slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ben-manes Thanks for the tip! Going to leave it as is for now to use the defaults as a baseline. Will set up a simple benchmark for our own sanity checking later, compare the two, and then pick one.

assertThat(caffeineCache.keySet()).hasSize(1);
assertThat(cache.computeIfAbsent("cat", unused -> "purr")).isEqualTo("purr");
}
}

@Nested
class WeakKeys {
@Test
void unbounded() {
Cache<String, String> 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();

Choose a reason for hiding this comment

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

GcFinalizable from guava testlib is great for this as more predictable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint! I've always wondered about this but in practice have yet to see a failure due to only calling gc once and continue with this pattern. Looking at it

https://github.com/google/guava/blob/master/guava-testlib/src/com/google/common/testing/GcFinalization.java#L186

I feel as if it would be mostly the same to move System.gc into the await() predicate, possibly tweaking the polling interval, which is nice to reduce the number of ways of doing something. Might do one or the other if this flakes in the future.

// Wait for GC to be reflected.
await()
.untilAsserted(
() -> {
caffeineCache.cleanup();
assertThat(caffeineCache.keySet()).isEmpty();
});
}

@Test
void bounded() throws Exception {
Cache<String, String> 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();
});
}
}
}

This file was deleted.