Addressing aws-sdk + plugin library muzzle issues / reflection creep

[Oberon00]

Hi, I was originally trying to come up with a PR for injecting into SQS message attributes, but noticed that it is quite cumbersome to reflect your way through all the classes/methods needed and that this project is usually using this cool muzzle thingy to get around that.

There is this comment, copied a few times for other classes: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/v1.25.1/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsMessageAccess.java#L16-L24

Basically, the AWS SDK v2 consists of a set of separate libraries per SQS-service and a core library. The aws-sdk-2 instrumentation has these dependencies:

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/v1.25.1/instrumentation/aws-sdk/aws-sdk-2.2/library/build.gradle.kts#L8-L9

IIUC, if I would add e.g. a dependency to SQS, I could code like I want and e.g. cast to SQS-specific classes and use their methods, but then somewhere (only in the agent or also for the library instrumentation?) the whole aws-sdk instrumentation would be suppressed if any of the depended-upon libraries are not available at runtime.

So this would be easily solvable if it there would be a separate dependency type like optionalLibrary that would prevent this (I guess one would need to take care to only actually use the plugin classes if you detect that it was loaded, so special care would still be needed).

If this would not be feasible, another way would be to have one plugin-instrumentation per aws-sdk plugin library, e.g. instead of having only an all-in-one io.opentelemetry.instrumentation/opentelemetry-aws-sdk-2.2, one would additionally have io.opentelemetry.instrumentation/opentelemetry-aws-sdk-sqs-2.2, and those would then need to interoperate somehow (in this case, I'm thinking of a SPI so that the core library chain-loads specific hook classes, and the API remains unified). In the end, this might be more awkward than the current reflection solution though.

Does any of these solutions sound reasonable to you? Where would I start to look if I wanted to try my hand at implementing the optionalLibrary thing?

CC @laurit @mateuszrzeszutek

[Oberon00]

Hmm, reading through https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/writing-instrumentation.md#instrumenting-code-that-is-not-available-as-a-maven-dependency, maybe the optionalLibrary I want would simply be compileOnly?

[laurit]

For Instrumentation dependencies where we want to use classes from application we use compileOnly or library(which extends compileOnly), dependencies in implementation scope are packaged inside the javaagent. optionalLibrary would similarly need to extend compileOnly. I think it could be rather tricky to implement as in the muzzle check we'd need to figure out for each dependency whether it is optional or not. It might be easier to explicitly list the packages that could be considered optional.
Currently the only option we have to suppress muzzle is the @NoMuzzle annotation that excludes methods with this annotation from muzzle checks. It is used in

opentelemetry-java-instrumentation/instrumentation/spring/spring-kafka-2.7/library/src/main/java/io/opentelemetry/instrumentation/spring/kafka/v2_7/InstrumentedRecordInterceptor.java

Lines 36 to 43 in d22dd8c

	@NoMuzzle
	@SuppressWarnings(
	"deprecation") // implementing deprecated method (removed in 3.0) for better compatibility
	@Override
	public ConsumerRecord<K, V> intercept(ConsumerRecord<K, V> record) {
	start(record, null);
	return decorated == null ? record : decorated.intercept(record);
	}

to implement an interface method that is removed in a later version and in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/logback/logback-appender-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/appender/v1_0/internal/LoggingEventMapper.java to get access to features that are not present in the earliest supported version. The drawback of this is that the muzzle checks will be completely disabled. Perhaps for your use case it would make sense to have something like ignore it if the class isn't present, but if it is do the muzzle checks.

[Oberon00]

But in this case, from what I can see already "compileOnly" would be an improvement over manual reflection, because at least you have the normal compiler type checks that you miss when using reflection. Or am I not understanding something?

[laurit]

Muzzle checks are done when you use compileOnly dependencies. You need to either suppress the muzzle checks with something like the @NoMuzzle annotation or split the instrumentation so that each aws library would have separate instrumentation and then somehow make all of them work together.

[Oberon00]

split the instrumentation so that each aws library would have separate instrumentation

That's what I imagined in my answer:

If this would not be feasible, another way would be to have one plugin-instrumentation per aws-sdk plugin library, e.g. instead of having only an all-in-one io.opentelemetry.instrumentation/opentelemetry-aws-sdk-2.2, one would additionally have io.opentelemetry.instrumentation/opentelemetry-aws-sdk-sqs-2.2, and those would then need to interoperate somehow (in this case, I'm thinking of a SPI so that the core library chain-loads specific hook classes, and the API remains unified). In the end, this might be more awkward than the current reflection solution though.

I guess @NoMuzzle might work fine as well.

What is the difference between compileOnly and library then? Probably this is already documented somewhere, could you point me to it? Unfortunately, library does not make for a good search term.

[laurit]

opentelemetry-java-instrumentation/conventions/src/main/kotlin/io.opentelemetry.instrumentation.base.gradle.kts

Lines 11 to 15 in d22dd8c

	* - library: A dependency on the instrumented library. Results in the dependency being added to
	* compileOnly and testImplementation. If the build is run with -PtestLatestDeps=true, the
	* version when added to testImplementation will be overridden by `+`, the latest version
	* possible. For simple libraries without different behavior between versions, it is possible
	* to have a single dependency on library only.

[Oberon00]

So I read up on it a bit, and it seems what we are working here against are not any compile time checks, but the runtime checks: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/safety-mechanisms.md#muzzle-runtime-checks

It seems that with some refactoring to move all code depending on a plugin library we should be able to use only a few @NoMuzzle annotations to "cut" the plugin library-dependent part from the transformer-reachable part.

The only problem I have with that is testing. It seems I would need to add a whole new test source set that does not depend on the plugin library and then check that the (non plugin-related) tests still succeed.
It would be cool if I could instead assert that certain packages do not appear in the muzzle-generated reference list.

EDIT: Seems like excludeDependency could be used for that

opentelemetry-java-instrumentation/gradle-plugins/src/main/kotlin/io/opentelemetry/javaagent/muzzle/MuzzleDirective.kt

Lines 54 to 59 in 2d7bd70

	/**
	* Adds transitive dependencies to exclude from the current muzzle test.
	*
	* @param excludeString A dependency in the gradle canonical form: '<group_id>:<artifact_id>'
	*/
	fun excludeDependency(excludeString: String?) {

EDIT2: No this seems to do something different

[laurit]

It would be cool if I could instead assert that certain packages do not appear in the muzzle-generated reference list.

Muzzle code generation plugin adds InstrumentationModuleMuzzle interface to InstrumentationModule implementations, you can get the muzzle references from there. See

opentelemetry-java-instrumentation/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferencesPrinter.java

Lines 32 to 35 in 5627a15

	for (ClassRef ref :
	InstrumentationModuleMuzzle.getMuzzleReferences(instrumentationModule).values()) {
	System.out.print(prettyPrint(ref));
	}

We use JvmTestSuite for testing multiple configurations, it should work for your use case too.

[Oberon00]

Hmm, I can't get that to work. I added the following in instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/test/groovy/SqsRefTest.groovy:

import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule
import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationModuleMuzzle
import spock.lang.Specification

class SqsRefTest extends Specification {
  def "does not reference sqs lib without @NoMuzzle"() {
    expect:
    for (final def module in ServiceLoader.load(InstrumentationModule.class)) {
      for (final def ref in InstrumentationModuleMuzzle.getMuzzleReferences(module).values()) {
        !ref.className.startsWith("com.amazon") || !ref.className.containsIgnoreCase("sqs")
      }
    }
  }
}

To the build.gradle.kts, I added

testImplementation(project(":javaagent-extension-api"))

But it fails with

io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule
java.lang.NoClassDefFoundError: io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule
	at SqsRefTest.does not reference sqs lib without @NoMuzzle(SqsRefTest.groovy:8)
Caused by: java.lang.ClassNotFoundException: io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)

despite compiling fine.

[laurit]

Javaagent modules remove stuff that is supposed to be in the agent from classpath. Try moving this test to separate module that does not apply javaagent conventions like in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/external-annotations/javaagent-unit-tests

[Oberon00]

Thank you! This was the final hurdle, I came up with a PR demonstrating how this can work: #8603