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

add the sources from apm-agent in to the final sources jar #3063

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

jackshirazi
Copy link
Contributor

@jackshirazi jackshirazi commented Mar 21, 2023

What does this PR do?

In 1.27.0 we restructured the subprojects so that all the main agent dependency management was moved from elastic-apm-agent to apm-agent, with no direct dependency on apm-agent from elastic-apm-agent. Instead it was made into a shaded dependency. This was to make the agent more isolated to reduce conflicts in classloading related to agent classes. An unnoticed side-effect of this was that the sources jar in elastic-apm-agent no longer contained the full agent sources.

To fix this, I tried a number of different configurations. Any configuration that directly added the apm-agent as a dependency caused the class jar file to contain the classes as well as the shaded classes, so those configurations failed. The shading plugin should have been able to create all the sources of the shaded dependencies, but although I tried really hard I couldn't find the magic combo that produced that. So I've gone with the brute force approach of just merging the sources from the apm-agent sources jar into the elastic-apm-agent sources jar. This has the advantage of being very simple, but the disadvantage of directly depending on a source jar file existing in another project.

Additionally this PR fixes the missing co.elastic.apm.agent.configuration -> co.elastic.apm.agent.premain.configuration that @JonasKunz noticed

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else

@apmmachine
Copy link
Contributor

apmmachine commented Mar 21, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-03-21T16:45:05.059+0000

  • Duration: 58 min 30 sec

Test stats 🧪

Test Results
Failed 0
Passed 3569
Skipped 121
Total 3690

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)


<target name="merge-sources" description="Merge source jars">
<zip destfile="${project.basedir}/target/${project.artifactId}-${project.version}-sources.jar">
<zipgroupfileset dir="${project.basedir}/../apm-agent/target/" includes="apm-agent-*-sources.jar"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me, I do however have an idea on how to get rid of the direct apm-agent file based reference:

  • use the maven-dependency-plugin execution in this pom above to explicitly copy the apm-agent sources JAR into the target folder (using the copy goal )
  • use the copied sources-jar from the target folder instead of the file path ${project.basedir}/../apm-agent/target/

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that a slightly different config, it requires the dependency to be declared as such which then would be included in the class files. Then I tried to exclude the dependencies after including them, then they don't show in the sources. So if I then do that and then copy the source jar, then do the merge it might work - but sounds like a very complex addition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think I even tried it and couldn't get the sources if the classes were excluded, and couldn't prevent the classes from being the final class jar if they weren't even when I tried to exclude them also using the shade target. It was a really painful week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice change, thank you! Much better

@jackshirazi jackshirazi enabled auto-merge (squash) March 21, 2023 15:38
@jackshirazi jackshirazi disabled auto-merge March 21, 2023 16:43
@jackshirazi jackshirazi enabled auto-merge (squash) March 21, 2023 16:45
@jackshirazi jackshirazi merged commit de4f3c8 into elastic:main Mar 21, 2023
@jackshirazi jackshirazi linked an issue Mar 22, 2023 that may be closed by this pull request
@SylvainJuge SylvainJuge mentioned this pull request Apr 12, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix source jar created on Jenkins build
3 participants