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

vendor: OTEL v1.19.0 / v0.45.0, containerd v1.7.11 #46830

Merged
merged 6 commits into from
Dec 12, 2023
Merged

vendor: OTEL v1.19.0 / v0.45.0, containerd v1.7.11 #46830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c14bd4f
vendor: vendor: upgrade OpenTelemetry to v1.19.0 / v0.45.0
thaJeztah Nov 19, 2023
49ad102
vendor: github.com/containerd/containerd v1.7.9
thaJeztah Nov 19, 2023
7028a03
vendor: github.com/containerd/containerd v1.7.10
thaJeztah Nov 30, 2023
fcf03cd
vendor: github.com/containerd/containerd v1.7.11
thaJeztah Dec 8, 2023
7d991b6
vendor: github.com/moby/buildkit v0.12.5-0.20231208203051-3b6880d2a00f
thaJeztah Dec 8, 2023
4d2a324
update to go.opentelemetry.io/otel/semconv/v1.21.0, remove "httpconv"…
thaJeztah Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions client/hijack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/codes"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/semconv/v1.17.0/httpconv"
"go.opentelemetry.io/otel/trace"
)

Expand Down Expand Up @@ -66,7 +65,8 @@ func (cli *Client) setupHijackConn(req *http.Request, proto string) (_ net.Conn,
}

ctx, span := tp.Tracer("").Start(ctx, req.Method+" "+req.URL.Path, trace.WithSpanKind(trace.SpanKindClient))
span.SetAttributes(httpconv.ClientRequest(req)...)
// FIXME(thaJeztah): httpconv.ClientRequest is now an internal package; replace this with alternative for semconv v1.21
// span.SetAttributes(httpconv.ClientRequest(req)...)
defer func() {
if retErr != nil {
span.RecordError(retErr)
Expand Down Expand Up @@ -98,7 +98,27 @@ func (cli *Client) setupHijackConn(req *http.Request, proto string) (_ net.Conn,
// Server hijacks the connection, error 'connection closed' expected
resp, err := clientconn.Do(req)
if resp != nil {
span.SetStatus(httpconv.ClientStatus(resp.StatusCode))
// This is a simplified variant of "httpconv.ClientStatus(resp.StatusCode))";
//
// The main purpose of httpconv.ClientStatus() is to detect whether the
// status was successful (1xx, 2xx, 3xx) or non-successful (4xx/5xx).
//
// It also provides complex logic to *validate* status-codes against
// a hard-coded list meant to exclude "bogus" status codes in "success"
// ranges (1xx, 2xx) and convert them into an error status. That code
// seemed over-reaching (and not accounting for potential future valid
// status codes). We assume we only get valid status codes, and only
// look at status-code ranges.
//
// For reference, see:
// https://github.com/open-telemetry/opentelemetry-go/blob/v1.21.0/semconv/v1.17.0/httpconv/http.go#L85-L89
// https://github.com/open-telemetry/opentelemetry-go/blob/v1.21.0/semconv/internal/v2/http.go#L322-L330
// https://github.com/open-telemetry/opentelemetry-go/blob/v1.21.0/semconv/internal/v2/http.go#L356-L404
code := codes.Unset
if resp.StatusCode >= http.StatusBadRequest {
code = codes.Error
}
span.SetStatus(code, "")
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need this part if we don't have the "httpconv.ClientRequest"? (disabled that part, per suggestion above)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe outside the scope of this PR, but worth exploring updating this logic to use http.Transport with a custom dialer since httputil.NewClientConn is deprecated. The otelhttp library could wrap the transport in that case and maybe simplify this.

Copy link
Member

Choose a reason for hiding this comment

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

Actually could probably be even easier to just wrap clientconn.Do as an http.RoundTripper and could use https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#NewTransport. Not sure if that matches what the old functionality was doing.

Copy link
Member

Choose a reason for hiding this comment

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

Playing around with that idea here #46920. I'm curious if that makes this clearer or cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @dmcgowan re: refactoring the actual (non-OTel) logic here for a future release, at which point we can bring back OTel "for free" in the process.

That said, I think your simplified status handling here is fine

}

//nolint:staticcheck // ignore SA1019 for connecting to old (pre go1.8) daemons
Expand Down
5 changes: 5 additions & 0 deletions cmd/dockerd/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import (
"github.com/spf13/pflag"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/resource"
"tags.cncf.io/container-device-interface/pkg/cdi"
)

Expand Down Expand Up @@ -238,6 +239,10 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {

setOTLPProtoDefault()
otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}))

// Override BuildKit's default Resource so that it matches the semconv
// version that is used in our code.
detect.Resource = resource.Default()
detect.Recorder = detect.NewTraceRecorder()

tp, err := detect.TracerProvider()
Expand Down
8 changes: 3 additions & 5 deletions testutil/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/resource"
"go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"gotest.tools/v3/icmd"
)

Expand All @@ -34,7 +34,7 @@ func (d devZero) Read(p []byte) (n int, err error) {

var tracingOnce sync.Once

// configureTracing sets up an OTLP tracing exporter for use in tests.
// ConfigureTracing sets up an OTLP tracing exporter for use in tests.
func ConfigureTracing() func(context.Context) {
if os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") == "" {
// No OTLP endpoint configured, so don't bother setting up tracing.
Expand All @@ -52,9 +52,7 @@ func ConfigureTracing() func(context.Context) {
tp = trace.NewTracerProvider(
trace.WithSpanProcessor(sp),
trace.WithSampler(trace.AlwaysSample()),
trace.WithResource(resource.NewSchemaless(
attribute.KeyValue{Key: semconv.ServiceNameKey, Value: attribute.StringValue("integration-test-client")},
)),
trace.WithResource(resource.NewSchemaless(semconv.ServiceName("integration-test-client"))),
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is not strictly needed, but while working on this code, I found the semconv.ServiceName utility, and thought it was slightly cleaner than to manually construct the attribute.KeyValue

)
otel.SetTracerProvider(tp)

Expand Down
25 changes: 12 additions & 13 deletions vendor.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ require (
github.com/bsphere/le_go v0.0.0-20200109081728-fc06dab2caa8
github.com/cloudflare/cfssl v1.6.4
github.com/containerd/cgroups/v3 v3.0.2
github.com/containerd/containerd v1.7.8
github.com/containerd/containerd v1.7.11
github.com/containerd/continuity v0.4.2
github.com/containerd/fifo v1.1.0
github.com/containerd/log v0.1.0
Expand Down Expand Up @@ -61,7 +61,7 @@ require (
github.com/miekg/dns v1.1.43
github.com/mistifyio/go-zfs/v3 v3.0.1
github.com/mitchellh/copystructure v1.2.0
github.com/moby/buildkit v0.12.4
github.com/moby/buildkit v0.12.5-0.20231208203051-3b6880d2a00f // v0.12 branch (v0.12.5-dev)
github.com/moby/ipvs v1.1.0
github.com/moby/locker v1.0.1
github.com/moby/patternmatcher v0.6.0
Expand Down Expand Up @@ -92,12 +92,12 @@ require (
github.com/vishvananda/netlink v1.2.1-beta.2
github.com/vishvananda/netns v0.0.4
go.etcd.io/bbolt v1.3.7
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.40.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.40.0
go.opentelemetry.io/otel v1.14.0
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.14.0
go.opentelemetry.io/otel/sdk v1.14.0
go.opentelemetry.io/otel/trace v1.14.0
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.45.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.45.0
go.opentelemetry.io/otel v1.19.0
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.19.0
go.opentelemetry.io/otel/sdk v1.19.0
go.opentelemetry.io/otel/trace v1.19.0
golang.org/x/mod v0.11.0
golang.org/x/net v0.17.0
golang.org/x/sync v0.3.0
Expand Down Expand Up @@ -196,11 +196,10 @@ require (
go.etcd.io/etcd/server/v3 v3.5.6 // indirect
go.opencensus.io v0.24.0 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace v0.40.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.14.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.14.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.14.0 // indirect
go.opentelemetry.io/otel/metric v0.37.0 // indirect
go.opentelemetry.io/proto/otlp v0.19.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.19.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.19.0 // indirect
go.opentelemetry.io/otel/metric v1.19.0 // indirect
go.opentelemetry.io/proto/otlp v1.0.0 // indirect
go.uber.org/atomic v1.9.0 // indirect
go.uber.org/multierr v1.8.0 // indirect
go.uber.org/zap v1.21.0 // indirect
Expand Down
Loading