Skip to content
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

Embedded extension #3237

Merged
merged 18 commits into from
Jun 14, 2021
Merged

Embedded extension #3237

merged 18 commits into from
Jun 14, 2021

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Jun 10, 2021

Builds on top of #3226 allowing to embed extension jar right inside javaagent jar for simplified deployment.

@anuraaga can you take a look at examples/extension/src/test/java/com/example/javaagent/smoketest/SpringBootIntegrationTest.java? It often time fails for me. Looks like later tests continue receiving spans from the previous tests. /clear-requests on fake backend does not work as expected or?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iNikem Do you see the flakiness only locally or also on CI? Either way can you file an issue?

@iNikem
Copy link
Contributor Author

iNikem commented Jun 10, 2021

@iNikem Do you see the flakiness only locally or also on CI? Either way can you file an issue?

We don't run examples' tests on CI. So only locally. Will file

@anuraaga
Copy link
Contributor

@iNikem I didn't confirm the fake backend image referenced in the examples smoketest is new enough for it, but we changed the path to /clear at some point, that might be the problem.

@iNikem
Copy link
Contributor Author

iNikem commented Jun 11, 2021

@iNikem I didn't confirm the fake backend image referenced in the examples smoketest is new enough for it, but we changed the path to /clear at some point, that might be the problem.

Yes, that's it! Thank you! Fixed now.

@@ -86,8 +136,13 @@ private static void addFileUrl(List<URL> result, File file) {
}
}

private static URLClassLoader getDelegate(ClassLoader parent, URL extensionUrl) {
return new ExtensionClassLoader(new URL[] {extensionUrl}, parent);
private static void extractFile(JarFile jarFile, JarEntry jarEntry, File outputFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outputDir since we're extracting a jar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that parameter is actually a file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops - so the output file is a JAR file itself. extractFile made me think the embedded JAR itself is getting extracted into its contents. I don't think unpackFile had the same impression for me so may be a clearer name for people like me

https://github.com/spring-projects/spring-boot/blob/v2.4.3/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/archive/JarFileArchive.java#L140

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We extract a file from jarEntry into an outputFile. Like "extract a file from a jar". How can this translate to a double extraction? :) But I can rename if you wish, no problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two "Jar" variables there :) My brain processed it as extracting the second one. Maybe it's silly, so don't mind either way

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possibility to read directly from the embedded jar? Otherwise we limit possibility to run on read-only file system, a request we've gotten, and @tylerbenson has mentioned previously that they support.

@iNikem
Copy link
Contributor Author

iNikem commented Jun 14, 2021

Is there any possibility to read directly from the embedded jar? Otherwise we limit possibility to run on read-only file system, a request we've gotten, and @tylerbenson has mentioned previously that they support.

I did not find it on the first try. But I don't think this is an actual blocker: on read-only system one can use external extensions jar. I can file a follow-up issue for that.

@anuraaga
Copy link
Contributor

FWIW, spring-boot has been extracting embedded JARs to file system for a loong time so it's tried and true :)

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests.

FWIW, spring-boot has been extracting embedded JARs to file system

I assumed that spring-boot used a class loader to read the embedded jars directly, given the requirement that the embedded jars have to be uncompressed?

examples/extension/build.gradle Outdated Show resolved Hide resolved
Comment on lines +55 to +56
File javaagentFile = installBootstrapJar(inst);
AgentInitializer.initialize(inst, javaagentFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines -84 to +71
constructor.newInstance(bootstrapUrl, innerJarFilename, agentParent);
new AgentClassLoader(javaagentFile, innerJarFilename, agentParent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trask trask merged commit b9eac53 into open-telemetry:main Jun 14, 2021
@iNikem iNikem deleted the embedded-extension branch June 15, 2021 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants