Skip to content

Commit 228e250

Browse files
jsternbergjsternberg
committed
detect: refactor the detect package
This refactors the detect package with the goal of making it more similar to otel's `autoexport` package and splitting out the additional functionality used by buildkit, like the trace recorder and delegated tracer, to more explicit processors rather than implicit through `autoexport`. This removes the global variables for the trace provider and meter provider along with the global variable for the exporters. This is replaced with functions that create the exporters. The delegated tracer has been removed from detect and moved into the normal tracing util package. This is still used by the command line to send delegated traces, but it's an explicit exporter that's added rather than implicit. Some functions have been renamed mostly to force dependent packages to change their usage rather than have a chance at incorrect usage because the semantics changed. Signed-off-by: Jonathan A. Sternberg <[email protected]>
1 parent b3cee61 commit 228e250

10 files changed

Lines changed: 229 additions & 227 deletions

File tree

‎cmd/buildctl/common/common.go‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212
"time"
1313

1414
"github.com/moby/buildkit/client"
15-
"github.com/moby/buildkit/util/tracing/detect"
15+
"github.com/moby/buildkit/util/tracing/delegated"
1616
"github.com/pkg/errors"
1717
"github.com/urfave/cli"
1818
"go.opentelemetry.io/otel/trace"
@@ -68,16 +68,10 @@ func ResolveClient(c *cli.Context) (*client.Client, error) {
6868
ctx := CommandContext(c)
6969
var opts []client.ClientOpt
7070
if span := trace.SpanFromContext(ctx); span.SpanContext().IsValid() {
71-
opts = append(opts, client.WithTracerProvider(span.TracerProvider()))
72-
73-
exp, _, err := detect.Exporter()
74-
if err != nil {
75-
return nil, err
76-
}
77-
78-
if td, ok := exp.(client.TracerDelegate); ok {
79-
opts = append(opts, client.WithTracerDelegate(td))
80-
}
71+
opts = append(opts,
72+
client.WithTracerProvider(span.TracerProvider()),
73+
client.WithTracerDelegate(delegated.DefaultExporter),
74+
)
8175
}
8276

8377
if caCert != "" {

‎cmd/buildctl/common/trace.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,28 @@ import (
55
"os"
66

77
"github.com/moby/buildkit/util/appcontext"
8+
"github.com/moby/buildkit/util/tracing/delegated"
89
"github.com/moby/buildkit/util/tracing/detect"
910
"github.com/urfave/cli"
1011
"go.opentelemetry.io/otel/attribute"
1112
"go.opentelemetry.io/otel/codes"
13+
sdktrace "go.opentelemetry.io/otel/sdk/trace"
1214
"go.opentelemetry.io/otel/trace"
1315
)
1416

1517
func AttachAppContext(app *cli.App) error {
1618
ctx := appcontext.Context()
1719

18-
tp, err := detect.TracerProvider()
20+
exp, err := detect.NewSpanExporter(ctx)
1921
if err != nil {
2022
return err
2123
}
24+
25+
tp := sdktrace.NewTracerProvider(
26+
sdktrace.WithResource(detect.Resource()),
27+
sdktrace.WithBatcher(exp),
28+
sdktrace.WithBatcher(delegated.DefaultExporter),
29+
)
2230
tracer := tp.Tracer("")
2331

2432
var span trace.Span
@@ -60,7 +68,7 @@ func AttachAppContext(app *cli.App) error {
6068
if span != nil {
6169
span.End()
6270
}
63-
return detect.Shutdown(context.TODO())
71+
return tp.Shutdown(context.TODO())
6472
}
6573
return nil
6674
}

‎cmd/buildctl/main.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616
"github.com/moby/buildkit/util/appdefaults"
1717
"github.com/moby/buildkit/util/profiler"
1818
"github.com/moby/buildkit/util/stack"
19-
_ "github.com/moby/buildkit/util/tracing/detect/delegated"
2019
_ "github.com/moby/buildkit/util/tracing/detect/jaeger"
2120
_ "github.com/moby/buildkit/util/tracing/env"
2221
"github.com/moby/buildkit/version"

‎cmd/buildkitd/main.go‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
sddaemon "github.com/coreos/go-systemd/v22/daemon"
2424
"github.com/docker/docker/pkg/reexec"
2525
"github.com/gofrs/flock"
26+
"github.com/hashicorp/go-multierror"
2627
"github.com/moby/buildkit/cache/remotecache"
2728
"github.com/moby/buildkit/cache/remotecache/azblob"
2829
"github.com/moby/buildkit/cache/remotecache/gha"
@@ -63,7 +64,9 @@ import (
6364
"github.com/urfave/cli"
6465
"go.etcd.io/bbolt"
6566
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
67+
"go.opentelemetry.io/otel/exporters/prometheus"
6668
"go.opentelemetry.io/otel/propagation"
69+
sdkmetric "go.opentelemetry.io/otel/sdk/metric"
6770
sdktrace "go.opentelemetry.io/otel/sdk/trace"
6871
tracev1 "go.opentelemetry.io/proto/otlp/collector/trace/v1"
6972
"golang.org/x/sync/errgroup"
@@ -217,6 +220,7 @@ func main() {
217220
app.Flags = append(app.Flags, appFlags...)
218221
app.Flags = append(app.Flags, serviceFlags()...)
219222

223+
var closers []func(ctx context.Context) error
220224
app.Action = func(c *cli.Context) error {
221225
// TODO: On Windows this always returns -1. The actual "are you admin" check is very Windows-specific.
222226
// See https://github.com/golang/go/issues/28804#issuecomment-505326268 for the "short" version.
@@ -259,15 +263,17 @@ func main() {
259263
}
260264
}
261265

262-
tp, err := detect.TracerProvider()
266+
tp, err := newTracerProvider(ctx)
263267
if err != nil {
264268
return err
265269
}
270+
closers = append(closers, tp.Shutdown)
266271

267-
mp, err := detect.MeterProvider()
272+
mp, err := newMeterProvider(ctx)
268273
if err != nil {
269274
return err
270275
}
276+
closers = append(closers, mp.Shutdown)
271277

272278
statsHandler := tracing.ServerStatsHandler(
273279
otelgrpc.WithTracerProvider(tp),
@@ -375,8 +381,13 @@ func main() {
375381
return err
376382
}
377383

378-
app.After = func(_ *cli.Context) error {
379-
return detect.Shutdown(context.TODO())
384+
app.After = func(_ *cli.Context) (err error) {
385+
for _, c := range closers {
386+
if e := c(context.TODO()); e != nil {
387+
err = multierror.Append(err, e)
388+
}
389+
}
390+
return err
380391
}
381392

382393
profiler.Attach(app)
@@ -737,13 +748,19 @@ func newController(c *cli.Context, cfg *config.Config) (*control.Controller, err
737748
return nil, err
738749
}
739750

740-
tc, _, err := detect.Exporter()
741-
if err != nil {
751+
tc := make(tracing.MultiSpanExporter, 0, 2)
752+
if detect.Recorder != nil {
753+
tc = append(tc, detect.Recorder)
754+
}
755+
756+
if exp, err := detect.NewSpanExporter(context.TODO()); err != nil {
742757
return nil, err
758+
} else if !detect.IsNoneSpanExporter(exp) {
759+
tc = append(tc, exp)
743760
}
744761

745762
var traceSocket string
746-
if tc != nil {
763+
if len(tc) > 0 {
747764
traceSocket = cfg.OTEL.SocketPath
748765
if err := runTraceController(traceSocket, tc); err != nil {
749766
return nil, err
@@ -942,9 +959,45 @@ type traceCollector struct {
942959
}
943960

944961
func (t *traceCollector) Export(ctx context.Context, req *tracev1.ExportTraceServiceRequest) (*tracev1.ExportTraceServiceResponse, error) {
945-
err := t.exporter.ExportSpans(ctx, transform.Spans(req.GetResourceSpans()))
946-
if err != nil {
962+
if err := t.exporter.ExportSpans(ctx, transform.Spans(req.GetResourceSpans())); err != nil {
947963
return nil, err
948964
}
949965
return &tracev1.ExportTraceServiceResponse{}, nil
950966
}
967+
968+
func newTracerProvider(ctx context.Context) (*sdktrace.TracerProvider, error) {
969+
opts := []sdktrace.TracerProviderOption{
970+
sdktrace.WithResource(detect.Resource()),
971+
sdktrace.WithSyncer(detect.Recorder),
972+
}
973+
974+
if exp, err := detect.NewSpanExporter(ctx); err != nil {
975+
return nil, err
976+
} else if !detect.IsNoneSpanExporter(exp) {
977+
opts = append(opts, sdktrace.WithBatcher(exp))
978+
}
979+
return sdktrace.NewTracerProvider(opts...), nil
980+
}
981+
982+
func newMeterProvider(ctx context.Context) (*sdkmetric.MeterProvider, error) {
983+
opts := []sdkmetric.Option{
984+
sdkmetric.WithResource(detect.Resource()),
985+
}
986+
987+
if r, err := prometheus.New(); err != nil {
988+
// Log the error but do not fail if we could not configure the prometheus metrics.
989+
bklog.G(context.Background()).
990+
WithError(err).
991+
Error("failed prometheus metrics configuration")
992+
} else {
993+
opts = append(opts, sdkmetric.WithReader(r))
994+
}
995+
996+
if exp, err := detect.NewMetricExporter(ctx); err != nil {
997+
return nil, err
998+
} else if !detect.IsNoneMetricExporter(exp) {
999+
r := sdkmetric.NewPeriodicReader(exp)
1000+
opts = append(opts, sdkmetric.WithReader(r))
1001+
}
1002+
return sdkmetric.NewMeterProvider(opts...), nil
1003+
}

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ require (
8585
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.21.0
8686
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.21.0
8787
go.opentelemetry.io/otel/exporters/prometheus v0.42.0
88-
go.opentelemetry.io/otel/metric v1.21.0
8988
go.opentelemetry.io/otel/sdk v1.21.0
9089
go.opentelemetry.io/otel/sdk/metric v1.21.0
9190
go.opentelemetry.io/otel/trace v1.21.0
@@ -165,6 +164,7 @@ require (
165164
github.com/vishvananda/netns v0.0.4 // indirect
166165
go.opencensus.io v0.24.0 // indirect
167166
go.opentelemetry.io/otel/exporters/otlp/otlpmetric v0.42.0 // indirect
167+
go.opentelemetry.io/otel/metric v1.21.0 // indirect
168168
golang.org/x/text v0.14.0 // indirect
169169
golang.org/x/tools v0.14.0 // indirect
170170
google.golang.org/genproto v0.0.0-20231016165738-49dd2c1f3d0b // indirect

util/tracing/detect/delegated/delegated.go renamed to util/tracing/delegated/delegated.go

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,24 @@ import (
44
"context"
55
"sync"
66

7-
"github.com/moby/buildkit/util/tracing/detect"
7+
"github.com/moby/buildkit/client"
88
sdktrace "go.opentelemetry.io/otel/sdk/trace"
99
)
1010

1111
const maxBuffer = 256
1212

13-
var exp = &Exporter{}
14-
15-
func init() {
16-
detect.Register("delegated", detect.TraceExporterDetector(func() (sdktrace.SpanExporter, error) {
17-
return exp, nil
18-
}), 100)
19-
}
13+
var DefaultExporter = &Exporter{}
2014

2115
type Exporter struct {
2216
mu sync.Mutex
2317
exporters []sdktrace.SpanExporter
2418
buffer []sdktrace.ReadOnlySpan
2519
}
2620

27-
var _ sdktrace.SpanExporter = &Exporter{}
21+
var (
22+
_ sdktrace.SpanExporter = (*Exporter)(nil)
23+
_ client.TracerDelegate = (*Exporter)(nil)
24+
)
2825

2926
func (e *Exporter) ExportSpans(ctx context.Context, ss []sdktrace.ReadOnlySpan) error {
3027
e.mu.Lock()

‎util/tracing/detect/delegated/delegated_test.go‎

Lines changed: 0 additions & 18 deletions
This file was deleted.

0 commit comments

Comments
 (0)