elixir-google-api

elixir-google-api copied to clipboard

Hi everyone!

Thanks for making those libraries available as they really help many Elixir projects bootstrap on Google Cloud. 👍

It seems today Google API is built on Tesla but I would like to request a feature where Google API actually defines an HTTP Client contract that should be implemented. My main concern is the cascading layers that we go through to do any sort of HTTP connection/polling customization or instrumentation.

For example, if I am using Google PubSub, it is built on top of Google.Gax:

https://github.com/GoogleCloudPlatform/elixir-google-api/blob/master/clients/pub_sub/lib/google_api/pub_sub/v1/connection.ex

Google.Gax connection is built on top of Tesla:

https://github.com/GoogleCloudPlatform/elixir-google-api/blob/bbf9cf4007852d1d9108fe43608afe5c74179bb6/clients/gax/lib/google_api/gax/connection.ex

And tesla itself is a "meta" client with multiple adapters:

https://github.com/teamon/tesla/#adapters

Inside the adapters, then we can find the actual HTTP client implementation, which defaults to httpc (which is not recommended for production). Tesla has information on how to change the adapters but it has no information on how to control the pool size, set connection wide settings, etc (or at least I couldn't find any).

Therefore, for me to do any customization, metrics, monitoring, etc on how HTTP requests come and go in my system, we need to go through all of those layers, and it is still not clear how this can be achieved.

This may be perhaps what Tesla aims to achieve but custom adapters are not strongly highlighted in their docs at the moment. Plus it seems that you already have your own request builder and a very thin contract around the connection, so from a glance it makes sense to expose it rather than force users to go through gax -> tesla -> adapter.

To clarify, I am not saying that Gax.Connection should be replaceable, as it still has some domain information, but rather that the places you call Tesla inside Gax.Connection could be abstracted in favor of a simpler contract. Since you already pass the Gax.Connection around, you could keep the "connection backend" stored there too. Thanks for reading!

EDIT: Removed incorrect suggestion to pass :connection as option as it is already passed as argument.

Thanks Jose for the great suggestion!

Defining the connection API seems like an easy step to take. We picked Tesla for it's extensibility and multipart request handling but we're not doing a great job of surfacing it. We'd also like to make Tesla an implementation detail of the default connections (but document how to extend it). Currently, Tesla structs bleed out into the developers' code.

Great! I have also opened two issues on Tesla, in case you'd rather expose it: https://github.com/teamon/tesla/issues/218 and https://github.com/teamon/tesla/issues/219

DISCLAIMER: I'm obviously biased here

The way I've always imagined client libraries build around tesla is that they expose a way to pass a custom %Tesla.Client{} to add/change functionality (like fuse/timeout/retry/headers/adapter/etc).

If you need a dynamic client you could do something like:

# MyApi

def new(token, middleware \\ []) do
  Tesla.build_client([{Tesla.Middleware.Header, "Bearer #{token}"}] ++ middleware)
end

# app code
client = MyApi.new(token, my_custom_middleware)
MyApi.some_function(client)

So, instead of hiding tesla it is actually exposed as first-class citizen and out of the box compatible with 3rd party tesla extensions. For example, need caching? - plug in tesla_cache middleware into your custom client.

I agree tesla docs are not the best currently but we are working on them! ( https://github.com/teamon/tesla/pull/228 )

@teamon this would be a valid approach as long as everything in Tesla can be done at runtime. Middleware are fine. However, you can't configure adapters at runtime, which is the reason why I opened this issue and the ones in Tesla.

The only way to make the adapter customizable with the current Tesla version would be if Google API added a global compile time configuration.

The other limitation is that it is unclear how to customize the adapters in Tesla. So if I need to tune hackney, I need to figure out how to go all the way from Google API -> Tesla -> Hackney, and that's not easy. As long as one of those contracts are well defined (Google API -> HTTP client) or (Tesla -> HTTP client), then we should be good to go.

You can override adapter in runtime, but it's not pretty and it needs to be improved a lot.

iex(1)> Tesla.get("https://httpbin.org/ip")
{:ok, %Tesla.Env{status: 200, ...}}

iex(2)> client = Tesla.build_adapter(fn env -> IO.inspect("Custom adapter") end)

iex(3)> Tesla.get(client, "https://httpbin.org/ip")
"Custom adapter"
# no request has been made

(Adapter is just the last middleware that does not call Tesla.run)

Regarding adapter customization - docs docs docs + best practices/tips & tricks

@teamon build_adapter has no documentation. So it is unclear if it is a private function that was accidentally public or something that is meant to be used. build_adapter also does not receive the client as argument, so I still unclear how to change the adapter of an existing client, which is likely what I would expect here.

In any case, the lack of documentation around those customizations are precisely what those issues are about. :) Given Tesla defaults to httpc, which I don't recommend for production usage, I would say replacing and customize adapters should be more prominently featured.

Yes, yes, and yes, all you say is (unfortunately) true. Thanks for starting the discussion around it! I hope we can get it solved in the near future.

Any update on this issue?

Tesla seems to have updated the api, so one could pass the adapter options directly now Tesla.client(middleware, adapter). But this is still not exposed by GoogleApi.Gax.Connection.

We are currently working around the issue by configuring the adapter globally. But we really would like to avoid global config, as this means we can't use different adapter configs (like no proxy for internal services) for different client.

config :tesla,
  adapter: {Tesla.Adapter.Hackney, [pool: Client.BigQuery, proxy: {host, port}]}

@ananthakumaran & others, you can specify a particular client for which the config applies. For example, for the DialogFlow client:

config :tesla, GoogleApi.Dialogflow.V2.Connection,
  adapter: {Tesla.Adapter.Hackney, [pool: :dialogflow]}

Currently found this issue when trying to define a new middleware for the Tesla client like adding the Tesla.Middleware.Timeout. Interested to see if that's possible in another way and/or the state of this issue.