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

Truly decide on, and document with rationale, support for Java 7 #1543

Closed
anuraaga opened this issue Aug 17, 2020 · 39 comments
Closed

Truly decide on, and document with rationale, support for Java 7 #1543

anuraaga opened this issue Aug 17, 2020 · 39 comments

Comments

@anuraaga
Copy link
Contributor

The topic of Java 7 support came up again

open-telemetry/opentelemetry-java-instrumentation#714

It continues to come up. I thought we had some consensus that supporting Java 7, mostly to support Android apps that target a large range of OS, is ok. But admittedly we've only had conversations on several issues, maybe some sigs, who knows, but nothing permanent. I think we need to decide and document this decision instead of leaving it as an elephant in the room. A concrete example of what doesn't work well without a decision is #575. There's no point in the investigation if the end result is "seems good, but requires Java 8, did we actually make a decision on this?".

@anuraaga anuraaga added the Bug Something isn't working label Aug 17, 2020
@Oberon00
Copy link
Member

This was truly and formally decided in #744 IMHO, see e.g. #744 (comment)

@anuraaga
Copy link
Contributor Author

If it truly is, then we should probably add it to a markdown file somewhere in the repo with some rationale, referring people to docs is easier than issues. I'm happy to write a doc but need to reconfirm the decision since I suspect the buyin is not complete.

@Oberon00
Copy link
Member

referring people to docs is easier than issues

I would usually search in issues not in code for such a decision. However, a summarizing comment edited into the issue description, or as final comment that can be linked to would be helpful. I'm not against a markdown doc either though, it would just not be my first choice.

@Oberon00
Copy link
Member

Oberon00 commented Aug 17, 2020

I suspect the buyin is not complete.

All I can say is that at least for Dynatrace, nothing has changed (and most likely won't change in the next few years) in that regard: we still need a functional SDK + dependencies that support Java 7. We don't have a strong opinion on auto instrumentation.

@jkwatson
Copy link
Contributor

jkwatson commented Aug 17, 2020

I suspect the buyin is not complete.

All I can say is that at least for Dynatrace, nothing has changed (and most likely won't change in the next few years) in that regard: we still need a functional SDK + dependencies that support Java 7. We don't have a strong opinion on auto instrumentation.

@Oberon00 Can you write up a summary as to why you need java 7 support? It's definitely a large drag on productivity to support it, and if Dynatrace is the only vendor that needs it, can Dynatrace maintain its own fork with java 7 support?

@Oberon00
Copy link
Member

Oberon00 commented Aug 17, 2020

@jkwatson

Can you write up a summary as to why you need java 7 support?

We will be using an OpenTelemetry-based solution for our own auto-instrumentation and we already know that some customers that want to use it are stuck on Java 7 there for quite some time. It's as simple as that, unfortunately.

if Dynatrace is the only vendor that needs it,

I don't think that is the case.

On #744 (comment) it looked like Lightstep was glad to have Java 7 support too. CC @carlosalberto

And on open-telemetry/opentelemetry-java-instrumentation#714 linked in the initial description, there seems to be the assumption that basic Java 7 support is useful for Android app developers.

EDIT: In #744 (comment), @lmineiro from Zalando spoke up to say that they are also still using Java 7 JVMs for some services.

This matches experience at Dynatrace: Unfortunately, Java 7 (and other old technologies) are not as uncommon in enterprise code as one would hope.

It's definitely a large drag on productivity to support it

Really? Of course one always feels bad when having to write/let the IDE generate the boilerplate for an anonymous inner class instead of simply writing a lambda, but I don't think it's that big. Except probably for default interface implementations which would really be useful for library evolution (but see next points).

I think the core SDK is (or should be) a relatively simple piece of code. Having exporters or special propagators, or resource detection plugins or auto instrumentation use Java 8 (or 11) is fine with us.

can Dynatrace maintain its own fork with java 7 support?

I think it's clear that OpenTelemetry won't be supporting Java 7 (or even 8) forever (and probably also not as long as Dynatrace wants to support Java 7). So at some point, it will probably come to that. But I think having at least the GA version support Java 7, until the next major release (dropping Java 7 already warrants one) would be a good.

@carlosalberto
Copy link
Contributor

Observe that this is also needed by Google, IIRC (cc @bogdandrutu )

@jkwatson
Copy link
Contributor

The last we heard from Ted, Lightstep no longer needed java 7 support. I'm ok with re-opening this issue post GA.

And, yes, default methods on interfaces, and static members on interfaces is a big win for API design that we can't take advantage of with java 7.

@carlosalberto
Copy link
Contributor

Oh yes, I forgot to mention this - from the LS side, we don't need Java 7 support.

@kittylyst
Copy link
Contributor

I've seen this debate play out in other projects and venues, so I thought I’d add my 2c.

In general, I think it’s helpful to separate out the two issues: Android support vs supporting very old legacy Java apps.

Android is a separate runtime that actually has very little to do with the JVM these days apart from having some language level compatibility. I feel like it’s reasonable to ask to what extent a mainline project should be constrained in order to accommodate a fork (and one that is not converging, but very definitely charting its own destiny).

To the other point, Java 7 was EOL’d over 5 years ago, and its market share of Java apps in 2020 is a couple of percent (and that represents all Java 7 apps, not the ones that would be interested in OTel) - so if that is not compelling enough to remove Java 7 support, then I think it’s reasonable to ask what people think the gating criteria for removal should be.

If 5.5 years since EOL and 2.5% usage isn’t long enough and small enough to warrant dropping support then what is?

@Oberon00
Copy link
Member

Oberon00 commented Aug 17, 2020

I only have anecdotal evidence for this, but among the reasons that systems are still stuck at Java 7 is often also a combination of the following: (1) the system is large and complex or otherwise difficult difficult to port, (2) the system is mission-critical and runs well enough ("never touch a running system"). Both of these will also make you want to monitor the system. So while the market share of Java 7 may be very small, I think the relative share of apps in need of monitoring among them is higher than for newer Java versions.

@iNikem
Copy link
Contributor

iNikem commented Aug 18, 2020

You cannot add monitoring without "touching" the system. Both manual and auto instrumentations are quite invasive. I don't understand how "it is too risky to migrate to java8" and "let's add manual monitoring throughout the whole application" can both be true

@Oberon00
Copy link
Member

Auto instrumentation does not have to involve much manual work. Of course already adding a -javaagent command line to all JVMs can be a lot of work, but the risk of adding auto instrumentation is (at least often perceived as) lower than switching to a newer JVM.

@iNikem
Copy link
Contributor

iNikem commented Aug 18, 2020

Yeah, one more mystery: people are afraid of newer JVM but totally ok with allowing some library to modify all their classes in obscure way 🤷

@Oberon00
Copy link
Member

It's not always just psychology though, e.g. see #744 (comment)

@kittylyst
Copy link
Contributor

kittylyst commented Aug 18, 2020

So while the market share of Java 7 may be very small, I think the relative share of apps in need of monitoring among them is higher than for newer Java versions.

The number I quoted, 2.5%, is from New Relic customer data - and New Relic (currently) still supports Java 7. So, even if you're correct, the "enhanced relative share" is reflected in that number.

One potential compromise that I've been thinking about - what about producing an initial 1.0 GA release that supports Java 7 and then immediately removing Java 7 support as soon as GA is released - so that all future releases depend on Java 8.

That way, there is a stable basis and version for Java 7 (and Android folks) to start from, which corresponds to an actual released GA version, but it is then up to the people who actually want to support the long tail of legacy apps to maintain it on a branch from then on? This balances providing a starting point and home for Java 7 support, without compromising the long-term code quality.

Edit: Just saw the comment upthread about a major version being required to drop Java 7 support (so 2.0 being the first release that could mandate Java 8) - that seems like a much bigger ask.

@Oberon00
Copy link
Member

Oberon00 commented Aug 18, 2020

One potential compromise that I've been thinking about - what about producing an initial 1.0 GA release that supports Java 7

That sounds sensible

and then immediately removing Java 7 support as soon as GA is released

That sounds too fast. If we release it as GA we have to support it at least for some time, normally I'd say until the next major release. I do agree that Java 7 support can be dropped at some point, but immediately after GA is too fast.

EDIT:

Edit: Just saw the comment upthread about a major version being required to drop Java 7 support (so 2.0 being the first release that could mandate Java 8) - that seems like a much bigger ask.

It is. Although we could say that just that warrants releasing a major version.

I will also have to discuss this internally. E.g. what would be the consequences for Dynatrace having to support Java 7 while OpenTelemetry only supports 8+ or 11+, in how far would it still make sense for us to contribute to opentelemetry-java, etc.

@carlosalberto
Copy link
Contributor

For the sake of velocity, I'd suggest to stick to Java 7 and revisit dropping it right after GA (with a proper discussion).

I'd suggest we also go ahead and document this properly, to avoid further confusion ;)

@Oberon00
Copy link
Member

I agree with that. Also, doing a Java 8 rewrite will most likely not increase our velocity in the short term towards GA.
One thing to note, even though I'm against it: If we would want to drop Java 7 support soon after GA, we would need to mark it as deprecated ASAP. But I'm strongly against that 😃

@kittylyst
Copy link
Contributor

I agree with that. Also, doing a Java 8 rewrite will most likely not increase our velocity in the short term towards GA.

It depends upon what you think a "rewrite" involves - if it's just changing the version in the build file and then letting IntelliJ migrate inner classes to lambdas semi-automatically, then it need not be a lot of work. The possibilities that Java 8 default methods and other changes to the OO model open up is another matter, of course.

One thing to note, even though I'm against it: If we would want to drop Java 7 support soon after GA, we would need to mark it as deprecated ASAP. But I'm strongly against that 😃

What are your criteria for dropping Java 7 support? If you're advocating that the market is not ready to drop support yet, then what is the point at which it will be?

@trask
Copy link
Member

trask commented Aug 18, 2020

static members on interfaces is a big win for API design

yes! I would like to convert several of the API classes (e.g. SpanId, TraceId, Status, TraceFlags, SpanContext, EndSpanOptions, TraceState just to start with) to interfaces, which would give us much more flexibility in auto-instrumentation and would allow us to avoid bridging those classes back and forth.

but without static members on interfaces, we will have to introduce separate classes for creating these objects, e.g. SpanIds, TraceIds, Statuses (?), etc, which will make the API less discoverable for users.

@Oberon00
Copy link
Member

which will make the API less discoverable for users.

That's not what I would consider a "big" win.

would like to convert several of the API classes (e.g. SpanId, TraceId, Status, TraceFlags, SpanContext, EndSpanOptions, TraceState just to start with) to interfaces,

Converting a value class to an interface is a rather big undertaking. Or would you only do this to support auto-instrumentation, i.e. create stays the only way to create an instance and you would instrument that?

For SpanContext, TraceState, see also open-telemetry/opentelemetry-specification#759 (comment)

@thisthat thisthat added blocked:agreement Feedback Requested and removed Bug Something isn't working labels Aug 19, 2020
@Oberon00
Copy link
Member

OpenTracing and OpenCensus, to which OpenTelemetry wants to be compatible, also both support Java 7. So that is also a point to consider, and an argument to at least have one GA version support Java 7. Especially regarding breaking OpenCensus/OpenTelemetry bridge support for Java 7, I'd like the input of some OpenCensus maintainer (I think the OC bridge is maintained within OC).

@trask
Copy link
Member

trask commented Aug 20, 2020

This is what we are proposing for the opentelemetry-java-instrumentation project:

Rationale for not supporting Java 7

@jkwatson
Copy link
Contributor

At the SIG meeting on 8/21/2020, Dynatrace folks (@thisthat) said that having the SDK be java 8 was acceptable. Given this, I wonder if there's appetite to moving to full java 8 for the SDK, pre-GA, but keeping the API be java 7 compatible?

@anuraaga
Copy link
Contributor Author

@jkwatson Not sure if there's any significant win if we only do Java 8 for SDK. While Java 8 APIs like Stream are nice, they tend to have reasonable overhead and most low-level libraries I've seen end up avoiding them anyways in many cases (we could make do with our SDK dependency on Guava in cases where stream-type pattern is OK). I think lambda would be the major change, but either retrolambda, or living with Runnable which isn't that bad thanks to IntelliJ, is a reasonable alternative.

I'd say we'd want to push the API to Java 8 too if we open that door where we can definitely see big wins like default / static interface methods, using java.time types in API, etc. While I wouldn't block it, I am a negative vote if of only bumping the SDK, I don't see enough benefit TBH.

@thisthat
Copy link
Member

At the SIG meeting on 8/21/2020, Dynatrace folks (@thisthat) said that having the SDK be java 8 was acceptable. Given this, I wonder if there's appetite to moving to full java 8 for the SDK, pre-GA, but keeping the API be java 7 compatible?

Maybe I was not totally clear in the SIG meeting. After GA, this would be ok 😉

@jkwatson
Copy link
Contributor

@anuraaga I was thinking specifically of being able to use CompletableFuture in the export pipelines.

@anuraaga
Copy link
Contributor Author

Ah yeah that'd be nice. But unfortunately while there are a lot of Java 8 features available on Android now, CompletableFuture isn't one of them :(

https://developer.android.com/studio/write/java8-support-table

@jkwatson
Copy link
Contributor

We really need some assistance/input from an Android maintainer/spokesperson. It may be that our SDK isn't particularly useful on Android anyway, and hence we could drop Android default SDK support. I don't know enough about Android to know.

@Oberon00
Copy link
Member

Why would the SDK not be useful on Android?

@thisthat
Copy link
Member

Why would the SDK not be useful on Android?

A problem raised during the last SIG meeting was that the current SDK might consume too much battery for an Android app.

@Oberon00
Copy link
Member

I guess the BatchSpanProcessor could be bad for Android with it's periodic timer, but the core SDK should be fine, it does not spawn any background threads or anything.

@jkwatson
Copy link
Contributor

I guess the BatchSpanProcessor could be bad for Android with it's periodic timer, but the core SDK should be fine, it does not spawn any background threads or anything.

I think it's mostly that none of us are Android experts, so we really can't say one way or another. For instance, we recently got a bug report (#1531) that appears to be caused by ThreadLocalRandom not being properly initialized on Android.

@Oberon00
Copy link
Member

Oberon00 commented Sep 17, 2020

Now that @jkwatson has already created #1649, I have to admit that no one seems to have spoken up for Java 7 except for Dynatrace. It thus seems inevitable that OpenTelemetry will drop Java 7 support and Dynatrace will have to maintain our own fork.
However, exactly because no one else wanted Java 7, it will probably not make sense for us to open source that fork. That comes with additional work over just maintaining an internal fork, and interest seems low anyway. EDIT: As @danielkhan commented at #1649 (comment).

@Oberon00
Copy link
Member

Oberon00 commented Sep 18, 2020

@trask regarding your comment above #1543 (comment):

The specification currently says that SpaContext MUST be a final class in the API. I wanted that changed in the past but it was declined (open-telemetry/oteps#58). So even with Java 8, we won't be able to use an interface for SpanContext while staying spec-compliant.

Relevant spec is https://github.com/open-telemetry/opentelemetry-specification/blob/708d5df4f67a416058af346a84c1696bf64ab240/specification/trace/api.md#spancontext:

SpanContext MUST be a final (sealed) class.

@anuraaga
Copy link
Contributor Author

@Oberon00 Thanks for pointing that spec out - I've filed open-telemetry/opentelemetry-specification#969 since while many use cases are laid out in the otep, I think even before that it's just an impossible spec to begin with :)

@Oberon00
Copy link
Member

I think this can now be closed since #1665 is merged and #1649 is also closed.
If we need a tracking issue in Java for open-telemetry/opentelemetry-specification#969, it should be a separate one IMHO.

@jkwatson
Copy link
Contributor

closing, as we have moved to java 8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants