Maybe this is not a topic for this PR, but I would like to discuss it anyway.

Up until now we've had only one OkHttpClient in the whole SDK here.

If you create OkHttpClient the way you do here it will create its own thread pool and connection pool which is most likely is a waste of resources for no reason. See this doc for example.

I suggest we do something like this to share the underlying thread pool and connection pool between different OkHttpClients in our SDK.

This also has the benefit that we can add some common settings for all clients (like cache). Not sure that we need it in this particular case though.

WDYT?

I think that's a great idea, and thank you for bringing it up here!

Great!

One more question. Do you think that OkHttpClient for the flags feature should have the same configuration as the one we already had in the SDK (link)? Or it should be different?

It of course depends on the backend. As far as I see, the host for flags will be different than for the rest of the SDK (this).

No strong opinion from me here.

It should have the same configuration given Flags are supposed to work in GovCloud as well, which puts restrictions on the set of TLS suites allowed for the FIPS compliance.

just to note, Flags is not currently supported in GovCloud, (the intent is for it to be, but we've not scoped a GovCloud deploy yet).

I think it's useful to use the same existing configuration and just allow certain overrides in the FlagsClient on parameters like timeout and number of retries.
We're not speccing any customization in the API at current, so I think copying the existing configuration makes the most sense, especially given it is already hardened

(I'll update my code here)

internal lateinit var callFactory: OkHttpCallFactory -> private val callFactory: OkHttpCallFactory

This isn't actually a "no-op" implementation. It will create a real OkHttpClient. You have to create a NoOpCallFactory.

Here it is:

object NoOpCallFactory: Call.Factory {
    
    object NoOpCall: Call {
        override fun cancel() {
            
        }

        override fun clone(): Call {
            return this
        }

        override fun enqueue(responseCallback: Callback) {
            
        }

        override fun execute(): Response {
            return Response.Builder().build()
        }

        override fun isCanceled(): Boolean {
            return false
        }

        override fun isExecuted(): Boolean {
            return false
        }

        override fun request(): Request {
            return Request.Builder().build()
        }

        override fun timeout(): Timeout {
            return Timeout.NONE
        }

    }
    override fun newCall(request: Request): Call {
        return NoOpCall
    }
}

thanks!
Would it make sense for the noop call factory to return the request it was created with?

Would it make sense for the noop call factory to return the request it was created with?

yes, this is better, indeed

Why are we using JSON here? You can create something like data class SiteConfig(val dc: String, val tld: String).

And siteConfig can become Map<DatadogSite, SiteConfig>.

agree with you here. changed

actually, #2922 is an even cleaner approach

minor: okhttp3 should go to import

done

minor: maybe it is worth to use default lambda argument in the createOkHttpCallFactory signature? Otherwise such call with empty lambda looks a bit strange.

added to the interface. empty () certainly looks more normal here, thx

minor: seems like useCustomFlagEndpoint call can go on another line. Formatting is off in this call chain.

fixed

seems like this line should be removed and there is no actual usage of mockDatadogContext above?

🧹

it actually still the same, the suggested change is not applied here and in the tests below

🧹

now done

done

added to the interface. empty () certainly looks more normal here, thx

fixed; I'll connect w/Jonathan on verifying my ktlint config