-
Notifications
You must be signed in to change notification settings - Fork 8
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
build(shade): shade and relocate dependencies to avoid conflicts #224
Conversation
For reference later, DataDog's Java agent does some similar stuff with slf4j, though it's a Gradle build: Similar with NewRelic: |
@ebaron I think this is mostly done. There could still be more work to do to clean up the SLF4J stuff, but I'm wary of over-investing time into this now when I think the current state will already (mostly?) solve the original user report that led to this work. If I have time in the release window I may continue looking at this but for now I think it's enough. |
@Josh-Matsuoka if any of this shaded JAR stuff looks at all familiar, please do give it a readover and let me know if there is anything I missed. Otherwise I would appreciate if you could also verify building it and testing it with the two sample applications and check that it works for you as well. |
Not entirely familiar with the shaded jar stuff but from what I understand it looks fine. Verified that both sample applications work nicely. |
… as embedded log provider
…gent and deps don't touch the application's logging framework
f10cfe1
to
d53f880
Compare
I'll wait a while longer and see if @ebaron will have time for review too, since this is a fairly substantial change to the way that the artifact gets built and distributed. |
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 looks fine to me 👍
(cherry picked from commit a497104)
… (#235) (cherry picked from commit a497104) Co-authored-by: Andrew Azores <[email protected]>
Related to #223
Fixes #213
Closes #214
This shades and relocates dependencies to minimize the risk that the Agent JAR conflicts with the application it is being loaded into.
SmallRye Config is not (yet) relocated because I've been having a hard time getting that to work, it seems to do a lot of runtime dynamic lookups and my shade plugin configuration knowledge is failing here.I was also hoping to come up with a solution to #213 here where an slf4j implementation could be shaded and relocated in so that the compile target is slf4j-api but both the API and implementation would be hidden away from the target application's classpath, but I haven't solved that yet either. The other solution I have been trying to chase is to get slf4j working in the usual way, where the Agent only depends on and compiles against slf4j-api, and then picks up a specific provider at runtime, but it seems that the classloading order breaks this - the Agent JAR is loaded first, containing only slf4j-api, and cannot locate any logging providers, so it auto-configures the no-op logging provider. Then the rest of the classpath is loaded, and if there is any other slf4j provider there, it is too late for the Agent to pick up and use.
As an interim "solution" for the slf4j problem above, this PR includes
slf4j-api
as the API compilation target andslf4j-simple
as the provider, both included in the shaded JAR. The SimpleLogger provider just writes to System.err and I included asimplelogger.properties
file to configure it to show a date/time stamp. It's possible for the end user to further configure this by overriding system properties on the logger even after initialization so that subsequent messages are written to System.out or to a local file, but this is not a proper slf4j facade that allows full configuration to write out to a different logging framework like log4j/logback/etc. But at the least, it should not cause classloading conflicts at runtime or other strangeness. It does mean that slf4j JARs may be found at runtime which may conflict with other logging libraries - see my Spring Petclinic fork, for example, which has to explicitly exclude a Spring logging component to avoid a classloading error at boot.To test, this should use the very latest
cryostat-core
due to cryostatio/cryostat-core#268 :Check out
cryostat-core
andmvn install
it, then build the Agent as of this PR:Then build and run sample applications with the PR agent:
quarkus-test
$ gh repo clone https://github.com/andrewazores/quarkus-test $ ./build.bash && ./run.bash
Ensure that the quarkus-test server comes up with output like:
spring-petclinic
$ gh repo clone https://github.com/andrewazores/spring-petclinic $ ./build.bash && ./run.bash
Ensure that the Spring server comes up with output like:
Cryostat Smoketest
After building the
quarkus-test
sample application, the usual Cryostatsmoketest.sh
should also use that rebuilt image. Use ex.podman logs -f quarkus-test-agent-1
and ensure that this has logging output from the Agent that indicates it is working as usual.