Manifest version parsing is not working in Wildfly

[mdindoffer]

Manifest version resolution introduced in #558 does not work in Widlfly.
The jarUrl.getFile() returns file:/home/wildfly-9.0.2.Final/modules/system/layers/base/org/apache/httpcomponents/main/httpclient-4.3.6.jar!/
The path contains an exclamation mark, probably because the full URL is jar:file:/home/wildfly-9.0.2.Final/modules/system/layers/base/org/apache/httpcomponents/main/httpclient-4.3.6.jar!/, i.e. includes the jar: protocol. This breaks the JarFile creation.

Stack Trace

co.elastic.apm.agent.bci.bytebuddy.CustomElementMatchers - Cannot read implementation version based on ProtectionDomain ProtectionDomain (jar:file:/home/wildfly-9.0.2.Final/modules/system/layers/base/org/apache/httpcomponents/main/httpclient-4.3.6.jar!/ <no signer certificates>)
null
<no principals>
java.security.Permissions@4994c74d (
("java.security.AllPermission" "<all permissions>" "<all actions>")
)

java.io.FileNotFoundException: file:/home/wildfly-9.0.2.Final/modules/system/layers/base/org/apache/httpcomponents/main/httpclient-4.3.6.jar! (No such file or directory)
at java.util.zip.ZipFile.open(Native Method)
at java.util.zip.ZipFile.<init>(ZipFile.java:225)
at java.util.zip.ZipFile.<init>(ZipFile.java:155)
at java.util.jar.JarFile.<init>(JarFile.java:166)
at java.util.jar.JarFile.<init>(JarFile.java:103)
at co.elastic.apm.agent.bci.bytebuddy.CustomElementMatchers.readImplementationVersionFromManifest(CustomElementMatchers.java:144)
at co.elastic.apm.agent.bci.bytebuddy.CustomElementMatchers.access$100(CustomElementMatchers.java:42)
at co.elastic.apm.agent.bci.bytebuddy.CustomElementMatchers$2.matches(CustomElementMatchers.java:126)
at co.elastic.apm.agent.bci.bytebuddy.CustomElementMatchers$2.matches(CustomElementMatchers.java:123)
at co.elastic.apm.agent.bci.ElasticApmAgent$4.matches(ElasticApmAgent.java:187)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$Transformation$Simple.matches(AgentBuilder.java:9983)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$Transformation$Simple.resolve(AgentBuilder.java:9996)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$Transformation$Compound.resolve(AgentBuilder.java:10252)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.resolve(AgentBuilder.java:10584)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.doTransform(AgentBuilder.java:10551)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.transform(AgentBuilder.java:10514)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.access$1500(AgentBuilder.java:10280)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer$LegacyVmDispatcher.run(AgentBuilder.java:10889)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer$LegacyVmDispatcher.run(AgentBuilder.java:10836)
at java.security.AccessController.doPrivileged(Native Method)
at co.elastic.apm.agent.shaded.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.transform(AgentBuilder.java:10437)
at sun.instrument.TransformerManager.transform(TransformerManager.java:188)
at sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428)
at java.lang.ClassLoader.defineClass1(Native Method)
at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
at org.jboss.modules.ModuleClassLoader.doDefineOrLoadClass(ModuleClassLoader.java:353)
at org.jboss.modules.ModuleClassLoader.defineClass(ModuleClassLoader.java:432)
at org.jboss.modules.ModuleClassLoader.loadClassLocal(ModuleClassLoader.java:269)
at org.jboss.modules.ModuleClassLoader$1.loadClassLocal(ModuleClassLoader.java:77)
at org.jboss.modules.Module.loadModuleClass(Module.java:560)
at org.jboss.modules.ModuleClassLoader.findClass(ModuleClassLoader.java:197)
at org.jboss.modules.ConcurrentClassLoader.performLoadClassUnchecked(ConcurrentClassLoader.java:455)
at org.jboss.modules.ConcurrentClassLoader.performLoadClassChecked(ConcurrentClassLoader.java:404)
at org.jboss.modules.ConcurrentClassLoader.performLoadClass(ConcurrentClassLoader.java:385)
at org.jboss.modules.ConcurrentClassLoader.loadClass(ConcurrentClassLoader.java:130)
at org.apache.http.impl.client.AbstractHttpClient.getRedirectStrategy(AbstractHttpClient.java:575)
at org.apache.http.impl.client.AbstractHttpClient.doExecute(AbstractHttpClient.java:810)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:57)
at org.jboss.resteasy.client.jaxrs.engines.ApacheHttpClient4Engine.invoke(ApacheHttpClient4Engine.java:283)
at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.invoke(ClientInvocation.java:436)
at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invoke(ClientInvoker.java:102)
at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientProxy.invoke(ClientProxy.java:76)
at com.sun.proxy.$Proxy294.get(Unknown Source)
e.t.c...

Environment
Wildfly 9.0.2.Final with provided httpclient 4.3.6

[mdindoffer]

Also, shouldn't we close the JarFile after the version parsing?

[felixbarny]

@eyalkoren Couldn't we just remove the version check here:

apm-agent-java/apm-agent-plugins/apm-apache-httpclient-plugin/src/main/java/co/elastic/apm/agent/httpclient/ApacheHttpAsyncClientRedirectInstrumentation.java

Lines 84 to 86 in f51f253

	public ElementMatcher.Junction<ProtectionDomain> getImplementationVersionPostFilter() {
	return implementationVersionLte("4.3.2");
	}

It seems like the header re-adding is idempotent:

apm-agent-java/apm-agent-plugins/apm-apache-httpclient-plugin/src/main/java/co/elastic/apm/agent/httpclient/ApacheHttpAsyncClientRedirectInstrumentation.java

Lines 55 to 60 in f51f253

	if (original.containsHeader(TraceContext.TRACE_PARENT_HEADER) && !redirect.containsHeader(TraceContext.TRACE_PARENT_HEADER)) {
	String traceContext = original.getFirstHeader(TraceContext.TRACE_PARENT_HEADER).getValue();
	if (traceContext != null) {
	redirect.setHeader(TraceContext.TRACE_PARENT_HEADER, traceContext);
	}
	}

Is the sole reason for the version check to reduce the overhead of the containsHeader check and to avoid the Iterator allocation in versions pre 4.0.1? Am I missing something?

@mdindoffer thanks for all the reports, really appreciated ❤️

[eyalkoren]

@mdindoffer Thanks for this useful input! I made a fix and will link a snapshot to test shortly.

@felixbarny the check is indeed for performance reasons in this case, but I would say we should always strive to instrument as little as possible even when the performance hit is not huge. In addition, I DO see value in this matcher, I know I would use it before if had it available. So, for now I will add a fix and if we see we can't get it robust enough with little effort, we can remove this matcher altogether.

[eyalkoren]

Snapshot jar for download

[mdindoffer]

@eyalkoren Yep, your fix works. Thanks.