Skip to content

Convert play-ws groovy to java#12377

Merged
trask merged 25 commits into
open-telemetry:mainfrom
heyams:heya/convert-play-ws-groovy-to-java
Oct 30, 2024
Merged

Convert play-ws groovy to java#12377
trask merged 25 commits into
open-telemetry:mainfrom
heyams:heya/convert-play-ws-groovy-to-java

Conversation

@heyams

@heyams heyams commented Oct 2, 2024 •
edited by jaydeluca
Loading

Copy link
Copy Markdown
Contributor

Related to #7195

@heyams heyams marked this pull request as ready for review October 2, 2024 22:32
@heyams heyams requested a review from a team as a code owner October 2, 2024 22:32
@heyams heyams force-pushed the heya/convert-play-ws-groovy-to-java branch from 925d932 to faf245c Compare October 3, 2024 21:53
@heyams heyams closed this Oct 7, 2024
@heyams heyams reopened this Oct 7, 2024
@heyams

heyams commented Oct 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:compileTestJava
./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:compileLatestDepTestJava
were green locally. CI stuck with this failure. Any ideas? cc @laurit

./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:latestDepTest -PtestLatestDeps=true was green locally too.

it stuck with this error in the CI log

Task :instrumentation:play:play-ws:play-ws-2.1:javaagent:compileLatestDepTestJava
2024-10-07T21:01:20.6307747Z /home/runner/work/opentelemetry-java-instrumentation/opentelemetry-java-instrumentation/instrumentation/play/play-ws/play-ws-2.1/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/playws/v2_1/PlayJavaStreamedWsClientTest.java:10: error: cannot access StandaloneWSRequest
2024-10-07T21:01:20.6311126Z class PlayJavaStreamedWsClientTest extends PlayJavaStreamedWsClientBaseTest {}
2024-10-07T21:01:20.6311976Z 
2024-10-07T21:01:20.6312183Z ^
2024-10-07T21:01:20.6312839Z   class file for play.libs.ws.StandaloneWSRequest not found

but i can't seem to repro locally.

2.1 regular test uses com.typesafe.play:play-ahc-ws-standalone_$2.12:2.1.0, which has play.libs.ws.StandaloneWSRequest.

@steverao

steverao commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:compileTestJava ./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:compileLatestDepTestJava were green locally. CI stuck with this failure. Any ideas? cc @laurit

./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:latestDepTest -PtestLatestDeps=true was green locally too.

According to related hint, you can reproduce it locally by executing
./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:compileTestJava -PtestLatestDeps=true

@heyams

heyams commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

thanks @steverao! i was able to repro per your comment and fix it.

Logs in Develocity seems more useful. I didn't check there and i forgot how to get there. Hint please?

@jaydeluca

jaydeluca commented Oct 10, 2024

Copy link
Copy Markdown
Member

Logs in Develocity seems more useful. I didn't check there and i forgot how to get there. Hint please?

@heyams i usually open the failed build step logs in the browser and search for the words "build scan". There will often be multiple so look for the one following the "FAILED" indicator. Use that link to generate a link to develocity

@heyams

heyams commented Oct 11, 2024

Copy link
Copy Markdown
Contributor Author

Logs in Develocity seems more useful. I didn't check there and i forgot how to get there. Hint please?

@heyams i usually open the failed build step logs in the browser and search for the words "build scan". There will often be multiple so look for the one following the "FAILED" indicator. Use that link to generate a link to develocity

thanks @jaydeluca.

@laurit laurit mentioned this pull request Oct 17, 2024
Closed
laurit
laurit reviewed Oct 17, 2024
View reviewed changes
Comment thread instrumentation/play/play-ws/play-ws-2.1/javaagent/build.gradle.kts
exclude("com.typesafe.play", "play-ahc-ws-standalone_$scalaVersion")
tasks {
if (testLatestDeps) {
// disable regular test running and compiling tasks when latest dep test task is run

@laurit laurit Oct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

comment is a bit misleading, as far as I can tell compile task is not disabled

@heyams heyams Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is only disabling test module. why do i need disable compile task?

laurit
laurit reviewed Oct 17, 2024
View reviewed changes
Comment thread ...epTest/java/io/opentelemetry/javaagent/instrumentation/playws/v2_1/PlayWsClientBaseTest.java
import play.shaded.ahc.org.asynchttpclient.DefaultAsyncHttpClientConfig;
import play.shaded.ahc.org.asynchttpclient.RequestBuilderBase;

abstract class PlayWsClientBaseTest<REQUEST> extends AbstractHttpClientTest<REQUEST> {

@laurit laurit Oct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Groovy version doesn't have separate sources for the latest dep tests. Are you sure we need to copy paste these for the latest dep tests?

@heyams heyams Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

new version uses different packages for a lot of classes. initially, I tried reflection, but there were too many changes. that is why i have created a new module. it's not exactly the same though if you look closely.

@laurit

laurit commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

when I run ./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:build -PtestLatestDeps=true locally it fails with java.io.IOException: Too many open files

@heyams

heyams commented Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

when I run ./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:build -PtestLatestDeps=true locally it fails with java.io.IOException: Too many open files

./gradlew :instrumentation:play:play-ws:play-ws-2.1:javaagent:build -PtestLatestDeps=true is green for me. might want to pull the latest and retry.

@trask trask merged commit 0b9dffa into open-telemetry:main Oct 30, 2024
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
@heyams @laurit
Convert play-ws groovy to java (open-telemetry#12377)
bc625b4 
Co-authored-by: Lauri Tulmin <ltulmin@splunk.com>
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
@heyams @laurit
Convert play-ws groovy to java (open-telemetry#12377)
42d55ed 
Co-authored-by: Lauri Tulmin <ltulmin@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurit laurit laurit approved these changes

@jaydeluca jaydeluca jaydeluca approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@heyams @steverao @jaydeluca @laurit @trask