Shadow jar copies whole springframework and javax in web module

[gavlyukovskiy]

I tried to build web module from sources and found out that the jar contains whole springframework and javax which leads to class collision and makes it impossible to run on newer versions of spring and/or javax - there is NoSuchField or NoSuchMethod and application don't start.

Please reconsider your usage of repackaging libraries, while it gives some benefits for mostly utility libraries that may break backward compatibility (e.g. Guava), it just won't work if you try to expose some of the code to the user. For example repackaging javax.annotations into com.microsoft.applicationinsights.core.dependencies.javaxannotation in core module is meaningless because usage of com.microsoft.applicationinsights.core.dependencies.javaxannotation.PostConstruct is not the same as javax.annotation.PostConstruct, it is not specified and won't be taken into account by any application server. Same applies on extending javax.servlet.Filter - if you do repackage of javax this filter won't be longer accepted by servlet engine.

As well it is very unusual (correct me if I'm wrong) to use shadow jar in libraries. Thus I'd recommend to reduce (or even remove) usage of shadow jar and include only libraries for which it is strongly justified. Personally I wouldn't repackage apache commons/httpclient because they cares about backward compatibility. As well sometimes we enable debug logging for package org.apache.http.client to see all outgoing request information and it would be an interesting finding that we also had to enable debug logging for com.microsoft.applicationinsights.core.dependencies.http.client.

[dhaval24]

@gavlyukovskiy did you tried using #481 . The current master shadow jar repackaging is broken for Web package and we found that it repackages every thing including servlet and spring framework which is not needed at all. #481 is not merged yet but it is designed to fix that problem (at least the intent was that).

Further, about your question on repackaging Apache Http Client : Here is the use case for it. Our SDK makes network call to end points using apache http. The Java Agent of application insights is used to capture the Http Dependencies of Apache http client and if we don't repackage it we end up capturing our own dependencies which is not desired.

I think your comment on repackaging javax annotation and servlet is also valid. I will look into this before we release this version. This could casuse critical problems. In the mean while could you please give a shot by building from #481 and see the effect. It would be great help in properly fixing this issue.

@grlima @littleaj we need to take a look at this.

[littleaj]

@gavlyukovskiy, As @dhaval24 said, this issue should be resolved by PR #481. You are correct, javax.annotations.* should not be repackaged and should be provided by the target application or the application server. The same goes for the springframework and other dependencies of the SDK marked as 'provided' in the build script.

For our records, what application server were you using when you discovered this? Java Version? SDK version? versions of relevant dependencies?

Regarding the shadowing of apache http client, this is a necessity due to our agent instrumentation.

[gavlyukovskiy]

For our records, what application server were you using when you discovered this? Java Version? SDK version? versions of relevant dependencies?

I'm using JDK 8 x64 1.8.0_151 and spring boot 1.5.8 with embedded tomcat of version 8.5.23 which requires servlet-api of version 3.1 at least.
I assume if failed due to the fact that application insights contains servlet-api of version 2.5.

I also tried with #481 fix and it seems to be working fine now, thanks. Though I think that assuming your requirements it is better to have repackaged only http client and guava. Guava doesn't care about backward compatibility at all and releases new major version with unnecessary breaking changes every half of a year. It's dependency hell when you have two libraries that depend on different version of guava, it is so bad that many library maintainers just went away of using this library at all. With all the rest, like apache commons, you can use provided as well.

[dhaval24]

@littleaj @grlima what do you think about this? I think @gavlyukovskiy point seems to be valid. In this way we can reduce our dependency repackaging also. Though agent jars need to repackage ASM also as we are doing currently but for others we might think of reducing the repackaging by using provided as @gavlyukovskiy said except for httpclient and guava.

Thanks for your inputs @gavlyukovskiy towards making sdk better.

[dhaval24]

@littleaj this is fixed right ?

[dhaval24]

@gavlyukovskiy have you tried the latest release of the sdk? I think the issue no longer persist. Let me know your experience so I can close this

[gavlyukovskiy]

Looks fine on latest master.