apollo-kotlin
apollo-kotlin copied to clipboard
Passing executionContext to the platform engine
Hi!
For some logging and measurement tasks, we lack the linking of the original query data (ApolloRequest) and the data available on a specific platform (NSURLRequest or OkHttp.Request).
This way the executionContext can be help, because we can add our own object in ApolloInterceptor and read it already in the platform-dependent Engine.
On apple platform:
fun interface DataTaskFactory {
fun dataTask(request: NSURLRequest, executionContext: ExecutionContext, completionHandler: UrlSessionDataTaskCompletionHandler): NSURLSessionDataTask
}
It is also necessary not to allow the context to freeze.
On jvm platform can be use OkHttp.Request.tag for store executionContext and read in OkHttp.Interceptor.
Hi 👋 . Passing executionContext down the Http chain is a substantial and possibly breaking API change so it might take a while to do.
You can pass arbitrary HttpHeaders to the HttpEngine and then parse/log them:
val response = apolloClient.query(query).addHttpHeader("CustomName", System.currentTimeMillis())
val httpInterceptor = object: HttpInterceptor {
override suspend fun intercept(request: HttpRequest, chain: HttpInterceptorChain): HttpResponse {
val startTime = request.headers.firstOrNull { it.name == "CustomName" }?.value?.toLongOrNull()
// Do something with startTime
}
}
This is what we do for the Http Cache for an example. Would that work for you?
Yes, it was a backup plan.
I didn't want to transfer all the values of the necessary objects through headers and send them to the backend.
There was also a need to keep the original OkHttpResponse / NSURLResponse in context to better logging it, but in the end we found an easier way.
With the values in the headers, it didn't turn out so bad - most of the values are already sent to the backend. But it's unstable, and I don't want to leave it on forever.
At the moment there is a solution, but I want this opportunity to eventually appear in future versions.
Alternatively, you can do preparatory work that should not break compatibility:
- In
HttpRequestaddexecutionContextwith default = Empty. - In
DefaultHttpRequestComposeradd setexecutionContextinHttpRequest.
These actions will already allow you to work with the executionContext in HttpInterceptor. It will also be possible, if desired, to implement your Engine with the send of the executionContext to where it is needed.
Then, on each platform, you can optionally present a way to use the context to prepare a request:
- Add to
Jvm.DefaultHttpEngineoptional interface with function (no suspend) (Request, ExecutionContext) -> Request. - Add to
Apple.DefaultHttpEngineoptional interface with function (no suspend) (NSURLRequest, ExecutionContext) -> NSURLRequest.
This will allow OkHttp to store the necessary information in the tag in Request. In the case of NSURLSession, it will be possible to save the necessary information somewhere by a reference to NSURLRequest and receive it in the DataFactory.
With Response, you may also need to think about it, but at the expense of Interceptors, there are no problems accessing the context for Responses from Requests.
I didn't want to transfer all the values of the necessary objects through headers and send them to the backend.
Note you can always filter out headers in your HttpInterceptor if you don't want them to reach your backend:
val interceptor = object : HttpInterceptor {
override suspend fun intercept(request: HttpRequest, chain: HttpInterceptorChain): HttpResponse {
return chain.proceed(request.newBuilder().headers(request.headers.filter { !it.name.startsWith("internal-") }).build())
}
}
In HttpRequest add executionContext with default = Empty.
Indeed adding a field to HttpRequest is a backward compatible change so that'd work 👍 . I'm still wondering whether ExecutionContext is the correct abstraction there. The HttpEngine API is very small at the moment and we might want to keep it this way and not pull too much from apollo-runtime in there.
Fixed in https://github.com/apollographql/apollo-kotlin/pull/5383