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

Drop Java 7 except for manual instrumentation that is important to Android ecosystem #714

Closed
trask opened this issue Jul 16, 2020 · 25 comments
Labels
area:build Issues about build infra, both local and central area:tests contributor experience Something that we can do to improve the experience of contributors (especially new contributors)

Comments

@trask
Copy link
Member

trask commented Jul 16, 2020

This has been discussed for a few weeks now in #599 and in the weekly meetings.

Agreement has been made to drop Java 7 except for manual instrumentation that is important to the Android ecosystem.

Our initial list of manual instrumentation that is important to the Android ecosystem is OkHttp and gRPC, but we can always add more manual instrumentation to this list as we receive feedback (and bump those manual instrumentations back down to Java 7).

@anuraaga
Copy link
Contributor

anuraaga commented Jul 16, 2020

For reference other instrumentation I found that are possibly relevant are

  • Apache http
  • Google http client (well, not sure how useful this instrumentation is in general we'd actually want Google API clients instrumented)
  • khttp (because kotlin)
  • rxjava (used heavily on Android, but perhaps the thread switch instrumentation is actually not usefulness there)

@trask trask added the contributor experience Something that we can do to improve the experience of contributors (especially new contributors) label Jul 19, 2020
@trask
Copy link
Member Author

trask commented Jul 19, 2020

Adding comment here from @huntc

I understand that having Android support is nice, but how essential is it for this particular project? Might it be more appropriate to develop an entirely new Kotlin based project and leverage its native capabilities e.g. coroutines etc?

I'm escalating in tomorrow's OpenTelemetry maintainer meeting to see if Google wants to have a hand in guiding our Android story.

@anuraaga
Copy link
Contributor

Just for some color for the conversation, I would say the vast majority of our API is very simple and works well with any Java version or runtime.

getTracer
startSpan
currentSpan
Span.tag etc

The comment about coroutines comes from considering the return value for results in Exporter in the SDK, not about instrumentation (I directed to the instrumentation issue just as a source of context for the discussion). This is a very small part of our API and doesn't seem like we would want to fork the whole SDK for it. Observability is low level, so we can expect to get a little grungy, but we do this to provide as good experience to users as possible. We don't want observability to dictate requirements for a user's app, so a low baseline for the SDK still makes sense to me.

My 2c

@bogdandrutu
Copy link
Member

@arminru @Oberon00 I know you had a requirement for Java 7

@Oberon00
Copy link
Member

The Java 7 requirement is mainly for the SDK and API. While it would be nice if the auto instrumentation also worked on Java 7, we currently have no plans for using it there.

@trask
Copy link
Member Author

trask commented Jul 20, 2020

@Oberon00 we are also baselining library instrumentation to Java 8, except for library instrumentation that is important to Android ecosystem (e.g. OkHttp, gRPC).

@iNikem iNikem added release:required-for-ga area:build Issues about build infra, both local and central area:tests labels Jul 22, 2020
@trask
Copy link
Member Author

trask commented Aug 17, 2020

Another reason to drop Java 7: JUnit 5

@Oberon00
Copy link
Member

In opentelemetry-java, Java 8 is used only for test code to enable Junit 5 while still supporting Java 7.

@trask
Copy link
Member Author

trask commented Aug 17, 2020

Yeah, but that severely limits your test coverage on Java 7. I'm not sure we want to say we're Java 7 compliant if 95% of our tests are only running on Java 8+.

@anuraaga
Copy link
Contributor

It's an interesting question about the SDK. I think for instrumentation, it's not as bad since we're only going to support a few libraries - we could have container-based smoke tests for them. I guess it'd give reasonable confidence the SDK works for them too

@trask
Copy link
Member Author

trask commented Aug 17, 2020

Makes me like the idea of separate Kotlin-based instrumentation for Android even more... Modern Java development is just not intended to target Java 7 😭

@iNikem
Copy link
Contributor

iNikem commented Aug 17, 2020

I still don't understand our Android story. I have never developed for Android. What do we offer them at all? What should work and what not?

@anuraaga
Copy link
Contributor

Android (or browser via opentelemetry-js which has its own maintenance issues to deal with because of browser support) allow making client spans on the actual user device - this can be extremely helpful! Many times even if the server feels fast, there are problems in the client that make it actually slow for the user and when tracing can find these, teams can really solve performance issues fast.

I think the Span and Tracer APIs are all that really need to function on external clients, and of course instrumentation using those in popular Android libraries. Exporters can rarely be used as is from an external client due to special auth or bandwidth constraints (I've seen external spans are often piggy backed onto normal API requests) so have less importance.

I'd say smoke tests for Java 7 libraries can give enough coverage to a small set of libraries that we could have reasonable confidence saying we support them.

@iNikem
Copy link
Contributor

iNikem commented Aug 17, 2020

Do we know which libraries we should support on java7 for Android?

@anuraaga
Copy link
Contributor

To narrow down the list from the start of the thread further, I think okhttp, grpc, apache http would have a huge amount of coverage and would be my recommended list. The bare minimum is I guess okhttp, even gRPC would get some minimal amount of instrumentation if it's okhttp transport was instrumented.

@iNikem
Copy link
Contributor

iNikem commented Aug 17, 2020

Is there any way to replace guesses with facts? :) I mean, first, do we know at least one Android project that wants to use Otel in general and our artifacts in particular?

@anuraaga
Copy link
Contributor

Hearing about real cases would definitely be cool. But I think we can be philosophical here - if there is a device that has the possibility of performance issues, we want to observe it right? Brave continuing to support Java 1.6 because all users deserve observability is awesome and I'm very happy to put in some extra work to support more users. If this doesn't align with OTel values, I need to know soon so I can look for another job.

@iNikem
Copy link
Contributor

iNikem commented Aug 17, 2020

If this doesn't align with OTel values

I haven't suggested that :) My point that our effort should be based on real-world needs. As I know nothing about Android development I am very eager to hear from some potential future user about what do they actually need.

What is still unclear to me, is how Android is going to consume Otel? There is no javaagent, right? So we are speaking about manual instrumentation? And we don't have manual instrumentation for "okhttp, grpc, apache http", do we? So Android developers cannot use this instrumentation repo atm at all? Or do I miss something?

If my understanding above is correct, then we don't have a problem "dropping java7 support will hurt Android developers". We have a problem "we don't support Android developers".

@anuraaga
Copy link
Contributor

Yeah, it can only be manual instrumentation. We have them as P3 tasks in our ga requirements because we care about them, but need some effort to support them. But an upgrade of the Java baseline means that even the P3 becomes unachievable. I think the cost to maintainers to try for this is much lower than everyone is making it out to be.

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/master/docs/ga-requirements.md

@trask
Copy link
Member Author

trask commented Aug 17, 2020

@trask
Copy link
Member Author

trask commented Aug 17, 2020

Hm, we can still meet (always good to see @anuraaga and @iNikem in the same place at the same time 😄), but reading the above more carefully, can we all agree with the initial proposal:

Drop Java 7 except for manual instrumentation that is important to Android ecosystem

I'm less concerned with the idea of running tests against Java 8 and expecting it to work on Java 7 for manual instrumentation (and like @anuraaga mentioned, we can have smoke test for those cases also).

@iNikem
Copy link
Contributor

iNikem commented Aug 17, 2020

We can agree on that, certainly :) My concern is how we define that limited set of libraries :)

And from GA perspective, we have to drop things ASAP, before GA. And we can add things either now or later. So "drop java7 support" vs "add Android support" are two very different tasks.

@trask
Copy link
Member Author

trask commented Aug 17, 2020

Brave continuing to support Java 1.6 because all users deserve observability is awesome and I'm very happy to put in some extra work to support more users

It looks like Brave has stayed on JUnit 4.

I suspect Brave hasn't upgraded to JUnit 5 so that they can ensure the quality of their pre-Java 8 support.

This is my concern.

It's hard to have it both ways: use modern tooling for Java testing (JUnit 5 and testcontainers), while also ensuring quality testing and support of older Java versions.

@trask
Copy link
Member Author

trask commented Aug 20, 2020

We met on this topic yesterday, and this is what we are proposing for the opentelemetry-java-instrumentation project:

Rationale for not supporting Java 7

@trask
Copy link
Member Author

trask commented Sep 24, 2020

Closed by #1229

@trask trask closed this as completed Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:build Issues about build infra, both local and central area:tests contributor experience Something that we can do to improve the experience of contributors (especially new contributors)
Projects
None yet
Development

No branches or pull requests

5 participants