From 6340cb67451ffaf33d602db08394fe908293d03b Mon Sep 17 00:00:00 2001 From: Yigit Boyar Date: Tue, 11 Apr 2023 11:20:50 -0700 Subject: [PATCH 1/2] Add ability to configure http stack This CL changes the TestRunnerService factory method to receive a new parameter that can be used to configure the http stack. This is especially useful for us to be able to get structured logs out of the Http requests of the ci-action library. Test: TestRunnerServiceImplTest --- .../kotlin/dev/androidx/ci/config/Config.kt | 5 +- .../androidx/ci/config/HttpConfigAdapter.kt | 62 +++++++++++++++++++ .../ci/firebase/FirebaseTestLabApi.kt | 37 +++++------ .../androidx/ci/firebase/ToolsResultApi.kt | 13 ++-- .../ci/testRunner/TestRunnerService.kt | 15 ++--- .../dev/androidx/ci/util/RetrofitExt.kt | 27 +++----- .../testRunner/TestRunnerServiceImplTest.kt | 54 ++++++++++++++++ .../testRunner/TestRunnerServicePlayground.kt | 1 - 8 files changed, 156 insertions(+), 58 deletions(-) create mode 100644 AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/Config.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/Config.kt index 0744345..68cc0c8 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/Config.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/Config.kt @@ -17,7 +17,6 @@ package dev.androidx.ci.config import com.google.auth.Credentials -import okhttp3.logging.HttpLoggingInterceptor /** * Common configuration for TestRunner. @@ -43,7 +42,7 @@ internal class Config { class FirebaseTestLab( val gcp: Gcp, val endPoint: String = "https://testing.googleapis.com/v1/", - val httpLogLevel: HttpLoggingInterceptor.Level = HttpLoggingInterceptor.Level.NONE, + val httpConfigAdapterFactory: HttpConfigAdapter.Factory = HttpConfigAdapter.Factory.NoOp ) class Datastore( val gcp: Gcp, @@ -58,7 +57,7 @@ internal class Config { class ToolsResult( val gcp: Gcp, val endPoint: String = "https://toolresults.googleapis.com/toolresults/v1beta3/", - val httpLogLevel: HttpLoggingInterceptor.Level = HttpLoggingInterceptor.Level.NONE, + val httpConfigAdapterFactory: HttpConfigAdapter.Factory = HttpConfigAdapter.Factory.NoOp ) class Gcp( diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt new file mode 100644 index 0000000..e95f9ff --- /dev/null +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt @@ -0,0 +1,62 @@ +package dev.androidx.ci.config + +import okhttp3.Interceptor +import okhttp3.logging.HttpLoggingInterceptor +import org.apache.logging.log4j.kotlin.loggerOf +import kotlin.reflect.KClass + +/** + * Configuration for OkHttp & Retrofit. + */ +interface HttpConfigAdapter { + fun createCallFactory( + delegate: okhttp3.Call.Factory + ): okhttp3.Call.Factory = delegate + + fun interceptors(): List = emptyList() + + fun interface Factory { + /** + * Creates an [HttpConfigAdapter] for the given [klass]. + * + * @param klass The class which is creating the Retrofit / OkHttp clients. + * @return A HttpConfigAdapter that will be used to configure the network stack. + */ + fun create(klass: KClass<*>): HttpConfigAdapter + + class Logging( + private val logLevel: HttpLoggingInterceptor.Level + ) : Factory { + override fun create(klass: KClass<*>): HttpConfigAdapter { + return LoggingConfig( + logClass = klass, + logLevel = logLevel + ) + } + } + + object NoOp : Factory { + override fun create(klass: KClass<*>): HttpConfigAdapter { + return object : HttpConfigAdapter {} + } + } + } + + class LoggingConfig( + private val logClass: KClass<*>, + private val logLevel: HttpLoggingInterceptor.Level + ) : HttpConfigAdapter { + override fun interceptors(): List { + val log4jLogger = loggerOf(logClass.java) + val loggingInterceptor = HttpLoggingInterceptor( + logger = { + log4jLogger.info(it) + } + ) + loggingInterceptor.level = logLevel + return listOf( + loggingInterceptor + ) + } + } +} \ No newline at end of file diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt index b2edc4e..b3f150f 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt @@ -25,7 +25,7 @@ import dev.androidx.ci.generated.ftl.TestEnvironmentCatalog import dev.androidx.ci.generated.ftl.TestMatrix import dev.androidx.ci.util.Retry import dev.androidx.ci.util.RetryCallAdapterFactory -import dev.androidx.ci.util.withLog4J +import dev.androidx.ci.util.withConfig import dev.zacsweers.moshix.reflect.MetadataKotlinJsonAdapterFactory import okhttp3.OkHttpClient import retrofit2.Retrofit @@ -68,23 +68,23 @@ internal interface FirebaseTestLabApi { fun build( config: Config.FirebaseTestLab ): FirebaseTestLabApi { - val client = OkHttpClient.Builder().authenticateWith( - config.gcp - ).addInterceptor { - val newBuilder = it.request().newBuilder() - newBuilder.addHeader( - "Content-Type", "application/json;charset=utf-8", - ) - newBuilder.addHeader( - "Accept", "application/json" - ) - it.proceed( - newBuilder.build() - ) - }.withLog4J( - level = config.httpLogLevel, - klass = FirebaseTestLabApi::class - ).build() + val httpAdapter = config.httpConfigAdapterFactory.create(FirebaseTestLabApi::class) + val client = OkHttpClient.Builder().withConfig( + httpAdapter + ).authenticateWith( + config.gcp + ).addInterceptor { + val newBuilder = it.request().newBuilder() + newBuilder.addHeader( + "Content-Type", "application/json;charset=utf-8", + ) + newBuilder.addHeader( + "Accept", "application/json" + ) + it.proceed( + newBuilder.build() + ) + }.build() val moshi = Moshi.Builder() .add(MetadataKotlinJsonAdapterFactory()) .build() @@ -93,6 +93,7 @@ internal interface FirebaseTestLabApi { .baseUrl(config.endPoint) .addConverterFactory(MoshiConverterFactory.create(moshi).asLenient()) .addCallAdapterFactory(RetryCallAdapterFactory.GLOBAL) + .callFactory(httpAdapter.createCallFactory(client)) .build() .create(FirebaseTestLabApi::class.java) } diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/ToolsResultApi.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/ToolsResultApi.kt index 058a1cd..d1a9b9f 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/ToolsResultApi.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/ToolsResultApi.kt @@ -23,7 +23,7 @@ import dev.androidx.ci.generated.testResults.ListHistoriesResponse import dev.androidx.ci.generated.testResults.ListStepsResponse import dev.androidx.ci.util.Retry import dev.androidx.ci.util.RetryCallAdapterFactory -import dev.androidx.ci.util.withLog4J +import dev.androidx.ci.util.withConfig import dev.zacsweers.moshix.reflect.MetadataKotlinJsonAdapterFactory import okhttp3.OkHttpClient import retrofit2.Retrofit @@ -65,7 +65,10 @@ internal interface ToolsResultApi { fun build( config: Config.ToolsResult ): ToolsResultApi { - val client = OkHttpClient.Builder().authenticateWith( + val httpAdapter = config.httpConfigAdapterFactory.create(ToolsResultApi::class) + val client = OkHttpClient.Builder().withConfig( + httpAdapter + ).authenticateWith( config.gcp ).addInterceptor { val newBuilder = it.request().newBuilder() @@ -78,10 +81,7 @@ internal interface ToolsResultApi { it.proceed( newBuilder.build() ) - }.withLog4J( - level = config.httpLogLevel, - klass = ToolsResultApi::class - ).build() + }.build() val moshi = Moshi.Builder() .add(MetadataKotlinJsonAdapterFactory()) .build() @@ -90,6 +90,7 @@ internal interface ToolsResultApi { .baseUrl(config.endPoint) .addConverterFactory(MoshiConverterFactory.create(moshi).asLenient()) .addCallAdapterFactory(RetryCallAdapterFactory.GLOBAL) + .callFactory(httpAdapter.createCallFactory(client)) .build() .create(ToolsResultApi::class.java) } diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt index 9de3f6f..806ca9f 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt @@ -3,6 +3,7 @@ package dev.androidx.ci.testRunner import com.google.auth.Credentials import dev.androidx.ci.config.Config import dev.androidx.ci.config.Config.Datastore.Companion.AOSP_OBJECT_KIND +import dev.androidx.ci.config.HttpConfigAdapter import dev.androidx.ci.datastore.DatastoreApi import dev.androidx.ci.firebase.FirebaseTestLabApi import dev.androidx.ci.firebase.ToolsResultApi @@ -119,10 +120,9 @@ interface TestRunnerService { */ gcsResultPath: String, /** - * If enabled, HTTP requests will also be logged. Keep in mind, they might include - * sensitive data. + * If provided, the given factory will be used to configure Retrofit & OkHttp. */ - logHttpCalls: Boolean = false, + httpConfigAdapterFactory: HttpConfigAdapter.Factory = HttpConfigAdapter.Factory.NoOp, /** * The coroutine dispatcher to use for IO operations. Defaults to [Dispatchers.IO]. */ @@ -134,11 +134,6 @@ interface TestRunnerService { */ testRunDataStoreObjectKind: String = AOSP_OBJECT_KIND ): TestRunnerService { - val httpLogLevel = if (logHttpCalls) { - HttpLoggingInterceptor.Level.BODY - } else { - HttpLoggingInterceptor.Level.NONE - } val gcpConfig = Config.Gcp( credentials = credentials, projectId = firebaseProjectId @@ -162,14 +157,14 @@ interface TestRunnerService { toolsResultApi = ToolsResultApi.build( config = Config.ToolsResult( gcp = gcpConfig, - httpLogLevel = httpLogLevel, + httpConfigAdapterFactory = httpConfigAdapterFactory ) ), firebaseProjectId = firebaseProjectId, firebaseTestLabApi = FirebaseTestLabApi.build( config = Config.FirebaseTestLab( gcp = gcpConfig, - httpLogLevel = httpLogLevel, + httpConfigAdapterFactory = httpConfigAdapterFactory ) ), gcsResultPath = gcsResultPath diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/util/RetrofitExt.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/util/RetrofitExt.kt index 06d39cf..9e4fb3e 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/util/RetrofitExt.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/util/RetrofitExt.kt @@ -16,15 +16,13 @@ package dev.androidx.ci.util +import dev.androidx.ci.config.HttpConfigAdapter import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch import okhttp3.OkHttpClient import okhttp3.Request -import okhttp3.logging.HttpLoggingInterceptor import okio.Timeout -import org.apache.logging.log4j.kotlin.logger -import org.apache.logging.log4j.kotlin.loggerOf import retrofit2.Call import retrofit2.CallAdapter import retrofit2.Callback @@ -33,7 +31,6 @@ import retrofit2.Retrofit import java.lang.reflect.Type import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean -import kotlin.reflect.KClass @Target(AnnotationTarget.FUNCTION) internal annotation class Retry(val times: Int = 3) @@ -183,22 +180,12 @@ internal class RetryCallAdapterFactory( } /** - * Add a log4j logger for the given level to all requests. + * Configures the OkHttp builder with the given [config]. */ -internal fun OkHttpClient.Builder.withLog4J( - level: HttpLoggingInterceptor.Level, - klass: KClass<*> -): OkHttpClient.Builder { - return if (level == HttpLoggingInterceptor.Level.NONE) { - this - } else { - val log4jLogger = loggerOf(klass.java) - val loggingInterceptor = HttpLoggingInterceptor( - logger = { - log4jLogger.info(it) - } - ) - loggingInterceptor.level = level - this.addInterceptor(loggingInterceptor) +internal fun OkHttpClient.Builder.withConfig( + config: HttpConfigAdapter +): OkHttpClient.Builder = apply { + config.interceptors().forEach { + addInterceptor(it) } } diff --git a/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServiceImplTest.kt b/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServiceImplTest.kt index ae357a0..8677e2c 100644 --- a/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServiceImplTest.kt +++ b/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServiceImplTest.kt @@ -1,7 +1,11 @@ package dev.androidx.ci.testRunner +import com.google.cloud.NoCredentials import com.google.common.truth.Truth.assertThat +import dev.androidx.ci.config.HttpConfigAdapter import dev.androidx.ci.fake.FakeBackend +import dev.androidx.ci.firebase.FirebaseTestLabApi +import dev.androidx.ci.firebase.ToolsResultApi import dev.androidx.ci.generated.ftl.ClientInfo import dev.androidx.ci.generated.ftl.ClientInfoDetail import dev.androidx.ci.generated.ftl.EnvironmentMatrix @@ -23,8 +27,12 @@ import dev.androidx.ci.generated.testResults.ToolOutputReference import dev.androidx.ci.testRunner.vo.DeviceSetup import dev.androidx.ci.util.sha256 import kotlinx.coroutines.runBlocking +import okhttp3.Call +import okhttp3.Interceptor import org.junit.Test import java.util.UUID +import java.util.concurrent.atomic.AtomicInteger +import kotlin.reflect.KClass class TestRunnerServiceImplTest { private val fakeBackend = FakeBackend() @@ -808,6 +816,52 @@ class TestRunnerServiceImplTest { ) } + @Test + fun createWithHttpAdapter() { + class TestHttpAdapter( + val klass: KClass<*> + ) : HttpConfigAdapter { + var createCallFactoryCalls = AtomicInteger(0) + var createInterceptorCalls = AtomicInteger(0) + override fun createCallFactory(delegate: Call.Factory): Call.Factory { + createCallFactoryCalls.incrementAndGet() + return super.createCallFactory(delegate) + } + + override fun interceptors(): List { + createInterceptorCalls.incrementAndGet() + return super.interceptors() + } + } + val createdTestAdapters = mutableListOf() + val adapterFactory = HttpConfigAdapter.Factory { + TestHttpAdapter(it).also { + createdTestAdapters.add(it) + } + } + TestRunnerService.create( + credentials = NoCredentials.getInstance(), + firebaseProjectId = fakeBackend.firebaseProjectId, + bucketName = "some-bucket", + bucketPath = "some-path", + gcsResultPath = "a-result-path", + httpConfigAdapterFactory = adapterFactory + ) + assertThat( + createdTestAdapters + ).hasSize(2) + createdTestAdapters.forEach { + assertThat(it.createCallFactoryCalls.get()).isEqualTo(1) + assertThat(it.createInterceptorCalls.get()).isEqualTo(1) + } + assertThat( + createdTestAdapters.map { it.klass } + ).containsExactly( + ToolsResultApi::class, + FirebaseTestLabApi::class + ) + } + private fun TestRunnerService.ResultFileResource.readFully() = openInputStream().use { it.readAllBytes() } diff --git a/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServicePlayground.kt b/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServicePlayground.kt index 570dde2..eee19f4 100644 --- a/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServicePlayground.kt +++ b/AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServicePlayground.kt @@ -17,7 +17,6 @@ internal class TestRunnerServicePlayground { bucketName = "androidx-ftl-test-results", bucketPath = "github-ci-action", gcsResultPath = "yigit-local", - logHttpCalls = false ) as TestRunnerServiceImpl } From 95f67b4e7dfe534c75738fe5d96986ddd7627394 Mon Sep 17 00:00:00 2001 From: Yigit Boyar Date: Tue, 11 Apr 2023 11:23:09 -0700 Subject: [PATCH 2/2] reformat code --- .../androidx/ci/config/HttpConfigAdapter.kt | 2 +- .../ci/firebase/FirebaseTestLabApi.kt | 30 +++++++++---------- .../ci/testRunner/TestRunnerService.kt | 1 - 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt index e95f9ff..4f52b6d 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/config/HttpConfigAdapter.kt @@ -59,4 +59,4 @@ interface HttpConfigAdapter { ) } } -} \ No newline at end of file +} diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt index b3f150f..ef3aab5 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/firebase/FirebaseTestLabApi.kt @@ -70,21 +70,21 @@ internal interface FirebaseTestLabApi { ): FirebaseTestLabApi { val httpAdapter = config.httpConfigAdapterFactory.create(FirebaseTestLabApi::class) val client = OkHttpClient.Builder().withConfig( - httpAdapter - ).authenticateWith( - config.gcp - ).addInterceptor { - val newBuilder = it.request().newBuilder() - newBuilder.addHeader( - "Content-Type", "application/json;charset=utf-8", - ) - newBuilder.addHeader( - "Accept", "application/json" - ) - it.proceed( - newBuilder.build() - ) - }.build() + httpAdapter + ).authenticateWith( + config.gcp + ).addInterceptor { + val newBuilder = it.request().newBuilder() + newBuilder.addHeader( + "Content-Type", "application/json;charset=utf-8", + ) + newBuilder.addHeader( + "Accept", "application/json" + ) + it.proceed( + newBuilder.build() + ) + }.build() val moshi = Moshi.Builder() .add(MetadataKotlinJsonAdapterFactory()) .build() diff --git a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt index 806ca9f..4e32b6e 100644 --- a/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt +++ b/AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt @@ -19,7 +19,6 @@ import dev.androidx.ci.testRunner.vo.RemoteApk import dev.androidx.ci.testRunner.vo.UploadedApk import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers -import okhttp3.logging.HttpLoggingInterceptor import java.io.InputStream interface TestRunnerService {