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

iNikem · 2021-11-18T09:20:42Z

This if first draft PR to verify my understand of what we agreed upon on Tuesday.

We have couple of usages that still don't work with WeakLockFreeCache:

FutureListenerWrappers wants weak values
ClassLoaderHasClassesNamedMatcher and AgentCachingPoolStrategy want to specify max capacity

How did we want to solve that?

…ation to instrumentation-api

iNikem · 2021-11-18T10:36:47Z

The bigger problem is AgentCachingPoolStrategy.SharedResolutionCacheAdapter.find, which currently cannot work with comparison-based cache keys

mateuszrzeszutek · 2021-11-18T13:36:23Z

I think FutureListenerWrappers should actually work correctly with a VirtualField now - a couple of weeks ago @laurit has introduced a lambda instrumentation that makes them go through ClassFileTransformer.

mateuszrzeszutek · 2021-11-18T13:40:47Z

...ain/java/io/opentelemetry/javaagent/extension/matcher/ClassLoaderHasClassesNamedMatcher.java

  private final Cache<ClassLoader, Boolean> cache =
-      Cache.builder().setWeakKeys().setMaximumSize(25).build();
+      Cache.builder().build();


Maybe we could just remove the max size here? This usage is very similar to the one in MuzzleMatcher, and we don't have any limit there.

👍 for just weak keys (no max) here

mateuszrzeszutek · 2021-11-18T13:43:21Z

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/cache/Cache.java

+ * Keys are always referenced weakly and are compared using identity comparison, not
+ * {@link Object#equals(Object)}.
+ */
+public interface Cache<K, V> {


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

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

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

iNikem · 2021-11-19T08:17:52Z

Only 2 problems remain.

One is simple: SqlStatementSanitizer wants to use a bounded cache. I am not sure if bringing in the whole CLHM just for one use case makes sense.

Another one is more complicated: AgentCachingPoolStrategy.SharedResolutionCacheAdapter relies on cache keys being compared by equal. But I don't totally grasp why do we need this custom implementation of ByteBuddy cache at all.

trask · 2021-11-21T03:59:21Z

SqlStatementSanitizer wants to use a bounded cache. I am not sure if bringing in the whole CLHM just for one use case makes sense.

This is an important cache for an important use case (jdbc). I'm not sure what alternative we have? I think LRU (CLHM) should be enough, not sure if we need more advanced LFU eviction (caffeine), which would avoid the complexity of caffeine2/3 for now at least (in both library and agent).

Another one is more complicated: AgentCachingPoolStrategy.SharedResolutionCacheAdapter relies on cache keys being compared by equal. But I don't totally grasp why do we need this custom implementation of ByteBuddy cache at all.

My guess based on prior work is that this is used to avoid re-analyzing classes during instrumentation (e.g. looking up structure/hierarchy of parent class/interfaces). This can help a lot with startup overhead. I recall the DataDog folks spent a good amount of time tuning this particular cache. I'd be hesitant to mess with it without doing a good amount of testing. I think CLHM should be good here also (no need for more advanced eviction).

iNikem · 2021-11-21T10:08:33Z

...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpHeaderAttributes.java

-      Cache.builder().setMaximumSize(32).build();
-  private static final Cache<String, AttributeKey<List<String>>> responseKeysCache =
-      Cache.builder().setMaximumSize(32).build();
+  private static final ConcurrentHashMap<String, AttributeKey<List<String>>> requestKeysCache =


Should I replace this with bounded cache now?

these are naturally limited, I just pushed comment to this file

iNikem · 2021-11-21T12:02:47Z

CLHM uses unsafe and thus fails our jpms tests. Should I switch to cache2k? I am a little bit concerned by its size, 400K is not that small.

trask · 2021-11-21T21:22:58Z

...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpHeaderAttributes.java

-      Cache.builder().setMaximumSize(32).build();
-  private static final Cache<String, AttributeKey<List<String>>> responseKeysCache =
-      Cache.builder().setMaximumSize(32).build();
+  private static final ConcurrentHashMap<String, AttributeKey<List<String>>> requestKeysCache =


these are naturally limited, I just pushed comment to this file

trask · 2021-11-21T21:25:15Z

instrumentation-api/build.gradle.kts

+  implementation("com.blogspot.mydailyjava:weak-lock-free")
+  implementation("com.googlecode.concurrentlinkedhashmap:concurrentlinkedhashmap-lru:1.4.2")


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

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)

trask · 2021-11-21T21:32:44Z

CLHM uses unsafe and thus fails our jpms tests. Should I switch to cache2k? I am a little bit concerned by its size, 400K is not that small.

I keep forgetting all the branches of this problem 😞

This is what using Caffeine 3 on Java 11 would solve for us.

Cache2k seems like a reasonable option to keep things simple for now.

As long as it's all hidden / shaded dependency we can improve on it later.

ben-manes · 2021-11-22T00:33:49Z

CLHM has a backport of CHM v8 which uses unsafe. Most just copy the code into their project and replace with the standard CHM. For example ms-jdbc, groovy, micronaut. This is pretty much equivalent to shading.

trask · 2021-11-22T00:48:24Z

CLHM has a backport of CHM v8 which uses unsafe. Most just copy the code into their project and replace with the standard CHM. For example ms-jdbc, groovy, micronaut. This is pretty much equivalent to shading.

thx @ben-manes! 👍

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/cache/BoundedCache.java

trask · 2021-11-22T21:34:53Z

...ain/java/io/opentelemetry/javaagent/instrumentation/netty/common/FutureListenerWrappers.java

+              }
+              return new WeakReference<>(wrapper);
+            });
+    return resultReference.get();


we may need to be careful here, I think the cache can return a "cleared" weak value reference now (previously caffeine took care of not making "cleared" values visible).

Instead, I think we could use VirtualField instead of Cache, essentially reverting #3059 now that #4182 solves the underlying problem with VirtualField not being applied to lambdas.

I sent two commits converting from Cache to VirtualField

Convert Netty wrapper cache to VirtualField

Work around lambda instrumentation failure

The second commit was needed to workaround a problem caused by the VirtualField instrumenting some of our own helper class lambdas. Normally I don't think helper classes are instrumented since they are injected from inside of ClassFileTransformer.transform (which is not recursive). But lambdas are different since they don't go through the normal class definition / transformation process.

I was already having second thoughts about the first commit above after typing up the warning about the memory leak that will occur if this code is ever used inside of library instrumentation.

I did add a commit to handle cleared weak values.

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/cache/BoundedCache.java

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/cache/Cache.java

...api/src/main/java/io/opentelemetry/instrumentation/api/cache/concurrentlinkedhashmap/LICENSE

anuraaga · 2021-11-24T02:05:28Z

...-api/src/main/java/io/opentelemetry/instrumentation/api/cache/concurrentlinkedhashmap/NOTICE

@@ -0,0 +1,7 @@
+ConcurrentLinkedHashMap
+Copyright 2008, Ben Manes


By the way I noticed micronaut doesn't reference the NOTICE, nor update license headers

https://github.com/micronaut-projects/micronaut-core/tree/813923882bcfed158d136a27bf8010edf70bb613/core/src/main/java/io/micronaut/core/util/clhm

IANAL, but I think the author tag satisfies "readable copy of attribution notices contained within such NOTICE file...within the Source form or documentation"

https://github.com/ben-manes/concurrentlinkedhashmap/blob/master/LICENSE#L111

Mostly not a fan of having license type files within the java source - converting to a docstring on a package-info.java would also be better to me if needing to include it

converting to a docstring on a package-info.java would also be better to me if needing to include it

went with this 👍

I also re-generated the license report, which pulls in NOTICE files if they're in the dependencies jar file, but in this case it's not.

.../java/io/opentelemetry/instrumentation/api/cache/weaklockfree/AbstractWeakConcurrentMap.java

...c/main/java/io/opentelemetry/instrumentation/api/cache/weaklockfree/DetachedThreadLocal.java

...src/main/java/io/opentelemetry/instrumentation/api/cache/weaklockfree/WeakConcurrentSet.java

anuraaga

Just point about NOTICE but not a huge deal

laurit · 2021-11-24T09:28:31Z

...ain/java/io/opentelemetry/javaagent/instrumentation/netty/common/FutureListenerWrappers.java

+  // remove the wrapped listener from the netty future, and if the value is collected prior to the
+  // key, that means it's no longer used (referenced) by the netty future anyways.
+  //
+  // also note: this is not using VirtualField in case this is ever converted to library


Alternatively we could add an option to use weak values when VirtualField fallback map is used. When we know that VirtualField value references key we could just use VirtualField with weak values and would not need to worry whether it is going to use fallback map because something wasn't instrumented or because it is used inside library instrumentation.

I'll open a issue for this 👍

laurit · 2021-11-24T09:32:17Z

...ain/java/io/opentelemetry/javaagent/extension/matcher/ClassLoaderHasClassesNamedMatcher.java

 import io.opentelemetry.javaagent.instrumentation.api.internal.ClassLoaderMatcherCacheHolder;
 import io.opentelemetry.javaagent.instrumentation.api.internal.InClassLoaderMatcher;
 import net.bytebuddy.matcher.ElementMatcher;

 class ClassLoaderHasClassesNamedMatcher extends ElementMatcher.Junction.AbstractBase<ClassLoader> {

-  private final Cache<ClassLoader, Boolean> cache =
-      Cache.builder().setWeakKeys().setMaximumSize(25).build();
+  private final Cache<ClassLoader, Boolean> cache = Cache.weak();


I like this, in my opinion 25 was way too low.

mateuszrzeszutek · 2021-11-29T12:12:42Z

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/cache/Cache.java


+import io.opentelemetry.instrumentation.api.cache.concurrentlinkedhashmap.ConcurrentLinkedHashMap;


Should we put CLHM and WeakLockFree in internal package(s)? They shouldn't be a part of our public API.

👍 created #4735 to track

…ation to instrumentation-api (open-telemetry#4667) * Drop instrumentation-api-caching module and move weak cache implementation to instrumentation-api * Some test fixes * Some cleanup * Temporary workaround for using weak values in FutureListenerWrappers * Spotless * Update ClassNames and SpanNames * Compilation and comment * Add bounded cache and clean interface * Polish * Add comment * Vendor ConcurrentLinkedHashMap in * Let errorprone ignore vendored CLHM for now * Keep license in java files too * Convert Netty wrapper cache to VirtualField * Work around lambda instrumentation failure Ideally we would ignore instrumenting helper classes... * Revert "Work around lambda instrumentation failure" This reverts commit 6d63815. * Revert "Convert Netty wrapper cache to VirtualField" This reverts commit dac1522. * Handle cleared weak values * Fix comment * Delete instrumentation-api-caching * Copy in weak-lock-free * Remove caffeine remnants * Fix checkstyle * Rename BoundedCache to MapBackedCached * Remove duplicate LICENSE * Remove outdated comment * Sync with SDK copy of weaklockfree * Enable checkstyle:off comment * Re-generate license report * Move NOTICE file to package-info.java Co-authored-by: Trask Stalnaker <[email protected]>

Drop instrumentation-api-caching module and move weak cache implement…

99b42e1

…ation to instrumentation-api

Some test fixes

f08d4f5

mateuszrzeszutek reviewed Nov 18, 2021

View reviewed changes

iNikem added 2 commits November 19, 2021 09:56

Some cleanup

e651e80

Temporary workaround for using weak values in FutureListenerWrappers

c63d3fe

trask added 2 commits November 20, 2021 18:30

Merge remote-tracking branch 'upstream/main' into remove-cache-from-api

f61ea3a

Spotless

830e1d2

trask force-pushed the remove-cache-from-api branch from 6ec3420 to 830e1d2 Compare November 21, 2021 03:15

trask added 2 commits November 20, 2021 19:18

Update ClassNames and SpanNames

4abddb3

Compilation and comment

7bdf3e0

Add bounded cache and clean interface

cde78fb

iNikem commented Nov 21, 2021

View reviewed changes

Polish

437da21

Add comment

fdc64bb

trask reviewed Nov 21, 2021

View reviewed changes

iNikem and others added 4 commits November 22, 2021 10:00

Vendor ConcurrentLinkedHashMap in

f7385d4

Let errorprone ignore vendored CLHM for now

5e595e0

Merge remote-tracking branch 'upstream/main' into remove-cache-from-api

0e04a04

Keep license in java files too

257a6ec

trask reviewed Nov 22, 2021

View reviewed changes

trask added 2 commits November 23, 2021 14:51

Merge remote-tracking branch 'upstream/main' into remove-cache-from-api

2f57a21

Convert Netty wrapper cache to VirtualField

dac1522

trask added 3 commits November 23, 2021 16:31

Delete instrumentation-api-caching

6ebb98b

Copy in weak-lock-free

37569ca

Remove caffeine remnants

fe8ff0b

trask marked this pull request as ready for review November 24, 2021 01:14

trask requested review from anuraaga, jkwatson, laurit, pavolloffay and tylerbenson as code owners November 24, 2021 01:14

trask approved these changes Nov 24, 2021

View reviewed changes

anuraaga reviewed Nov 24, 2021

View reviewed changes

trask added 6 commits November 23, 2021 18:14

Fix checkstyle

d4d02da

Rename BoundedCache to MapBackedCached

55f8292

Remove duplicate LICENSE

ce93d40

Remove outdated comment

03824a4

Sync with SDK copy of weaklockfree

a8805fb

Enable checkstyle:off comment

7ad1a12

trask mentioned this pull request Nov 24, 2021

Add Android API-friendliness checks #4505

Merged

anuraaga approved these changes Nov 24, 2021

View reviewed changes

trask added 2 commits November 23, 2021 20:11

Re-generate license report

7ae3ab2

Move NOTICE file to package-info.java

11bee8a

laurit reviewed Nov 24, 2021

View reviewed changes

laurit approved these changes Nov 24, 2021

View reviewed changes

trask merged commit 821a4b8 into open-telemetry:main Nov 24, 2021

iNikem deleted the remove-cache-from-api branch November 25, 2021 08:26

mateuszrzeszutek reviewed Nov 29, 2021

View reviewed changes

trask mentioned this pull request Nov 29, 2021

Move concurrentlinkedhashmap and weaklockfree to internal packages #4735

Closed

mateuszrzeszutek mentioned this pull request Nov 30, 2021

Remove caffeine from instrumentation-api to make it work on Android #4532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iNikem commented Nov 18, 2021

iNikem commented Nov 18, 2021

mateuszrzeszutek commented Nov 18, 2021

mateuszrzeszutek Nov 18, 2021

trask Nov 18, 2021

mateuszrzeszutek Nov 18, 2021

iNikem Nov 18, 2021

iNikem Nov 21, 2021

iNikem commented Nov 19, 2021

trask commented Nov 21, 2021

iNikem Nov 21, 2021

trask Nov 21, 2021

iNikem commented Nov 21, 2021

trask Nov 21, 2021

trask Nov 21, 2021

trask Nov 24, 2021

trask commented Nov 21, 2021

ben-manes commented Nov 22, 2021

trask commented Nov 22, 2021

trask Nov 22, 2021

trask Nov 24, 2021

anuraaga Nov 24, 2021

anuraaga Nov 24, 2021 •

edited

Loading

trask Nov 24, 2021

anuraaga left a comment

laurit Nov 24, 2021

trask Nov 24, 2021

laurit Nov 24, 2021

mateuszrzeszutek Nov 29, 2021

trask Nov 29, 2021

		implementation("com.blogspot.mydailyjava:weak-lock-free")
		implementation("com.googlecode.concurrentlinkedhashmap:concurrentlinkedhashmap-lru:1.4.2")

		@@ -0,0 +1,7 @@
		ConcurrentLinkedHashMap
		Copyright 2008, Ben Manes


		import io.opentelemetry.instrumentation.api.cache.concurrentlinkedhashmap.ConcurrentLinkedHashMap;

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

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

Conversation

iNikem commented Nov 18, 2021

iNikem commented Nov 18, 2021

mateuszrzeszutek commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented Nov 19, 2021

trask commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented Nov 21, 2021

ben-manes commented Nov 22, 2021

trask commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga Nov 24, 2021 •

edited

Loading