Skip to content

Conversation

@trask
Copy link
Member

@trask trask commented Sep 19, 2025

@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (567e737) to head (411f24a).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7681   +/-   ##
=========================================
  Coverage     90.12%   90.12%           
  Complexity     7183     7183           
=========================================
  Files           814      814           
  Lines         21675    21675           
  Branches       2123     2123           
=========================================
  Hits          19534    19534           
  Misses         1475     1475           
  Partials        666      666           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@trask trask marked this pull request as ready for review September 19, 2025 16:16
@trask trask requested a review from a team as a code owner September 19, 2025 16:16
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, because it seems like it at least reduces the number of places that depend on okhttp-jvm....but does it actually really solve the problem in #7678 if the published pom still declares dependency on okhttp-jvm (like this example from contrib)???

@laurit
Copy link
Contributor

laurit commented Sep 19, 2025

Approving, because it seems like it at least reduces the number of places that depend on okhttp-jvm....but does it actually really solve the problem in #7678 if the published pom still declares dependency on okhttp-jvm (like this example from contrib)???

The idea behind this is that the gradle metadata in https://repo1.maven.org/maven2/io/opentelemetry/contrib/opentelemetry-opamp-client/1.49.0-alpha/opentelemetry-opamp-client-1.49.0-alpha.module points to okhttp not okhttp-jvm so gradle can choose the appropriate dependency. Since using just okhttp won't work with maven we change the dependency to okhttp-jvm in the published pom.

@jkwatson
Copy link
Contributor

Can someone verify this works for Android builds?

@breedx-splk
Copy link
Contributor

Approving, because it seems like it at least reduces the number of places that depend on okhttp-jvm....but does it actually really solve the problem in #7678 if the published pom still declares dependency on okhttp-jvm (like this example from contrib)???

The idea behind this is that the gradle metadata in https://repo1.maven.org/maven2/io/opentelemetry/contrib/opentelemetry-opamp-client/1.49.0-alpha/opentelemetry-opamp-client-1.49.0-alpha.module points to okhttp not okhttp-jvm so gradle can choose the appropriate dependency. Since using just okhttp won't work with maven we change the dependency to okhttp-jvm in the published pom.

OH thanks for that clarification...I always forget that modules exist in java. 🙈 That makes much more sense now.

@cbruegg
Copy link

cbruegg commented Sep 22, 2025

@jkwatson I've checked out @trask's branch, ran publishToMavenLocal and updated from 1.52.0 to 1.55.0-SNAPSHOT in my Android project. Unlike with 1.53.0, the app built and ran fine. I confirmed with ./gradlew app:dependencies that Gradle really uses okhttp instead of okhttp-jvm:

|         |    |    +--- io.opentelemetry:opentelemetry-exporter-sender-okhttp:1.55.0-SNAPSHOT
|         |    |    |    +--- io.opentelemetry:opentelemetry-exporter-common:1.55.0-SNAPSHOT (*)
|         |    |    |    +--- io.opentelemetry:opentelemetry-sdk-common:1.55.0-SNAPSHOT (*)
|         |    |    |    \--- com.squareup.okhttp3:okhttp:5.1.0 (*)

This matches 1.52.0 (which is working):

|         |    |    +--- io.opentelemetry:opentelemetry-exporter-sender-okhttp:1.52.0
|         |    |    |    +--- io.opentelemetry:opentelemetry-exporter-common:1.52.0 (*)
|         |    |    |    +--- io.opentelemetry:opentelemetry-sdk-common:1.52.0 (*)
|         |    |    |    \--- com.squareup.okhttp3:okhttp:5.1.0 (*)

In contrast, this is what it looks like with 1.53.0 (which is not working):

|         |    |    +--- io.opentelemetry:opentelemetry-exporter-sender-okhttp:1.53.0
|         |    |    |    +--- io.opentelemetry:opentelemetry-exporter-common:1.53.0 (*)
|         |    |    |    +--- io.opentelemetry:opentelemetry-sdk-common:1.53.0 (*)
|         |    |    |    \--- com.squareup.okhttp3:okhttp-jvm:5.1.0

In summary, this PR indeed seems to resolve #7678. Thanks @trask, @jkwatson, @breedx-splk and @laurit for all your support!

@laurit
Copy link
Contributor

laurit commented Sep 22, 2025

OH thanks for that clarification...I always forget that modules exist in java. 🙈 That makes much more sense now.

it doesn't really have anything to do with java modules, gradle metadata uses .module extension.

@jkwatson jkwatson merged commit 9153d6c into open-telemetry:main Sep 22, 2025
29 checks passed
@ColtonIdle
Copy link

ColtonIdle commented Oct 15, 2025

Was this released in https://github.com/open-telemetry/opentelemetry-java/releases/tag/v1.55.0 ?

edit: looks like i didn't look close enough. does indeed seem to be the case. awesome!

@trask trask deleted the okhttp branch October 20, 2025 17:27
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.

OpenTelemetry should depend on okhttp, not okhttp-jvm

6 participants