Add SPI to configure additional bootstrap package prefixes #1380
Conversation
pavolloffay
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
| * The instrumentation does not have access to the installer, therefore this utility class is used | ||
| * to share package prefixes. | ||
| */ | ||
| public class BootstrapPackagePrefixesHolder { |
There was a problem hiding this comment.
I could get around without introducing this class. In the AgentInstaller I have set the package prefixes to the config (in the static block) and then used the config in the classloader instrumentation. We could also remove the SPI so vendors would set the packages in the config - I don't like this since it easily opens the config to the consumers.
There was a problem hiding this comment.
can this go in javaagent-bootstrap near HelperResources which serves somewhat similar purpose?
There was a problem hiding this comment.
When the class is in javaagent-bootstrap (and added to helper class names in the classloader instrumentation) the set prefixes set by the AgentInstaller are always null - the agent installer has a different instance as the instrumentation.
Loading the providers from the instrumentation fails at build time on the muzzle size limit.
There was a problem hiding this comment.
I have moved it to internal package in the same module.
| * The instrumentation does not have access to the installer, therefore this utility class is used | ||
| * to share package prefixes. | ||
| */ | ||
| public class BootstrapPackagePrefixesHolder { |
There was a problem hiding this comment.
can this go in javaagent-bootstrap near HelperResources which serves somewhat similar purpose?
...rc/main/java/io/opentelemetry/instrumentation/api/config/BootstrapPackagePrefixesHolder.java
Outdated
Show resolved
Hide resolved
| @@ -88,7 +89,7 @@ public ClassLoaderInstrumentation() { | |||
| return null; | |||
| } | |||
| try { | |||
| for (String prefix : Constants.BOOTSTRAP_PACKAGE_PREFIXES) { | |||
| for (String prefix : BootstrapPackagePrefixesHolder.getBootstrapPrefixes()) { | |||
There was a problem hiding this comment.
Is Config not available here? If it is, we can just use a config property instead of a separate SPI?
There was a problem hiding this comment.
See @pavolloffay's #1380 (comment)
We could also remove the SPI so vendors would set the packages in the config - I don't like this since it easily opens the config to the consumers.
I think I agree, I think nice to encapsulate vendor extension points in SPI, even if we end up with lots of them(?)
There was a problem hiding this comment.
Sorry for missing the comment! SGTM
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java
Outdated
Show resolved
Hide resolved
| return bootstrapPrefixes; | ||
| } | ||
|
|
||
| public static void setBootstrapPrefixes(String[] prefixes) { |
There was a problem hiding this comment.
Can you make this class more similar to Config/OpenTelemetrySdkAccess? I.e. volatile static field, internal... setter method name, checking that you can only set the value once.
| private static String[] bootstrapPrefixes; | ||
|
|
||
| public static String[] getBootstrapPrefixes() { | ||
| return bootstrapPrefixes; |
There was a problem hiding this comment.
If it's supposed to be available in instrumentation-api consider using a List (and Collections.unmodifiableList() wrapper): arrays are mutable and can be easily changed by anybody.
This feature is useful when a large set of custom instrumentations is using common classes from a custom package. Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
pavolloffay
force-pushed
the
bootstrap-prefix
branch
from
8d72f87
to
4184885
Compare
|
@mateuszrzeszutek I have updated the PR |
| return BOOSTRAP_PACKAGE_PREFIXES; | ||
| } | ||
|
|
||
| public static void setBoostrapPackagePrefixes(List<String> prefixes) { |
There was a problem hiding this comment.
Just a minor comment: how about internalSetBootstrapPackagePrefixes?
|
Closing and re-opening to re-trigger the stuck CLA check |
This feature is useful when a large set of custom instrumentations is
using common classes from a custom package.
cc) @trask
Signed-off-by: Pavol Loffay p.loffay@gmail.com