feat: more context on docker client#47518
feat: more context on docker client#47518Benehiko wants to merge 2 commits intomoby:masterfrom Benehiko:docker-client-ctx
Conversation
Signed-off-by: Alano Terblanche <[email protected]>
laurazard
added
area/api
| func (cli *Client) ClientVersion(_ context.Context) string { | ||
| return cli.version | ||
| } |
There was a problem hiding this comment.
Doesn't look like we're using it here, so not sure we should add
There was a problem hiding this comment.
I think it might be necessary if we want full trace visibility of the call stack from otel. But I could be wrong here and maybe it's unnecessary as you say
| // ) | ||
| func NewClientWithOpts(ops ...Opt) (*Client, error) { | ||
| func NewClientWithOpts(ctx context.Context, ops ...Opt) (*Client, error) { | ||
| hostURL, err := ParseHostURL(DefaultDockerHost) |
There was a problem hiding this comment.
This will likely be the tricky bit to look at; should a client get a context, or should the context be scoped to an action (like client.MakeSomeAPIRequest). I'm included to say for the client, it would be the latter (per call), and it may be up to the consumer of the client to keep a "global" context if things must be inherited from that. 🤔
There was a problem hiding this comment.
I think this is a similar argument of otel trace visibility over actual context usage (like cancelling io bound tasks).
Signed-off-by: Alano Terblanche <[email protected]>
3 participants
This is a WIP and a breaking change to the docker client APIs. Most likely required by OTEL for better tracing.
Relates to
- What I did
Add context to more of the docker client function calls.
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)