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.

Sign up for GitHub

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

Jump to bottom

Convert play-ws groovy to java #12377

Merged
merged 25 commits into from
Oct 30, 2024
Merged

Convert play-ws groovy to java #12377

merged 25 commits into from
Oct 30, 2024

Conversation

heyams
Copy link
Contributor

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

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
Copy link
Contributor Author

heyams commented Oct 7, 2024 •
edited
Loading

./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
Copy link
Contributor

steverao commented Oct 8, 2024 •
edited
Loading

./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
Copy link
Contributor Author

heyams commented Oct 8, 2024 •
edited
Loading

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
Copy link
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
Copy link
Contributor Author

heyams commented Oct 11, 2024

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
Open
laurit
laurit reviewed Oct 17, 2024
View reviewed changes
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
Copy link
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

Copy link
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
...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> {
Copy link
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?

Copy link
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
Copy link
Contributor

laurit commented Oct 17, 2024

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
Copy link
Contributor Author

heyams commented Oct 25, 2024

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
56 checks passed
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 <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaydeluca jaydeluca jaydeluca approved these changes

@laurit laurit laurit 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