Skip to content

distribution: Pass Traceparent OTEL header#49156

Merged
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:distribution-http-otel
Jan 3, 2025
Merged

distribution: Pass Traceparent OTEL header#49156
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:distribution-http-otel

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Dec 20, 2024

Wrap http.RoundTripper used by distribution code (push/pull) with the otelhttp.Transport.

- How to verify it

  1. Configure OTEL exporter for the engine (OTEL_EXPORTER_OTLP_ENDPOINT env-var)
  2. Setup a MITM proxy, eg: mitmdump -p 5050 -m reverse:<actual registry mirror> --flow-detail 2
  3. Configure the MITM proxy as an insecure registry
  4. docker pull <proxy>/<something>
  5. Observe HTTP headers in proxy

Example:

10.10.10.241:63754: HEAD http://10.10.10.232:5001/v2/alpine/manifests/latest
    Host: 10.10.10.232:5001
    User-Agent: Go-http-client/1.1
    Accept: application/json
    Accept: application/vnd.docker.distribution.manifest.v2+json
    Accept: application/vnd.docker.distribution.manifest.list.v2+json
    Accept: application/vnd.oci.image.index.v1+json
    Accept: application/vnd.oci.image.manifest.v1+json
    Accept: application/vnd.docker.distribution.manifest.v1+prettyjws
    Traceparent: 00-811d186341b98b7dcce83b20c8cdc257-2814665a74a19a71-01
    Connection: close
 << 404 Not Found 0b
    Content-Type: application/json
    Docker-Distribution-Api-Version: registry/2.0
    X-Content-Type-Options: nosniff
    Date: Fri, 20 Dec 2024 13:42:21 GMT
    Content-Length: 100
    Connection: close

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added area/distribution Image Distribution status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. labels Dec 20, 2024
@vvoland vvoland added this to the 28.0.0 milestone Dec 20, 2024
@vvoland vvoland self-assigned this Dec 20, 2024
thaJeztah
thaJeztah reviewed Dec 20, 2024
View reviewed changes
Comment thread registry/registry.go
// TODO(dmcgowan): Call close idle connections when complete and use keep alive
DisableKeepAlives: true,
}
return otelhttp.NewTransport(
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Dec 20, 2024

Choose a reason for hiding this comment

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

I saw there's also a utility for this, not sure if that's better? (possibly it allows not having to import otelhttp, but leave that to to the utility);

moby/vendor/github.com/containerd/containerd/tracing/tracing.go

Lines 51 to 59 in ad69293

// UpdateHTTPClient updates the http client with the necessary otel transport
func UpdateHTTPClient(client *http.Client, name string) {
client.Transport = otelhttp.NewTransport(
client.Transport,
otelhttp.WithSpanNameFormatter(func(operation string, r *http.Request) string {
return name
}),
)
}

Copy link
Copy Markdown
Contributor Author

@vvoland vvoland Dec 20, 2024

Choose a reason for hiding this comment

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

There is, but it takes an returns the whole http.Client instead of a http.RoundTripper so can't really use it

@vvoland vvoland force-pushed the distribution-http-otel branch from 545b92b to ed39c51 Compare December 20, 2024 14:11
@vvoland
distribution: Pass Traceparent OTEL HTTP header
93e9f7f 
Wrap `http.RoundTripper` used by distribution code (push/pull) with the
`otelhttp.Transport`.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland force-pushed the distribution-http-otel branch from ed39c51 to 93e9f7f Compare December 20, 2024 14:49
thaJeztah
thaJeztah approved these changes Dec 23, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM to align with the containerd implementation (which already passes through these headers).

Perhaps would be good to include this in a draft / test-PR in docker/cli to see if the new imports have any unwanted ripple effect w.r.t. dependencies.

@thaJeztah thaJeztah merged commit ac19b5c into moby:master Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@vvoland vvoland

Labels

area/distribution Image Distribution area/metrics/otel area/metrics kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@vvoland @cpuguy83 @thaJeztah