-
Notifications
You must be signed in to change notification settings - Fork 870
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
Remove generated InstrumentationModule#getMuzzleReferences() method from the source code #4087
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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.javaagent.tooling.muzzle; | ||
|
||
import static java.util.Collections.emptyMap; | ||
|
||
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; | ||
import io.opentelemetry.javaagent.extension.muzzle.ClassRef; | ||
import java.lang.invoke.MethodHandle; | ||
import java.lang.invoke.MethodHandles; | ||
import java.lang.invoke.MethodType; | ||
import java.util.Map; | ||
|
||
/** | ||
* This class is used to access {@code getMuzzleReferences} method from the {@link | ||
* InstrumentationModule} class. That method is not visible in the source code and instead is | ||
* generated automatically during compilation by the {@code | ||
* io.opentelemetry.instrumentation.javaagent-codegen} Gradle plugin. | ||
*/ | ||
class MuzzleReferencesAccessor { | ||
private static final ClassValue<MethodHandle> getMuzzleReferences = | ||
new ClassValue<MethodHandle>() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I think of it we don't really need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure? Will be happy to remove this cache! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep -- the only place ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I will try Lauri idea from #4087 (comment) anyway later on. That will probably remove this whole class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That one is only used in the gradle muzzle-check plugin, it doesn't affect the agent at all. |
||
@Override | ||
protected MethodHandle computeValue(Class<?> type) { | ||
MethodHandles.Lookup lookup = MethodHandles.publicLookup(); | ||
MethodHandle handle; | ||
try { | ||
// This method is generated automatically during compilation by | ||
// the io.opentelemetry.instrumentation.javaagent-codegen Gradle plugin. | ||
handle = | ||
lookup.findVirtual(type, "getMuzzleReferences", MethodType.methodType(Map.class)); | ||
} catch (NoSuchMethodException | IllegalAccessException e) { | ||
handle = null; | ||
} | ||
return handle; | ||
} | ||
}; | ||
|
||
/** | ||
* Returns references to helper and library classes used in the given module's type | ||
* instrumentation advices, grouped by {@link ClassRef#getClassName()}. | ||
*/ | ||
@SuppressWarnings("unchecked") | ||
static Map<String, ClassRef> getFor(InstrumentationModule instrumentationModule) { | ||
Map<String, ClassRef> muzzleReferences = emptyMap(); | ||
MethodHandle methodHandle = getMuzzleReferences.get(instrumentationModule.getClass()); | ||
if (methodHandle != null) { | ||
try { | ||
muzzleReferences = (Map<String, ClassRef>) methodHandle.invoke(instrumentationModule); | ||
} catch (Throwable ignored) { | ||
// silence error prone | ||
} | ||
} | ||
return muzzleReferences; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
import static java.util.Collections.singletonList; | ||
|
||
import io.opentelemetry.instrumentation.api.caching.Cache; | ||
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; | ||
import io.opentelemetry.javaagent.extension.muzzle.ClassRef; | ||
import io.opentelemetry.javaagent.extension.muzzle.FieldRef; | ||
import io.opentelemetry.javaagent.extension.muzzle.Flag; | ||
|
@@ -36,7 +37,14 @@ public final class ReferenceMatcher { | |
private final Set<String> helperClassNames; | ||
private final InstrumentationClassPredicate instrumentationClassPredicate; | ||
|
||
public ReferenceMatcher( | ||
public static ReferenceMatcher of(InstrumentationModule instrumentationModule) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
return new ReferenceMatcher( | ||
instrumentationModule.getMuzzleHelperClassNames(), | ||
MuzzleReferencesAccessor.getFor(instrumentationModule), | ||
instrumentationModule::isHelperClass); | ||
} | ||
|
||
ReferenceMatcher( | ||
List<String> helperClassNames, | ||
Map<String, ClassRef> references, | ||
Predicate<String> libraryInstrumentationPredicate) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
javaagent-codegen
should add an interface that hasgetMuzzleReferences
to instrumentation module so you wouldn't need to use reflection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that MethodHandle has no performance overhead on invocation. But I will think about this idea, it may be more elegant indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try this idea as a follow-up optimisation. I like it, but prefer to finish current iteration of removing ClassRef and friends from public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also be easier to extend with other muzzle-generated methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is possible to build extension jars that contain instrumentation modules then all of the muzzle and codegen stuff has become part of our api. Have you thought about how to version it and how to handle cases where agent is incompatible with codegen version used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, good question... The only vague thought I have atm is "javaagent must support all previous stable versions of codegen within any given major version." Which assumes "in sync" versioning of the agent and gradle plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no, this is a great point, so maybe we shouldn't get rid of these classes from our public API? 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classes should be part of the contract between muzzle generation and muzzle verification. But no end-user, including extension authors, should see them.