Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

distribution: Pass Traceparent OTEL header #49156

Merged
merged 1 commit into from
Jan 3, 2025
Merged

distribution: Pass Traceparent OTEL header #49156

merged 1 commit into from
Jan 3, 2025
+33 −12

Conversation

vvoland
Copy link
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 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
registry/registry.go
// TODO(dmcgowan): Call close idle connections when complete and use keep alive
DisableKeepAlives: true,
}
return otelhttp.NewTransport(
Copy link
Member

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
Contributor Author

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 <pawel.gronowski@docker.com>
@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
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
140 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

@vvoland vvoland

Labels
area/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