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

Make resource-detector compatible with Java 8 #275

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

psx95
Copy link
Contributor

@psx95 psx95 commented Nov 30, 2023

Description

This PR makes the resource detector Java 8 compatible. This is to support #266.

How the changes were tested

  • Used the GCP Detector jar built from this change in a Java 8 application (running in a docker container) to successfully detect resources. The Java 8 application was similar to resource-example in the current repo.
  • Used the javap tool to check the major version with which the classes was compiled -
 javap -v javap -v detectors/resources/build/classes/java/main/com/google/cloud/opentelemetry/detectors/*.class | grep major

Output -

  major version: 52
  major version: 52
  major version: 52
  major version: 52

Indicates all classes within resource detector are Java 8 compatible.

Other artifacts are still compiled & targeted for Java 11.

javap -v exporters/*/build/classes/java/main/com/google/cloud/opentelemetry/*/*.class | grep major

Output -

  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55
  major version: 55

@psx95 psx95 requested a review from jsuereth November 30, 2023 23:48
@psx95 psx95 marked this pull request as ready for review December 1, 2023 19:36
@psx95 psx95 requested a review from a team as a code owner December 1, 2023 19:36
@psx95 psx95 requested a review from dashpole December 1, 2023 19:36
@dashpole
Copy link
Contributor

dashpole commented Dec 2, 2023

Do we have any test/checks to ensure it continues to support java 8? Or is this enough, as long as we don't change this config?

@psx95
Copy link
Contributor Author

psx95 commented Dec 2, 2023

Do we have any test/checks to ensure it continues to support java 8? Or is this enough, as long as we don't change this config?

No, not at the moment, I tested everything manually in a one-off fashion. We currently do not have any tests pertaining to specific Java version.

This was just the first step, checking to see if our build works properly. Once we refactor out the resource-detector-support library, we can add tests there.

However, based on the gradle documentation on release flag used in this config, I think it should give us better assurances than using the Java compatibility options.

Maybe @jsuereth can weigh in here

@psx95 psx95 merged commit fed23bf into GoogleCloudPlatform:main Dec 4, 2023
13 checks passed
@psx95 psx95 deleted the detector-resources-java-8 branch December 4, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants