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

Drop instrumentation-api-caching module and move weak cache implementation to instrumentation-api #4667

Merged
merged 33 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
99b42e1
Drop instrumentation-api-caching module and move weak cache implement…
iNikem Nov 18, 2021
f08d4f5
Some test fixes
iNikem Nov 18, 2021
e651e80
Some cleanup
iNikem Nov 19, 2021
c63d3fe
Temporary workaround for using weak values in FutureListenerWrappers
iNikem Nov 19, 2021
f61ea3a
Merge remote-tracking branch 'upstream/main' into remove-cache-from-api
trask Nov 21, 2021
830e1d2
Spotless
trask Nov 21, 2021
4abddb3
Update ClassNames and SpanNames
trask Nov 21, 2021
7bdf3e0
Compilation and comment
trask Nov 21, 2021
cde78fb
Add bounded cache and clean interface
iNikem Nov 21, 2021
437da21
Polish
iNikem Nov 21, 2021
fdc64bb
Add comment
trask Nov 21, 2021
f7385d4
Vendor ConcurrentLinkedHashMap in
iNikem Nov 22, 2021
5e595e0
Let errorprone ignore vendored CLHM for now
iNikem Nov 22, 2021
0e04a04
Merge remote-tracking branch 'upstream/main' into remove-cache-from-api
trask Nov 22, 2021
257a6ec
Keep license in java files too
trask Nov 22, 2021
2f57a21
Merge remote-tracking branch 'upstream/main' into remove-cache-from-api
trask Nov 23, 2021
dac1522
Convert Netty wrapper cache to VirtualField
trask Nov 23, 2021
6d63815
Work around lambda instrumentation failure
trask Nov 24, 2021
7448cde
Revert "Work around lambda instrumentation failure"
trask Nov 24, 2021
fad0967
Revert "Convert Netty wrapper cache to VirtualField"
trask Nov 24, 2021
9ab70f4
Handle cleared weak values
trask Nov 23, 2021
3f36678
Fix comment
trask Nov 24, 2021
6ebb98b
Delete instrumentation-api-caching
trask Nov 24, 2021
37569ca
Copy in weak-lock-free
trask Nov 24, 2021
fe8ff0b
Remove caffeine remnants
trask Nov 24, 2021
d4d02da
Fix checkstyle
trask Nov 24, 2021
55f8292
Rename BoundedCache to MapBackedCached
trask Nov 24, 2021
ce93d40
Remove duplicate LICENSE
trask Nov 24, 2021
03824a4
Remove outdated comment
trask Nov 24, 2021
a8805fb
Sync with SDK copy of weaklockfree
trask Nov 24, 2021
7ad1a12
Enable checkstyle:off comment
trask Nov 24, 2021
7ae3ab2
Re-generate license report
trask Nov 24, 2021
11bee8a
Move NOTICE file to package-info.java
trask Nov 24, 2021
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 @@ -12,8 +12,4 @@ dependencies {
compileOnly("io.opentelemetry:opentelemetry-api")
compileOnly(project(":instrumentation-api"))
compileOnly(project(":javaagent-instrumentation-api"))

// this only exists to make Intellij happy since it doesn't (currently at least) understand our
// inclusion of this artifact inside of :instrumentation-api
compileOnly(project(":instrumentation-api-caching"))
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,4 @@ plugins {

extra["mavenGroupId"] = "io.opentelemetry.javaagent.instrumentation"

base.archivesName.set(projectDir.parentFile.name)

dependencies {
// this only exists to make Intellij happy since it doesn't (currently at least) understand our
// inclusion of this artifact inside of :instrumentation-api
compileOnly(project(":instrumentation-api-caching"))
}
base.archivesName.set(projectDir.parentFile.name)
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,4 @@ plugins {

extra["mavenGroupId"] = "io.opentelemetry.instrumentation"

base.archivesName.set(projectDir.parentFile.name)

dependencies {
// this only exists to make Intellij happy since it doesn't (currently at least) understand our
// inclusion of this artifact inside of :instrumentation-api
compileOnly(project(":instrumentation-api-caching"))
}
base.archivesName.set(projectDir.parentFile.name)
5 changes: 0 additions & 5 deletions instrumentation-api-annotation-support/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ group = "io.opentelemetry.instrumentation"
dependencies {
implementation(project(":instrumentation-api"))

// this only exists to make Intellij happy since it doesn't (currently at least) understand our
// inclusion of this artifact inside of :instrumentation-api
compileOnly(project(":instrumentation-api-caching"))
testCompileOnly(project(":instrumentation-api-caching"))

api("io.opentelemetry:opentelemetry-api")
api("io.opentelemetry:opentelemetry-semconv")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.instrumentation.api.annotation.support;

import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;
import java.lang.reflect.Method;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.api.annotation.support;

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.tracer.AttributeSetter;
import java.lang.reflect.Method;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.api.annotation.support

import io.opentelemetry.api.common.AttributesBuilder
import io.opentelemetry.instrumentation.api.caching.Cache
import io.opentelemetry.instrumentation.api.cache.Cache
import spock.lang.Specification

import java.lang.reflect.Method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;
import java.lang.reflect.Method;
import java.util.function.Function;
import org.junit.jupiter.api.Test;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import javax.annotation.Nullable;

/** A cache from keys to values. */
public interface Cache<K, V> {
interface Cache<K, V> {

/** Returns a new {@link CacheBuilder} to configure a {@link Cache}. */
static CacheBuilder builder() {
Expand Down
8 changes: 2 additions & 6 deletions instrumentation-api/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -14,27 +14,23 @@ sourceSets {
// set to generate into. By default it would be the src/main directory itself.
srcDir("$buildDir/generated/sources/jflex")
}

val cachingShadedDeps = project(":instrumentation-api-caching")
output.dir(cachingShadedDeps.file("build/extracted/shadow"), "builtBy" to ":instrumentation-api-caching:extractShadowJar")
}
}

group = "io.opentelemetry.instrumentation"

dependencies {
compileOnly(project(":instrumentation-api-caching"))

api("io.opentelemetry:opentelemetry-api")
api("io.opentelemetry:opentelemetry-semconv")

implementation("io.opentelemetry:opentelemetry-api-metrics")
implementation("org.slf4j:slf4j-api")
implementation("com.blogspot.mydailyjava:weak-lock-free")
implementation("com.googlecode.concurrentlinkedhashmap:concurrentlinkedhashmap-lru:1.4.2")
Copy link
Member

Choose a reason for hiding this comment

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

let's shade these in. less transitive dependencies will make instrumentation-api more palatable for libraries to embed directly

Copy link
Member

Choose a reason for hiding this comment

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

we copied in CLHM in order to update code to use default CHM

I copied in weak-lock-free also (instead of shading), just to avoid gradle shadow problems (only 4 files)


compileOnly("com.google.auto.value:auto-value-annotations")
annotationProcessor("com.google.auto.value:auto-value")

testCompileOnly(project(":instrumentation-api-caching"))
testImplementation(project(":testing-common"))
testImplementation("org.mockito:mockito-core")
testImplementation("org.mockito:mockito-junit-jupiter")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.cache;

import java.util.concurrent.ConcurrentMap;
import java.util.function.Function;
import javax.annotation.Nullable;

final class BoundedCache<K, V> implements Cache<K, V> {
trask marked this conversation as resolved.
Show resolved Hide resolved

private final ConcurrentMap<K, V> delegate;

BoundedCache(ConcurrentMap<K, V> delegate) {
this.delegate = delegate;
}

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

@Nullable
@Override
public V get(K key) {
return delegate.get(key);
}

@Override
public void put(K key, V value) {
delegate.put(key, value);
}

@Override
public void remove(K key) {
delegate.remove(key);
}

// Visible for tests
int size() {
return delegate.size();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.cache;

import com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap;
import java.util.function.Function;
import javax.annotation.Nullable;

/**
* A cache from keys to values.
*
* <p>Keys are always referenced weakly and are compared using identity comparison, not {@link
trask marked this conversation as resolved.
Show resolved Hide resolved
* Object#equals(Object)}.
*/
public interface Cache<K, V> {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep the name Cache? It's now just a weak-keyed map, since we removed caffeine et al.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not want to rename in this PR, to avoid too many changes at once. We can rename later, if we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now there are two different implementations, so we better keep this generic name


/**
* Returns new unbounded cache
*
* <p>Keys are referenced weakly and compared using identity comparison, not {@link
* Object#equals(Object)}.
*/
static <K, V> Cache<K, V> weak() {
return new WeakLockFreeCache<>();
}

/**
* Returns new bounded cache.
*
* <p>Both keys and values are strongly referenced.
*/
static <K, V> Cache<K, V> bounded(int capacity) {
ConcurrentLinkedHashMap<K, V> map =
new ConcurrentLinkedHashMap.Builder<K, V>().maximumWeightedCapacity(capacity).build();
return new BoundedCache<>(map);
}

/**
* 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);

/**
* Returns the cached value associated with the provided {@code key} if present, or {@code null}
* otherwise.
*/
@Nullable
V get(K key);

/** Puts the {@code value} into the cache for the {@code key}. */
void put(K key, V value);

/** Removes a value for {@code key} if present. */
void remove(K key);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.caching;
package io.opentelemetry.instrumentation.api.cache;

import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap;
import java.util.function.Function;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import static io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics.CounterNames.SQL_STATEMENT_SANITIZER_CACHE_MISS;

import com.google.auto.value.AutoValue;
import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;
import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics;
import javax.annotation.Nullable;

Expand All @@ -21,7 +21,7 @@ public final class SqlStatementSanitizer {
private static final SupportabilityMetrics supportability = SupportabilityMetrics.instance();

private static final Cache<CacheKey, SqlStatementInfo> sqlToStatementInfoCache =
Cache.builder().setMaximumSize(1000).build();
Cache.bounded(1000);

public static SqlStatementInfo sanitize(@Nullable String statement) {
return sanitize(statement, SqlDialect.DEFAULT);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,19 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.instrumentation.api.caching.Cache;
import java.util.List;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;

final class HttpHeaderAttributes {

private static final Cache<String, AttributeKey<List<String>>> requestKeysCache =
Cache.builder().setMaximumSize(32).build();
private static final Cache<String, AttributeKey<List<String>>> responseKeysCache =
Cache.builder().setMaximumSize(32).build();
// these are naturally bounded because they only store keys listed in
// otel.instrumentation.http.capture-headers.server.request and
// otel.instrumentation.http.capture-headers.server.response
private static final ConcurrentMap<String, AttributeKey<List<String>>> requestKeysCache =
new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>> responseKeysCache =
new ConcurrentHashMap<>();

static AttributeKey<List<String>> requestAttributeKey(String headerName) {
return requestKeysCache.computeIfAbsent(headerName, n -> createKey("request", n));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.instrumentation.api.internal;

import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;
import io.opentelemetry.instrumentation.api.field.VirtualField;
import javax.annotation.Nullable;
import org.slf4j.Logger;
Expand Down Expand Up @@ -40,19 +40,19 @@ public static VirtualFieldSupplier get() {
private static final class CacheBasedVirtualFieldSupplier implements VirtualFieldSupplier {

private final Cache<Class<?>, Cache<Class<?>, VirtualField<?, ?>>>
ownerToFieldToImplementationMap = Cache.builder().setWeakKeys().build();
ownerToFieldToImplementationMap = Cache.weak();

@Override
public <U extends T, T, F> VirtualField<U, F> find(Class<T> type, Class<F> fieldType) {
return (VirtualField<U, F>)
ownerToFieldToImplementationMap
.computeIfAbsent(type, c -> Cache.builder().setWeakKeys().build())
.computeIfAbsent(type, c -> Cache.weak())
.computeIfAbsent(fieldType, c -> new CacheBasedVirtualField<>());
}
}

private static final class CacheBasedVirtualField<T, F> extends VirtualField<T, F> {
private final Cache<T, F> cache = Cache.builder().setWeakKeys().build();
private final Cache<T, F> cache = Cache.weak();

@Override
@Nullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

package io.opentelemetry.instrumentation.api.tracer;

import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;

public final class ClassNames {

private static final Cache<Class<?>, String> simpleNames = Cache.builder().setWeakKeys().build();
private static final Cache<Class<?>, String> simpleNames = Cache.weak();

/**
* This method is used to generate a simple name based on a given class reference, e.g. for use in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.instrumentation.api.tracer;

import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.cache.Cache;
import io.opentelemetry.instrumentation.api.util.ClassAndMethod;
import java.lang.reflect.Method;
import java.util.Map;
Expand All @@ -14,8 +14,7 @@

public final class SpanNames {

private static final Cache<Class<?>, Map<String, String>> spanNameCaches =
Cache.builder().setWeakKeys().build();
private static final Cache<Class<?>, Map<String, String>> spanNameCaches = Cache.weak();

/**
* This method is used to generate a span name based on a method. Anonymous classes are named
Expand Down
Loading