Skip to content

Enable Go HTTP tracing of registry interactions#5040

Merged
dmcgowan merged 1 commit intocontainerd:masterfrom
estesp:http-trace
Mar 3, 2021
Merged

Enable Go HTTP tracing of registry interactions#5040
dmcgowan merged 1 commit intocontainerd:masterfrom
estesp:http-trace

Conversation

@estesp
Copy link
Copy Markdown
Member

@estesp estesp commented Feb 17, 2021 •
edited
Loading

Enables a Go embedded feature for tracing certain aspects of HTTP connections; useful for debug of connectivity, DNS, etc. issues with registry interactions.

Example output:

DEBU[0000] fetching                                      image="docker.io/library/alpine:latest"
DEBU[0000] resolving                                     host=registry-1.docker.io
DEBU[0000] do request                                    host=registry-1.docker.io request.header.accept="application/vnd.docker.distribution.manifest.v2+json, ap
plication/vnd.docker.distribution.manifest.list.v2+json, application/vnd.oci.image.manifest.v1+json, application/vnd.oci.image.index.v1+json, */*" request.header.
user-agent=containerd/v1.5.0-beta.1-9-g87c9aeb6c.m request.method=HEAD url="https://registry-1.docker.io/v2/library/alpine/manifests/latest"
DEBU[0000] DNS lookup                                    host=registry-1.docker.io
DEBU[0000] DNS lookup complete                           coalesced=false result=34.238.187.50
DEBU[0000] Connection successful                         remote_addr="34.238.187.50:443" reused=false

dims
dims reviewed Feb 17, 2021
View reviewed changes
Comment thread remotes/docker/resolver.go Outdated
@estesp estesp force-pushed the http-trace branch 2 times, most recently from 7342933 to 51eb80f Compare February 17, 2021 03:28
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 17, 2021

Build succeeded.

@containerd containerd deleted a comment from theopenlab-ci Bot Feb 17, 2021
@containerd containerd deleted a comment from theopenlab-ci Bot Feb 17, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 17, 2021

Build succeeded.

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Feb 17, 2021

It seems like we should store the tracer in a context that the client creates rather than using a hard-coded tracer?
Something like ctx = docker.WithHTTPTracer(ctx, &httprace.ClientTrace{})

WDYT?

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Feb 17, 2021

This has the benefit of having more contextual information in the log entries.

AkihiroSuda
AkihiroSuda reviewed Feb 18, 2021
View reviewed changes
Comment thread cmd/ctr/commands/commands.go Outdated
kzys
kzys reviewed Feb 18, 2021
View reviewed changes
Comment thread remotes/docker/resolver.go Outdated
Comment on lines 142 to 155
Copy link
Copy Markdown
Member

@kzys kzys Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @cpuguy83. The default tracer is fine for ctr but assuming all containerd clients need these three events looks a bit odd.

@estesp
Copy link
Copy Markdown
Member Author

estesp commented Feb 19, 2021

@cpuguy83 @kzys yeah, makes sense really. Latest commit drops the integration with server side and just uses httptrace client tracer on two commands (image push|pull); I was digging into something related to CRI, but don't think it makes sense to add anything permanent there as the correct long-term option for that would probably be a CRI config option to specify a list of items to turn on via httptrace (which can only do a few things anyway)

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 19, 2021

Build succeeded.

kzys
kzys approved these changes Feb 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cpuguy83
cpuguy83 approved these changes Feb 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cpuguy83
cpuguy83 approved these changes Feb 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dims
dims approved these changes Feb 24, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dmcgowan
dmcgowan reviewed Feb 24, 2021
View reviewed changes
Comment thread cmd/ctr/commands/images/pull.go Outdated
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this sort of data would fit well in `WithFields

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 25, 2021

Looks good. The only think i would change would be to factor out the trace client code into a NewDebugTraceClient or something similar.

@estesp
Copy link
Copy Markdown
Member Author

estesp commented Mar 3, 2021

@crosbymichael @dmcgowan thanks for the reviews--updated with use of WithField and made a common tracer func

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 3, 2021

Build succeeded.

fuweid
fuweid approved these changes Mar 3, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mxpv
mxpv reviewed Mar 3, 2021
View reviewed changes
Comment thread cmd/ctr/commands/images/pull.go
Usage: "pull content and metadata from all platforms",
},
cli.BoolFlag{
Name: "trace",
Copy link
Copy Markdown
Member

@mxpv mxpv Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name this http-trace by any chance? (because I'm about to add http-dump for debugging http requests/responses)

Comment thread cmd/ctr/commands/images/push.go
}

if context.Bool("trace") {
ctx = httptrace.WithClientTrace(ctx, NewDebugClientTrace(ctx))
Copy link
Copy Markdown
Member

@mxpv mxpv Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be relevant for all registry interactions, not just push/pull.
What do you think about moving this to GetResolver, so all ctr sub commands would benefit from this.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Mar 3, 2021

@mxpv I suggest just making those updates in a follow up PR if you are already adding more similar functionality. I also have some related metrics I am working around here as well. LGTM on this initial PR.

@dmcgowan dmcgowan merged commit 1dcfe7f into containerd:master Mar 3, 2021
@mxpv mxpv mentioned this pull request Mar 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan left review comments

@mxpv mxpv mxpv left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@kzys kzys kzys approved these changes

@dims dims dims approved these changes

@samuelkarp samuelkarp samuelkarp approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@estesp @cpuguy83 @crosbymichael @dmcgowan @kzys @dims @samuelkarp @mxpv @AkihiroSuda @fuweid