Clean up go clients

[timstclair]

Clients - Should have a clear package with minimal dependencies for go client code. Most users should only need to import a versioned client and API

• Move current ‘client’ code to ‘client/v1’
• Define interfaces for REST clients
• Add library client implementation (wrapper around manager)

community-assigned: @Thiht

[timstclair]

@Thiht - Please add your suggestions for improvements to this tracking issue, and I'll be happy to review your PRs.

[Thiht]

Great!

Here are a few expectations I have from the client:

• Every REST endpoint should be exposed in the client
  • List of the available endpoints
  • The methods should take a nilable/optional v2.RequestOptions whenever it makes sense
    • It would be nice to have a DefaultRequestOptions somewhere with the default values
  • The method names should match the endpoint name (machine -> Machine() instead of MachineInfo() for example)
  • Obviously add tests for every method
    • Currently Stats is broken, it should return a map[string][]v2.DeprecatedContainerStats instead of a map[string]v2.ContainerInfo
      • And maybe it'd be better for the resource to be a map[string][]v2.ContainerInfo, this "Deprecated" is scary :)
• When creating a client, the version shouldn't be hardcoded but parameterizable
  • Maybe this would result in a single client able to handle both v1 and v2? I don't know if it's doable or a good idea
• At least httpGetJsonData should be publicly exposed to allow users to access non-exposed endpoints (in case the development of the API moves faster than the client)
  • This method should be more specialized and handle baseUrl internally, it would allow to get rid of the *Url helper methods. Thus it would just need an endpoint string parameter instead of url string

These modifications would have helped me a lot when using it!

As for moving client to client/v1, won't it result in breaking the existing builds? Or is it not an issue since cAdvisor is still 0.x.x?

[vishh]

I don't think client code is expected to be as stable as the wire format
itself. So we can change it. Users should have vendored the existing client
code.

On Thu, Mar 24, 2016, 4:02 AM Thibaut Rousseau notifications@github.com
wrote:

Great!

Here are a few expectations I have from the client:

• Every REST endpoint should be exposed in the client

  • List of the available endpoints
    https://github.com/google/cadvisor/blob/master/api/versions.go
  • The methods should take a nilable/optional v2.RequestOptions
    whenever it makes sense

   - It would be nice to have a DefaultRequestOptions somewhere
   with the default values
   <https://github.com/google/cadvisor/blob/05068c196a3c3343329258662ce624a9464d6b6f/api/versions.go#L518-L522>
  

  • The method names should match the endpoint name (machine ->
    Machine() instead of MachineInfo()
  • Obviously add tests for every method
    • Currently Stats
      https://github.com/google/cadvisor/blob/master/client/v2/client.go#L91
      is broken, it should return a
      map[string][]v2.DeprecatedContainerStats instead of a
      map[string]v2.ContainerInfo
      • And maybe it'd be better for the resource to be a
        map[string][]v2.ContainerInfo, this "Deprecated" is scary :)
    • When creating a client, the version shouldn't be hardcoded
      https://github.com/google/cadvisor/blob/master/client/v2/client.go#L45
      but parameterizable
  • Maybe this would result in a single client able to handle both v1
    and v2? I don't know if it's doable or a good idea

• At least httpGetJsonData
  https://github.com/google/cadvisor/blob/master/client/v2/client.go#L166
  should be publicly exposed to allow users to access non-exposed endpoints
  (in case the development of the API moves faster than the client)

  • This method should be more specialized and handle baseUrl
    https://github.com/google/cadvisor/blob/master/client/v2/client.go#L35
    internally, it would allow to get rid of the *Url helper methods
    https://github.com/google/cadvisor/blob/master/client/v2/client.go#L107-L125.
    Thus it would just need an endpoint string parameter instead of url
    string

These modifications would have helped me a lot when using it!

As for moving client to client/v1, won't it result in breaking the
existing builds? Or is it not an issue since cAdvisor is still 0.x.x?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1180 (comment)

[timstclair]

Great suggestions! A few notes:

When creating a client, the version shouldn't be hardcoded but parameterizable
Maybe this would result in a single client able to handle both v1 and v2? I don't know if it's doable or a good idea

The V1 and V2 clients may expose different methods, and those methods are going to have different input and output parameter types. I don't think there is a good way of accomplishing this point without using interface{} types to circumvent the type system, which is a bad idea. I think it's better to require users to explicitly import the client type they want, even if the constructors for both look the same.

At least httpGetJsonData should be publicly exposed to allow users to access non-exposed endpoints (in case the development of the API moves faster than the client)

I think this would be better solved with policy / pre-submit checks: require additional endpoints to be handled in the clients.

As for moving client to client/v1, won't it result in breaking the existing builds?

A lot of the big cleanup changes we're proposing will break builds that switch to the new code. As @vishh mentioned, users should be vendoring their dependencies to avoid this. However, I've created an api-cleanup branch for staging breaking changes for now. This way we can merge all the breaking changes into the master branch at the same time, and clients will only need to be updated once (it will be a big update - there are ways we can help with it).

[Thiht]

Vendoring seems to be the way to go with Go so you're right, these changes should not be a problem. I'm not completely familiar with this way of doing things yet!

The api-cleanup branch seems like a good compromise to me.

---

• At least httpGetJsonData should be publicly exposed to allow users to access non-exposed endpoints (in case the development of the API moves faster than the client)

I think this would be better solved with policy / pre-submit checks: require additional endpoints to be handled in the clients.

It would be ideal if you think that's feasible. Exposing the method is a nice workaround for the users when not everything is available in the client, but I agree it would be better to keep the client and the API synced. It's a bit more work but I'm confident it can be handled.

---

• When creating a client, the version shouldn't be hardcoded but parameterizable
  • Maybe this would result in a single client able to handle both v1 and v2? I don't know if it's doable or a good idea

The V1 and V2 clients may expose different methods, and those methods are going to have different input and output parameter types. I don't think there is a good way of accomplishing this point without using interface{} types to circumvent the type system, which is a bad idea. I think it's better to require users to explicitly import the client type they want, even if the constructors for both look the same.

I quick-checked and I see what you mean, let's keep v1 and v2 separate then. Is there the same issue between the v2 minor versions? Because it would be really handy for the user to be able to specify at least the minor version when communicating with servers using an older cAdvisor version

[timstclair]

Is there the same issue between the v2 minor versions?

I believe the minor version differences are mostly in the endpoints / request API, which should be hidden by the client wrapper, so I think we can just provide a single v1 and v2 client based on the most recent minor release.

[Thiht]

I started working with:

• This method should be more specialized and handle baseUrl internally, it would allow to get rid of the *Url helper methods. Thus it would just need an endpoint string parameter instead of url string

on my fork. I noticed the httpGet* methods take a postData argument, which doesn't really make sense semantically in a REST context. Moreover, it doesn't seem to be used anywhere (yet?) as far as I can tell. Do you mind if I remove it completely, or at the very least split the "httpGet* methods with postData argument" in "httpGet* methods and httpPost* methods", which would be nicer on the eyes?

[timstclair]

I noticed the httpGet* methods take a postData argument, which doesn't really make sense semantically in a REST context.

I agree this needs to be cleaned up, both on the client side and the server side, and would be happy to accept a patch. On the server side, I'd like to see the v1.ContainerInfoRequest and other requests encoded / decoded as query params to provide propre GET endpoints.

[Thiht]

Hello,
I've got a little problem with the v2 API I don't really know how to solve properly. The problem is /api/v2.0/stats and /api/v2.1/stats respectively return map[string][]v2.DeprecatedContainerStats and map[string]v2.ContainerInfo, two different types.

So I don't really know how to represent this in the client. I was thinking maybe making two methods would be an acceptable solution:

// Named Stats2_0 to highlight the fact it's an old version (or "StatsLegacy" or whatever)
func (*Client) Stats2_0(...) (map[string][]v2.DeprecatedContainerStats, error) {
  // ...
}

// Normal name for the 2.1 method
func (*Client) Stats(...) (map[string]v2.ContainerInfo, error) {
  // ...
}

Do you have another solution? The only other idea I have in mind would be for Stats to return an interface{} for the user to assert with the correct type but it does seem a bit ugly.

[timstclair]

As I mentioned above, I think we should hide the details of the minor versions in the client. In other words, let's provide a single V2 client interface which provides the latest v2 API. The client should handle backwards compatability, and upconvert the deprecated version to the latest version. Does that make sense?