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

Add Ktor 3.0.0 support #12453

Closed
wants to merge 6 commits into from
Closed

Add Ktor 3.0.0 support #12453

wants to merge 6 commits into from

Conversation

e5l
Copy link

@e5l e5l commented Oct 17, 2024

Ktor 3.0 has been released last week.
This PR adds a new module to support the next major release.

Fix KTOR-7515

@e5l e5l requested a review from a team as a code owner October 17, 2024 07:15
Copy link

linux-foundation-easycla bot commented Oct 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@e5l e5l force-pushed the e5l/ktor-3.0.0 branch 3 times, most recently from 2aa3669 to 85513ba Compare October 17, 2024 07:46
@FunkyMuse FunkyMuse mentioned this pull request Oct 17, 2024
@laurit
Copy link
Contributor

laurit commented Oct 17, 2024

The instrumentation for ktor 2 and 3 looks really similar. The only difference I was able to spot is in https://github.com/ktorio/opentelemetry-java-instrumentation/blob/8e0154b25564612af1870ec75e0bbf9f366f7ea5/instrumentation/ktor/ktor-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ktor/v3_0/ServerInstrumentation.java#L28 My question is are there more changes needed to make instrumentation work on ktor 3? If not then I think we don't need to add a separate module for ktor 3, you could just replace named with namedOneOf and list both class names. I know that the test code didn't compile with ktor 3, we could handle this by having ktor 3 tests in a separate module.

@e5l
Copy link
Author

e5l commented Oct 17, 2024

Hey @laurit, thanks for the review!
Sure, let me fix that.

The only thing I'm afraid of is that we might need to add some extra for 3.1 and other minors (still not a major issue :) ).

@e5l e5l force-pushed the e5l/ktor-3.0.0 branch 2 times, most recently from 94f1fd6 to b904265 Compare October 24, 2024 06:58
@FunkyMuse
Copy link

i'm really sorry to be bothering here on the PR, but are there any news regarding this?

it's a blocker for us to go to production

@e5l
Copy link
Author

e5l commented Oct 28, 2024

Hey @FunkyMuse, let me check the build

@e5l
Copy link
Author

e5l commented Oct 28, 2024

Fixed, let's wait for the new build

@e5l
Copy link
Author

e5l commented Oct 29, 2024

Passed!
@theletterf, @laurit, could you please check?

docs/supported-libraries.md Outdated Show resolved Hide resolved
docs/supported-libraries.md Outdated Show resolved Hide resolved
Comment on lines +63 to +65
// this instrumentation creates a span per each physical request
// related issue https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/5722
disableTestRedirects()
Copy link
Contributor

Choose a reason for hiding this comment

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

I created #12530 to enable the redirect tests for ktor 2. You'll have to make the same change if that PR is merged before yours.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for noting

@@ -92,7 +92,7 @@ These are the supported libraries and frameworks:
| [Jodd Http](https://http.jodd.org/) | 4.2+ | N/A | [HTTP Client Spans], [HTTP Client Metrics] |
| [JSP](https://javaee.github.io/javaee-spec/javadocs/javax/servlet/jsp/package-summary.html) | 2.3+ | N/A | Controller Spans [3] |
| [Kotlin Coroutines](https://kotlinlang.org/docs/coroutines-overview.html) | 1.0+ | N/A | Context propagation |
| [Ktor](https://github.com/ktorio/ktor) | 1.0+ | [opentelemetry-ktor-1.0](../instrumentation/ktor/ktor-1.0/library),<br>[opentelemetry-ktor-2.0](../instrumentation/ktor/ktor-2.0/library) | [HTTP Client Spans], [HTTP Client Metrics], [HTTP Server Spans], [HTTP Server Metrics] |
| [Ktor](https://github.com/ktorio/ktor) | 1.0+ | [opentelemetry-ktor-1.0](../instrumentation/ktor/ktor-1.0/library),<br>[opentelemetry-ktor-2.0](../instrumentation/ktor/ktor-2.0/library) | [HTTP Client Spans], [HTTP Client Metrics], [HTTP Server Spans], [HTTP Server Metrics] |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reformat the table.

Copy link
Author

Choose a reason for hiding this comment

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

done

id("org.jetbrains.kotlin.jvm")
}

val ktorVersion = "3.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

You have test classes under src/main so the tests really won't be executed. What you need to do is have the common testing classes in this module and move the tests elsewhere. I'd use the gradle test suite feature and create a suite for ktor3 under both ktor2 library and javaagent modules. Let me know if it is unclear how to do this.

@laurit
Copy link
Contributor

laurit commented Oct 31, 2024

@e5l I tried getting the tests running by changing the test from agent tests to library tests just to see whether they work. Agent tests need to be run with the agent which requires using a different convention in the gradle build file which we can sort out later. I could not get them running and need your help with this. Here is the diff with my changes

diff --git a/instrumentation/ktor/ktor-3.0/testing/build.gradle.kts b/instrumentation/ktor/ktor-3.0/testing/build.gradle.kts
index d3fb3f3d0a..ac2bae285b 100644
--- a/instrumentation/ktor/ktor-3.0/testing/build.gradle.kts
+++ b/instrumentation/ktor/ktor-3.0/testing/build.gradle.kts
@@ -19,6 +19,10 @@ dependencies {
   compileOnly("org.jetbrains.kotlin:kotlin-stdlib-jdk8")
   compileOnly("io.ktor:ktor-server-netty:$ktorVersion")
   compileOnly("io.ktor:ktor-client-cio:$ktorVersion")
+
+  testImplementation(project(":instrumentation:ktor:ktor-2.0:library"))
+  testImplementation("io.ktor:ktor-server-netty:$ktorVersion")
+  testImplementation("io.ktor:ktor-client-cio:$ktorVersion")
 }
 
 kotlin {
diff --git a/instrumentation/ktor/ktor-3.0/testing/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/AbstractKtorHttpServerTest.kt b/instrumentation/ktor/ktor-3.0/testing/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/AbstractKtorHttpServerTest.kt
index 0f5ccacfac..607e4427d5 100644
--- a/instrumentation/ktor/ktor-3.0/testing/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/AbstractKtorHttpServerTest.kt
+++ b/instrumentation/ktor/ktor-3.0/testing/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/AbstractKtorHttpServerTest.kt
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.instrumentation.ktor.v3_0.server.io.opentelemetry.instrumentation.ktor.v3_0.server
+package io.opentelemetry.instrumentation.ktor.v3_0.server
 
 import io.ktor.http.*
 import io.ktor.server.application.*
diff --git a/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/client/KtorHttpClientInstrumentationTest.kt b/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/client/KtorHttpClientInstrumentationTest.kt
index d32922a71e..b92e8e20ae 100644
--- a/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/client/KtorHttpClientInstrumentationTest.kt
+++ b/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/client/KtorHttpClientInstrumentationTest.kt
@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.ktor.v3_0.client
 
 import io.ktor.client.*
+import io.opentelemetry.instrumentation.ktor.v2_0.client.KtorClientTracing
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension
 import org.junit.jupiter.api.extension.RegisterExtension
 
@@ -14,9 +15,14 @@ class KtorHttpClientInstrumentationTest : AbstractKtorHttpClientTest() {
   companion object {
     @JvmStatic
     @RegisterExtension
-    private val TESTING = HttpClientInstrumentationExtension.forAgent()
+    private val TESTING = HttpClientInstrumentationExtension.forLibrary()
   }
 
   override fun HttpClientConfig<*>.installTracing() {
+    install(KtorClientTracing) {
+      setOpenTelemetry(TESTING.openTelemetry)
+      capturedRequestHeaders(TEST_REQUEST_HEADER)
+      capturedResponseHeaders(TEST_RESPONSE_HEADER)
+    }
   }
 }
diff --git a/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/KtorHttpServerInstrumentationTest.kt b/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/KtorHttpServerInstrumentationTest.kt
index 164d6b2b53..c0fe680fa0 100644
--- a/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/KtorHttpServerInstrumentationTest.kt
+++ b/instrumentation/ktor/ktor-3.0/testing/src/test/kotlin/io/opentelemetry/instrumentation/ktor/v3_0/server/KtorHttpServerInstrumentationTest.kt
@@ -6,7 +6,6 @@
 package io.opentelemetry.instrumentation.ktor.v3_0.server
 
 import io.ktor.server.application.*
-import io.opentelemetry.instrumentation.ktor.v3_0.server.io.opentelemetry.instrumentation.ktor.v3_0.server.AbstractKtorHttpServerTest
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension
 import io.opentelemetry.instrumentation.testing.junit.http.HttpServerInstrumentationExtension
 import io.opentelemetry.instrumentation.testing.junit.http.HttpServerTestOptions
@@ -17,7 +16,7 @@ class KtorHttpServerInstrumentationTest : AbstractKtorHttpServerTest() {
   companion object {
     @JvmStatic
     @RegisterExtension
-    val TESTING: InstrumentationExtension = HttpServerInstrumentationExtension.forAgent()
+    val TESTING: InstrumentationExtension = HttpServerInstrumentationExtension.forLibrary()
   }
 
   override fun getTesting(): InstrumentationExtension {
@@ -25,6 +24,13 @@ class KtorHttpServerInstrumentationTest : AbstractKtorHttpServerTest() {
   }
 
   override fun installOpenTelemetry(application: Application) {
+    /*
+    application.install(KtorServerTracing) {
+      setOpenTelemetry(TESTING.openTelemetry)
+      capturedRequestHeaders(TEST_REQUEST_HEADER)
+      capturedResponseHeaders(TEST_RESPONSE_HEADER)
+    }
+     */
   }
 
   override fun configure(options: HttpServerTestOptions) {

the http client test works fine, but for the server tests I need help with implementing the installOpenTelemetry method. Could you have a look?

@e5l
Copy link
Author

e5l commented Oct 31, 2024

Sure, let me check

@e5l
Copy link
Author

e5l commented Oct 31, 2024

It looks like the problem is with generic variance and we will have to make a separate module to avoid it

@e5l
Copy link
Author

e5l commented Oct 31, 2024

I've managed to fix the difference. The only thing that needs a workaround is - the binary breaking change around the call started event. Checking if it can be fixed without splitting the modules

@e5l
Copy link
Author

e5l commented Oct 31, 2024

@laurit, looks like the build is good. Could you check?

@e5l
Copy link
Author

e5l commented Nov 5, 2024

Outdated when #12562 will be merged.
Feel free to close

@laurit laurit closed this Nov 11, 2024
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