CacheBuilder fails if jdk.unsupported module is not available

[bcmedeiros]

Is your feature request related to a problem? Please describe.
The current version of opentelemetry-java (1.5.3 as of this bug report) requires the jdk.unsupported JDK module to be included if one is using jlink to build a custom JDK. This is not a sustainable approach.

The following error occurs:

[otel.javaagent 2021-09-11 08:20:50:638 +0000] [main] INFO io.opentelemetry.javaagent.tooling.VersionLogger - opentelemetry-javaagent - version: 1.5.3
[otel.javaagent 2021-09-11 08:20:50:911 +0000] [main] INFO io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent - Your platform does not provide complete low-level API for accessing direct buffers reliably. Unless explicitly requested, heap buffer will always be preferred to avoid potential system instability.
ERROR io.opentelemetry.javaagent.OpenTelemetryAgent
java.lang.reflect.InvocationTargetException
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
    at java.base/java.lang.reflect.Method.invoke(Unknown Source)
    at io.opentelemetry.javaagent.bootstrap.AgentInitializer.initialize(AgentInitializer.java:40)
    at io.opentelemetry.javaagent.OpenTelemetryAgent.agentmain(OpenTelemetryAgent.java:51)
    at io.opentelemetry.javaagent.OpenTelemetryAgent.premain(OpenTelemetryAgent.java:44)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
    at java.base/java.lang.reflect.Method.invoke(Unknown Source)
    at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndStartAgent(Unknown Source)
    at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndCallPremain(Unknown Source)
Caused by: java.lang.NoClassDefFoundError: sun/misc/Unsafe
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.UnsafeAccess.load(UnsafeAccess.java:66)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.UnsafeAccess.<clinit>(UnsafeAccess.java:40)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.BLCHeader$DrainStatusRef.<clinit>(BoundedLocalCache.java:3931)
    at java.base/java.lang.Class.forName0(Native Method)
    at java.base/java.lang.Class.forName(Unknown Source)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.LocalCacheFactory.newBoundedLocalCache(LocalCacheFactory.java:87)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.BoundedLocalCache$BoundedLocalManualCache.<init>(BoundedLocalCache.java:3387)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.BoundedLocalCache$BoundedLocalManualCache.<init>(BoundedLocalCache.java:3383)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.internal.shaded.caffeine.cache.Caffeine.build(Caffeine.java:1079)
    at io.opentelemetry.javaagent.shaded.instrumentation.api.caching.CacheBuilder.build(CacheBuilder.java:71)
    at io.opentelemetry.javaagent.tooling.muzzle.AgentCachingPoolStrategy<clinit>(AgentCachingPoolStrategy.java:58)
    at io.opentelemetry.javaagent.tooling.muzzle.AgentTooling.<clinit>(AgentTooling.java:15)
    at io.opentelemetry.javaagent.tooling.AgentInstaller.installBytebuddyAgent(AgentInstaller.java:129)
    at io.opentelemetry.javaagent.tooling.AgentInstaller.installBytebuddyAgent(AgentInstaller.java:97)

Describe the solution you'd like
I would like to be able to run opentelemetry-java with a custom built JDK that does NOT include jdk.unsupported.

Describe alternatives you've considered
I'm currenlty adding jdk.unsupported in my jlink command, but I need to have a plan to remove those eventually. opentelemetry-java is the most important dependency in my classpath that requires jdk.unsupported.

[bcmedeiros]

This is a follow-up of open-telemetry/opentelemetry-java#3339

[bcmedeiros]

This seems to be what we want: ben-manes/caffeine#273

[bcmedeiros]

Created this as a bug by mistake, can someone change it to a feature request?

[anuraaga]

We'll probably need to fallback to a different implementation if caffeine can't be initialized. We should only try it after fixing #4096 since that seems like it could be more tricky.

[bcmedeiros]

We'll probably need to fallback to a different implementation if caffeine can't be initialized.

Is Guava an option here? I see the cache builder supports a few features, I'm wondering if Guava could be a replacement.

[mateuszrzeszutek]

Could we make use of the multi-release jar functionality and use caffeine 2+ for Java [8, 11) and caffeine 3+ for Java 11+?

[anuraaga]

@mateuszrzeszutek I think that's very heavyweight since caffeine is relatively big to have two copies. Though it'd still be much smaller than adding Guava to the slim distro. Non-slim has guava forced in by grpc though.

I think the simplest thing to do at first is to provide an escape hatch by adding an SPI to load a Cache implementation. I suspect users of jlink are advanced enough to be able to work with this to provide something using caffeine3 or guava, is that right @brunojcm? :P

We could follow up with possibilities of providing a published agent extension module that can be used as is, or have the non-slim agent register a guava SPI implementation if Unsafe is unavailable. Hopefully we could delay on this usability improvement, given the escape hatch, until a rule of three with some more demand - from my understanding, currently there are a total of 0 users jlinking without jdk.unsupported, including @brunocjm so it feels premature to do the work without actual usage.

[anuraaga]

By the way, while I said "We should only try it after fixing #4096 since that seems like it could be more tricky." I realized this isn't entirely correct. It's actually more important to solve the issue for the caching API since it's used from library instrumentation. We would need to make a decision on it, but presumably it's fair to say the agent does require jdk.unsupported and use library instrumentation if you can't provide that. Presumably library instrumentation fits better with the goal of small size that such jlinking is trying to achieve anyways.

Though @laurit's suggestion of providing our own sun.misc.Unsafe is interesting - I'm afraid of maintenance issues across JVMs if the assumption of "just delegate to jdk.internal" doesn't always hold true as it's literally replacing an internal detail of the JVM.

[iNikem]

Is it possible to port Unsafe removal from Caffeine 3 to 2?

[anuraaga]

@iNikem FWIU, unsafe removal uses Java 9+ APIs. /cc @ben-manes

[bcmedeiros]

I asked this there already, he said no

[laurit]

I didn't even consider library implementations. Would just fixing caching-api be enough? I looked into exporterLibs-relocated.jar generated in javaagent build and there seem to be a bunch of references for unsafe. Are all of these optional or just not used by our code? I assume that this jar contains exporters and their dependencies and library instrumentation would also need to be used with an exporter.
Our own sun.misc.Unsafe wouldn't help with library instrumentation as we can't get access to internal unsafe implementation without javaagent. From maintainability perspective I wouldn't expect it to be any worse than what we have with framework instrumentations. If we are unsure about how to implement some method we can always look at the actual sun.misc.Unsafe and do the same.

[ben-manes]

In regards to Caffeine...

What about simply having Java 8 users force the version back down to 2.x? Per jdbi docs,

NOTE: to run on Java 8, you may need to manage the caffeine dependency back to the latest 2.x release. 3.x is necessary for newer JDKs but does not run on 8.

This would discourage JDK8 but not deny it, which seems appropriate at this point.

At this point I do not think it is worth the effort to migrate 2.x to AF*U. While their performance has improved to the point where it shouldn't be a problem, the work involved exceeds the benefits imho. Encouraging upgrades and disincentivizing EOL jdks seems like an easier path, at least with respect to the caching library.

[anuraaga]

@ben-manes Thanks that is thought provoking. We currently shade caffeine since we usually expect shading to make life easier downstream, avoiding conflicts and such. In this case it removes the ability to pick a version based on your JVM though. We'll want to reconsider this