search

canonical / gomaasclient

Go MAAS client
Apache License 2.0
23 stars 28 forks source link

Support for observability & context propagation #32

Closed Sovietaced closed 10 months ago

Sovietaced commented 12 months ago

Hello, first of all I just wanted to say thanks for making this library! Definitely nice to to write HTTP requests from scratch.

One thing I noticed that its not really possible for users to instrument any of the calls made to MAAS. There is no option to pass a pluggable HTTP transport and even if we did, none of the APIs accept a context.Context in their signature. I believe it is idiomatic in go to plumb contexts around to all API methods.

Anyway, I'm happy to contribute to this.

skatsaounis commented 11 months ago

Hi @Sovietaced. Thank you for opening this issue and your willingness to contribute to the project.

For the first part of your suggestions, the private function here could be modified to accept a custom transport as another argument. In addition, we could add a new public function that uses the private one in a way that the transport can be set.

For the second part of the suggestions, I am afraid that there is no clear way forward. For historical reasons, this Go client has been created to extend the gomaasapi. That other client is used by Juju to manage a MAAS cloud, it existed before gomaasclient and it also has limited support for MAAS API. If you try to explore its code, you will quickly understand that you cannot set context to any HTTP call with any method. (e.g., https://github.com/juju/gomaasapi/blob/v2/client.go#L163).

If you think that implementing part 1 would add value to the client use cases, feel free to open a PR and I will gladly review it.

Sovietaced commented 11 months ago

For the second part of the suggestions, I am afraid that there is no clear way forward. For historical reasons, this Go client has been created to extend the gomaasapi. That other client is used by Juju to manage a MAAS cloud, it existed before gomaasclient and it also has limited support for MAAS API. If you try to explore its code, you will quickly understand that you cannot set context to any HTTP call with any method. (e.g., https://github.com/juju/gomaasapi/blob/v2/client.go#L163).

OK thanks. I will file a separate bug for that.

If you think that implementing part 1 would add value to the client use cases, feel free to open a PR and I will gladly review it.

Sounds good