jaegertracing / jaeger-client-java

🛑 This library is DEPRECATED!
https://jaegertracing.io/
Other
491 stars 232 forks source link

Drop Java 6 Compatibility #511

Closed isaachier closed 3 years ago

isaachier commented 6 years ago

Requirement - what kind of business use case are you trying to solve?

Allow use of Java 8 and above in client. This would simplify some of the code.

Problem - what in Jaeger blocks you from solving the requirement?

For some reason, we preserve Java 6 compatibility in the client. @felixbarny for why this was added last year when Java 6 is basically deprecated/ancient technology.

Proposal - what do you suggest to solve the problem or improve the existing situation?

Stop supporting Java 6 in future releases.

black-adder commented 6 years ago

This would simplify some of the code - can you quantify? As an infra platform, we should support as many versions as possible.

jpkrohling commented 6 years ago

Java 6 is EOL for years now. I vote for dropping 1.6 support.

can you quantify?

Lambdas and default methods on interfaces are things that I miss most, from this library's perspective.

As an infra platform, we should support as many versions as possible.

IMO, it's only a real problem if we introduce breaking changes in the agent/collector that would require a bump on client versions. Otherwise, people stuck with 1.6 can just keep using an old client version (just like they keep using their old Java version).

black-adder commented 6 years ago

Ok, according to this, java 6 does seem to be on the way out so I can see us dropping it, although I'd like a really good reason for doing so.

Allow use of Java 8 and above in client

Does this mean people using java 7 will be out of luck?

vprithvi commented 6 years ago

Another alternative is evaluating a library like retrolambda, which backports lambdas to Java 6.

isaachier commented 6 years ago

Sure @black-adder:

Update:

black-adder commented 6 years ago

ok sure, but it'd have to be a breaking change to prevent users of java 6 accidentally pulling a non-compatible client version.

I like prithvi's idea and would like to explore it.

isaachier commented 6 years ago

The real question is why we added this in the first place. IDK exactly how you quantify a breaking change for a language we never really claimed to support. @felixbarny added this support in #132 (original issue #129). That's why I'd like to see if he still needs this.

black-adder commented 6 years ago

never really claimed to support

The fact that we landed the change means we support it. We have to deal with it now and I'd prefer not do a breaking change.

isaachier commented 6 years ago

Ya I get it. Bottom line: we should get rid of it in the next breaking release.

black-adder commented 6 years ago

I added this to this week's Jaeger biweekly meeting agenda, should be some riveting stuff.

felixbarny commented 6 years ago

I’m not affiliated with stagemonitor anymore. @liebhaeuser do you still require Java 6 compatibly for Jaeger/stagemonitor?

isaachier commented 6 years ago

Thanks @felixbarny for following up.

jpkrohling commented 6 years ago

If we are aware that someone needs Java 6, then we should definitely not break them. Looks like stagemonitor still uses Java 6:

https://github.com/stagemonitor/stagemonitor/blob/eca3b0cd87b7e11b3ee36d8db144cdb2b1900f57/build.gradle#L113-L114

Another alternative is evaluating a library like retrolambda, which backports lambdas to Java 6.

While lambdas are really helpful, I'm not sure I would add a bytecode transformation tool just to make use of that.

Does this mean people using java 7 will be out of luck?

Java 7 is EOL since 2015. Just to be clear: EOL means that ordinary people do not have any support from Oracle and bugs are not being fixed, not even security ones. So, people should really not be using anything less than Java 8.

it'd have to be a breaking change to prevent users of java 6 accidentally pulling a non-compatible client version

I assume that people using Java 6 are doing so because their code base is old and they can't afford to simply use a newer JVM version. In that case, I can really see them also not touching Jaeger Client Java's version. StageMonitor, for instance, is still on Jaeger Client Java v0.24.0, released in Feb this year:

https://github.com/stagemonitor/stagemonitor/blob/eca3b0cd87b7e11b3ee36d8db144cdb2b1900f57/build.gradle#L59

I cannot see how one would "accidentally" pull a newer Jaeger Client Java version without actively bumping it on Gradle/Maven. And once they do, it should become apparent very quickly that they pulled a version that isn't supported on their environment. We might need to do some testing, but I think their code might not even compile or the tests won't run, due to incompatible class file format version.

yurishkuro commented 6 years ago

To my memory, stagemonitor was indeed the only project that requested support for 6 in both OpenTracing and Jaeger. This may have been two years ago, when 6 might have been slightly more likely to be found in production than today. I agree that we could move on, but only to 7 as min, as 7 is still quite wildly used. However, I don't think 7 gives us any significant advantages.

isaachier commented 6 years ago

At least it gives diamond operators (always thought that was Java 8). Anyway, stagemonitor claims to support Java 6 even today, so I agree it is probably a breaking change. Practically, it isn't a huge deal, just a matter of preference and why not if we can. I realize we can't until the next major version is released.

codefromthecrypt commented 6 years ago

I will help :)

agents and not always agents, but anyway agents like to cast widest net. stage monitor was the only agent to interact with OT/Jaeger at the time. you can ask same question to others who have agents such as Fabian or others at instana.

this issue is likely more important when the agent uses jaeger directly as opposed to what they often do which is a bridge approach.. anyway someone with agent expericence can help.

definitely do consider retrolambda as it eliminates the diamond and multi catch problem even if not all lambdas can be rewritten back to java 6.

yurishkuro commented 6 years ago

Requirement: Allow use of Java 8 and above in client. This would simplify some of the code.

As a general comment, I am -1 on this being a "requirement". Simplifying the client code (actually more like "pretty-fying") shouldn't come ahead of backwards compatibility and be the reason to break user apps.

According to Won's link, Java 7 is still running quite strong, so we cannot abandon it. And if we're not going to 8+, then going to 7+ does not seem to provide us with a lot of benefits. The main benefit to me would be default methods in interfaces in Java 8 (and even that more on the OpenTracing API side than in Jaeger).

image

isaachier commented 6 years ago

I'm not opposed to maintaining an older version if the only sacrifice is complexity. However, more often than not, what ends up happening is that the entire ecosystem moves away from that support. We end up not being able to rely on libraries that drop support. The support can also come at the expense of performance penalties. I think Java 7 is a good compromise.

In Friday's meeting @jpkrohling and I seemed to agree that since we are < 1.0.0 we can still make backwards-incompatible changes when necessary. @jpkrohling suggested upgrading to Java 8 and seeing what response we get. He has a number of good points, especially about the users of EOL versions tending to use old versions of Jaeger as well, so there is no risk of anything breaking them.

yurishkuro commented 6 years ago

So let's compare:

yurishkuro commented 6 years ago

not being able to rely on libraries that drop support

for example?

yurishkuro commented 6 years ago

btw, regarding things discussed on the call - the highlights from the discussion should be documented, but given how few interested parties are usually present on the call the final decisions should be made publicly on the github issues, either via consensus or majority vote.

isaachier commented 6 years ago

My main counterargument to these Java 6/7 support debates is where do we draw the line? I'm fine if the answer is consistent. For example, why don't we support Java 5, or 4, or even 1? Someone out there is using it!

Java 7 is a reasonable lower bound from my point of view. I just agree with @jpkrohling. Fundamentally, it is easier to ask for forgiveness than permission (as we all know from Python). Sending out a survey won't help, but breaking the code in the newest release would help us identify who out there needs Java 7.

isaachier commented 6 years ago

And yes, next time we ought to document it.

isaachier commented 6 years ago

Last comment for now, in terms of libraries, I don't see any issues yet. Luckily, gRPC still supports even Java 6.

jpkrohling commented 6 years ago

To recap my arguments from the meeting (and earlier in this thread) and better document them:

  1. To me, the main point is agreeing on a policy. Both Java 6 and 7 are EOL for years now (like 5, 4, ...). Except for a couple of vendors charging very high sums, nobody has a supported Java 6/7 JVM, meaning that people running them are running non-secure and non-patched software. In the past, library vendors were supporting an EOL Java 6 because of Android, but that's not the case anymore;
  2. Linked to the above: Java versioning is changing to have more frequent releases, with a few LTS releases in between. Java 8 is one such LTS release. IMO, a good policy would be "we support whatever LTS versions are currently supported";
  3. 30% on Java 6+7 was 15 months ago, according to that blog post. We don't have recent numbers, but it's then at most 30%, potentially far lower;
  4. Gut feeling: people who are using older JVMs tend to stick with older versions of software, so, people will not jump into the latest Jaeger client. They'd use whatever they have right now and never change it (unless they get hit by a bug).
  5. As long as we don't break client -> [agent|collector] compatibility, people can just use an older version of the client. We can keep a branch around for 0.30.x, so that interested parties can backport fixes that they need. Louis-Étienne mentioned that he'd be willing to work with this branch and backport bug fixes that are of interest to him.

On the technical side, we don't have many reasons to move. Our code base isn't that big, so, nothing will really change our lives here. Things that are nice, though: some performance improvements, especially around iterators/looping (by using streams), date/time API, lambdas, ...

With all that, the suggestion is then to bump our build to use Java 8 and release the next minor version (v0.31.0) but not use any Java 7+ specific features for a few months. If we don't hear any complaints by Jan 2019, we can start making use of the new features. Otherwise, we just rollback this change and release new minor (v0.32.0).

yurishkuro commented 6 years ago

Louis-Étienne (@ledor473) - you work for Ticketmaster, correct? This isn't a vendor charging high sums, it's an end user. And I am sure there is plenty of enterprise software out there that is not yet running on Java 8.

Basically, my point is that if we go to Java 8+, we cut off some (unknown, but still sizable) user base. If we only go to Java 7, it doesn't give us a lot of benefits over Java 6, so why bother.

Btw, Zipkin's Brave is on Java 7+.

isaachier commented 6 years ago

Actually I like @jpkrohling's idea of using Java 8 for build purposes, but not any of the 7+ features until enough time has passed to guarantee that the vast majority of potential users have already upgraded unharmed.

isaachier commented 6 years ago

Also, still would love to hear response to this:

why don't we support Java 5, or 4, or even 1?

jpkrohling commented 6 years ago

If we only go to Java 7, it doesn't give us a lot of benefits over Java 6, so why bother.

Until when are we going to support Java 6/7?

ledor473 commented 6 years ago

Yes @yurishkuro I'm working at Ticketmaster. We do have applications that still use Java 6/7. I wouldn't say Java 6 represent a large footprint, but Java 7 is still present. And yes, some of theses applications are instrumented with OT/Jaeger!

I have no doubt that we are not the only one in that situation as Oracle itself still supports Java 6 until Dec 2018 with their Extended Support.

Just to put things in perspective, anything written 4 years ago would have been done in Java 7.

While I'd prefer to see Java 6/7 support, I do understand that it's not ideal to build new features and it makes the development harder. Also, there seems to be a shift in the Java community to support Java 8+. For example, Spring 5 requires it.

I know that eventually Java 6 and 7 support will be dropped (even Java 8 will be old soon), but when that day come I still think that the Jaeger architecture will help make this transition smoother. As @jpkrohling pointed out, keeping the Client -> Agent compatibility will allow older applications to use an older version of the SDK. If there's ever an incompatiblity, we could still run an older Agent for as long as there's no incompatiblity at the Agent -> Collector layer. Ideally we would document that just like Spring documents the JDK support for 4.3 vs 5.x.

yurishkuro commented 6 years ago

Oracle itself still supports Java 6 until Dec 2018 with their Extended Support

Maybe this is a reasonable deadline for us to set as well for dropping Java 6. Still don't know about Java 7.

ledor473 commented 6 years ago

Maybe Java 7 could be dropped at the same time if an "end to end testing" is done (in a separate CI project maybe?) to ensure a minimal compatibility with recent Agent/Collector?

codefromthecrypt commented 6 years ago

Btw, Zipkin's Brave is on Java 7+ https://github.com/openzipkin/brave/blob/e538ea8e8c95aebc0375cdf2d4d0bb1484d617e1/pom.xml#L54 .

this isn't completely correct. The default source level is 7, but the core library is 6 and we don't use levels higher than the library being instrumented. https://github.com/openzipkin/brave/blob/master/instrumentation/dubbo-rpc/pom.xml#L14

If we do... it is by accident and sometimes people tell us

jpkrohling commented 6 years ago

@yurishkuro wrote:

This isn't a vendor charging high sums, it's an end user

I meant that there are only a couple of vendors providing support for the JVM. If @ledor473's company isn't paying one of those vendors, then they are running a non-supported JVM, potentially with non-patched security vulnerabilities.

@ledor473 wrote:

Oracle itself still supports Java 6 until Dec 2018 with their Extended Support.

That's one of the vendors that I mentioned before charging high sums of money to offer commercial support :-) I might be wrong here, but from what I understand, only paying customers are receive this support. Everyone else is running non-patched JVMs.

On the same page, we can see that Java 7 is on the same boat, but one layer before: "premier support", but also means that only paying customers are receiving this support.

Maybe Java 7 could be dropped at the same time if an "end to end testing" is done

I'd still do it in phases: release only Java 8 artifacts from now on (or from Jan 2019) and get people aware that things are changing. Then, 6 months later, we are free to start adopting features available in new versions.

But more importantly, I think we should decide on a policy, especially given that few JVMs from now on will be LTS. For instance, Java 10 was released in March this year and it stops receiving updates in about one month, when Java 11 is released.

yurishkuro commented 6 years ago

I rather like Zipkin Brave's policy: support whichever minimal version is allowed by the frameworks that Brave instruments explicitly (with 1.6 as the lower bound). And from this PR https://github.com/openzipkin/brave/pull/757 there are a lot of popular frameworks that still support 1.6.

jpkrohling commented 6 years ago

support whichever minimal version is allowed by the frameworks that Brave instruments explicitly

The suggestion is then to base our policy on whatever version OpenTracing Java sets as base? I'm fine with that.

jpkrohling commented 6 years ago

I do not want to reopen this debate, but wanted to leave this here for reference:

https://snyk.io/blog/jvm-ecosystem-report-2018

87% is on Java 8 or more recent, 9% on Java 7, 3% are on Java 6 or lower and 1% don't know (sample size > 10,200).

pavolloffay commented 6 years ago

Even java 11 is out (LTS support after java 8)

jkwatson commented 3 years ago

Does jaeger even work with java 6 any more? I can't use the thrift library for java 6, as it depends on okhttp which doesn't work with java 6. Is there a known configuration that will work for jaeger, the thrift protocol and java 6?

yurishkuro commented 3 years ago

ha, what a walk down the memory lane this ticket is...

According to https://snyk.io/blog/developers-dont-want-to-leave-java-8-as-64-hold-firm-on-their-preferred-release/, 3% of users are on Java 7 or lower. I think at this point we should be ok to go to 8, and let the holdouts use the older SDK versions (plus, we're not really building much into Java SDK these days, so they won't be missing a lot).

william-tran commented 3 years ago

I have concurrency improvements in an internal fork that are less than trivial to backport to java 6, see https://github.com/jaegertracing/jaeger-client-java/pull/609#issuecomment-664465901. Would be great to see java 8 the baseline.

yurishkuro commented 3 years ago

I am tagging this as ready to implement, need a PR to upgrade all build files to 1.8