auth: add span to FetchToken helpers #10231
Conversation
Before this, during a call to the docker resolver, we would generate span wrappers for each HTTPRequest correctly, however, as the docker resolver reaches out to the docker authorizer, it could create HTTP requests (for fetching tokens) that would not be wrapped in any span. This can result in rather confusing traces, e.g. something like: remotes.docker.resolver.HTTPRequest HTTP HEAD (fetch index, fails with 401) HTTP GET (fetch token) remotes.docker.resolver.HTTPRequest HTTP HEAD (fetch index) remotes.docker.resolver.HTTPRequest HTTP GET (fetch manifest) By adding a span into the FetchToken, this trace becomes a little easier to consume: remotes.docker.resolver.HTTPRequest HTTP HEAD (fetch index, fails with 401) remotes.docker.resolver.FetchToken HTTP GET (fetch token) remotes.docker.resolver.HTTPRequest HTTP HEAD (fetch index) remotes.docker.resolver.HTTPRequest HTTP GET (fetch manifest) Signed-off-by: Justin Chadwell <me@jedevc.com>
|
Hi @jedevc. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Bump - any issues with this? |
| @@ -95,6 +96,10 @@ type OAuthTokenResponse struct { | |||
|
|
|||
| // FetchTokenWithOAuth fetches a token using a POST request | |||
| func FetchTokenWithOAuth(ctx context.Context, client *http.Client, headers http.Header, clientID string, to TokenOptions) (*OAuthTokenResponse, error) { | |||
| c := *client | |||
| client = &c | |||
| tracing.UpdateHTTPClient(client, tracing.Name("remotes.docker.resolver", "FetchTokenWithOAuth")) | |||
There was a problem hiding this comment.
Not 100% sure what the conventions are; should this include "auth" (remotes.docker.auth.resolver) if this follows the name of the package we're in?
Before this, during a call to the docker resolver, we would generate span wrappers for each HTTPRequest correctly, however, as the docker resolver reaches out to the docker authorizer, it could create HTTP requests (for fetching tokens) that would not be wrapped in any span.
This can result in rather confusing traces, e.g. something like:
By adding a span into the FetchToken, this trace becomes a little easier to consume:
I think this is the right kind of approach for this. However, maybe it makes more sense to add this in the
authHandlerinstead? Happy to discuss 🎉