Move jaeger-thrift shadow jar to its own classifier

[jpkrohling]

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Which problem is this PR solving?

• By default, libraries usually provide non-shadow JARs, unless explicitly specified via, for instance, Maven classifiers. We thought that providing a shadowed version by default would help most users, but this is getting more complicated that it deserves.
• Having the shadow by default makes it harder for our users to assess what's running and at which version.

Short description of the changes

• Revert the relevant parts of Make jaeger-thrift's shaded JAR the default one #461

Also related: #486, #494 / #498

cc @mdvorak

[codecov]

Codecov Report

Merging #526 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #526     +/-   ##
===========================================
+ Coverage     88.42%   88.52%   +0.1%     
  Complexity      505      505             
===========================================
  Files            66       66             
  Lines          1883     1883             
  Branches        239      239             
===========================================
+ Hits           1665     1667      +2     
+ Misses          142      140      -2     
  Partials         76       76

Impacted Files	Coverage Δ	Complexity Δ
...egertracing/internal/reporters/RemoteReporter.java	85.71% <0%> (+2.38%)	7% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6060f4b...02252d9. Read the comment docs.

[pavolloffay]

+1.

Could you please briefly summarize what problems shaded jars introduce?

[objectiser]

LGTM - although I would prefer we find out the reason the shadow jar was created in the first place, and whether that use case still exists.

@yurishkuro Do you recall the reason for needing the shaded jar?

[objectiser]

"A non-shaded version.." - I think that should be "A shaded version..."

[jpkrohling]

Fixed

[jpkrohling]

although I would prefer we find out the reason the shadow jar was created in the first place, and whether that use case still exists.

We created the shadow because we were using a very old version of Thrift and conflicted with the version of Thrift people were using. See #228 as one example. For this use case, people can still use the shaded jar.

[jpkrohling]

Could you please briefly summarize what problems shaded jars introduce?

It's basically what we have in the description: shadow, by definition, hides our dependencies. It bloats the jar and makes security audits harder, for instance.

[pavolloffay]

shaded by default was added here #460.

[objectiser]

@jpkrohling That is the point - we were using an older version of thrift, which is no longer the case - therefore the reason for having the shaded jar no longer exists.

[yurishkuro]

The reason is the same, we don't know which version of thrift the end user application is using and thrift versions in Java are highly incompatible even between patches.

TBH the amount of effort we spent on this thrif & shade issue would've been better spent by just writing serialization manually.

[jpkrohling]

@objectiser sorry, I see now what you mean. Looks like it was done as part of #80 (PR #83).

cc @vprithvi

[jpkrohling]

TBH the amount of effort we spent on this thrif & shade issue would've been better spent by just writing serialization manually.

@pavolloffay said the exact same thing to me via IRC, but I don't have the time to do it now. It should be part of #516 though.

That said: is this PR good to be merged? Or should I remove the shadow artifact altogether?

[vprithvi]

For more context around #80: we ran into an issue where span reporting was broken silently when the project overrode the version of thrift to 0.9.3. I'm not sure if that is still an issue.

Because of the prevalence of thrift, I am concerned that removing default shading could cause more problems than it solved. (On the other hand, more experienced users can use a the shade/shadow plugin to relocate the offending classes themselves.)

[jpkrohling]

I am concerned that removing default shading could cause more problems than it solved

By "default shading", did you mean not providing a shaded version at all, or did you mean providing it as the main artifact? Right now, the shaded version is the main artifact for jaeger-thrift and we want to revert that (added recently, as part of #461).

On the other hand, more experienced users can use a the shade/shadow plugin to relocate the offending classes themselves

+1

[objectiser]

That said: is this PR good to be merged? Or should I remove the shadow artifact altogether?

@jpkrohling I'm fine either way, was just looking at an opportunity to simplify.

[jpkrohling]

Based on the comments, I understand that this PR looks good, as we don't want to remove the shadow artifact. As such, I'm merging this.