-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Add opentelemetry support #2152
Changes from 2 commits
292f1fa
8f50bae
cfb73e5
9717e62
c676959
b10f259
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,9 +8,9 @@ import ( | |
"time" | ||
|
||
"github.com/moby/buildkit/client" | ||
opentracing "github.com/opentracing/opentracing-go" | ||
"github.com/pkg/errors" | ||
"github.com/urfave/cli" | ||
"go.opentelemetry.io/otel/trace" | ||
) | ||
|
||
// ResolveClient resolves a client from CLI args | ||
|
@@ -64,7 +64,7 @@ func ResolveClient(c *cli.Context) (*client.Client, error) { | |
|
||
ctx := CommandContext(c) | ||
|
||
if span := opentracing.SpanFromContext(ctx); span != nil { | ||
if span := trace.SpanFromContext(ctx); span != nil { | ||
opts = append(opts, client.WithTracer(span.Tracer())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is very surprising to me. Having access to the current tracer to create child spans inside the caller's span has been a key feature to me, as it was in opentracing to add flexible instrumentation to the packages. With this removed, I don't see a logical way how tracing would be implemented in a project like BuildKit. I do not want to use global tracer. It is a code smell and makes it so that the packages make assumptions of how tracing is organized in the final binary. If project depending on Buildkit wants to set up a global tracer, everything will still work. BuildKit's packages should be reusable on their own; in many cases, different components of it are included in other tools. The packages do not make assumptions that they only work as singletons or that you can't have two instances of an object using different tracers. I also don't want every constructor/pkg to contain code on how to initialize the tracer and carry that information. That would be a big maintenance cost with no additional value. Sometimes in BuildKit a context goes through 10+ layers of components. Letting functions to ignore tracing if none is configured and add extra contextualized child spans if the caller code set up a tracer/parent-span seems a very logical way to manage this. Even if a project is very complicated, with different components and tracing points, the root context usually starts from a very specific location(eg. gprc client). I looked at the issues open-telemetry/opentelemetry-go#1892 open-telemetry/opentelemetry-go#1887 and to me, if this behavior is not expected, it is an issue with the delegation of the global tracer. I saw some delegation in the global package and thought this was what it was for already. Above else, I think it shows how bad pattern using a global tracer is for synchronizing individual components. Why would you even want to take parent span from context but use a different tracer. Doesn't it automatically mean that your data is messed up and you can't get a proper report back? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The With the It seems that what you're looking for is the ability to extract a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean a different implementation of To have reusable packages, I need a way for my library to just define the tracing points. The library should not care how the actual tracing infrastructure is configured, if two instances of the same object use the same tracer or not etc. This is all up to the caller. Also, every constructor/function should not take and pass tracer objects around. They can of course choose to optionally take them if they want to provide override points, but the default path should just work. With this change, you are not providing a way to achieve that(without me defining my own wrapper that I can continue to pass with context, meaning packages will be incompatible with non-buildkit callers).
That might technically work, but if it means a traceid per spanid it makes no logical sense. I just need a way to pass tracing context through my components. You do it half-way by providing a spancontext but force me to use a global variable for the other half of what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
An example of a package that I claim is quite useless atm. https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http#NewTransport . This is supposed to be a super reusable package, for different services, implementations of roundtrip etc. It takes the Tracer(Provider) on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you available tomorrow for a conversation with @MrAlias and I? I fear that we may be talking past each other or not operating from consistent assumptions and understandings and it may be faster to resolve that with synchronous communication. It is very important to me to understand your concerns and whether addressing them may require changes to the public API we are looking to stabilize in the near term. |
||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,42 +2,25 @@ package common | |
|
||
import ( | ||
"context" | ||
"io" | ||
"os" | ||
"strings" | ||
|
||
"github.com/moby/buildkit/util/appcontext" | ||
opentracing "github.com/opentracing/opentracing-go" | ||
"github.com/opentracing/opentracing-go/ext" | ||
"github.com/opentracing/opentracing-go/log" | ||
jaeger "github.com/uber/jaeger-client-go" | ||
"github.com/moby/buildkit/util/tracing/detect" | ||
"github.com/urfave/cli" | ||
"go.opentelemetry.io/otel/attribute" | ||
"go.opentelemetry.io/otel/codes" | ||
"go.opentelemetry.io/otel/trace" | ||
) | ||
|
||
func getTracer() (opentracing.Tracer, io.Closer) { | ||
if traceAddr := os.Getenv("JAEGER_TRACE"); traceAddr != "" { | ||
tr, err := jaeger.NewUDPTransport(traceAddr, 0) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
// metricsFactory := prometheus.New() | ||
return jaeger.NewTracer( | ||
"buildctl", | ||
jaeger.NewConstSampler(true), | ||
jaeger.NewRemoteReporter(tr), | ||
) | ||
} | ||
|
||
return opentracing.NoopTracer{}, &nopCloser{} | ||
} | ||
|
||
func AttachAppContext(app *cli.App) { | ||
func AttachAppContext(app *cli.App) error { | ||
ctx := appcontext.Context() | ||
|
||
tracer, closer := getTracer() | ||
tracer, err := detect.Tracer() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
var span opentracing.Span | ||
var span trace.Span | ||
|
||
for i, cmd := range app.Commands { | ||
func(before cli.BeforeFunc) { | ||
|
@@ -49,10 +32,8 @@ func AttachAppContext(app *cli.App) { | |
} | ||
} | ||
|
||
span = tracer.StartSpan(name) | ||
span.LogFields(log.String("command", strings.Join(os.Args, " "))) | ||
|
||
ctx = opentracing.ContextWithSpan(ctx, span) | ||
ctx, span = tracer.Start(ctx, name) | ||
span.SetAttributes(attribute.KeyValue{Key: "command", Value: attribute.ArrayValue(os.Args)}) | ||
tonistiigi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
clicontext.App.Metadata["context"] = ctx | ||
return nil | ||
|
@@ -62,7 +43,7 @@ func AttachAppContext(app *cli.App) { | |
|
||
app.ExitErrHandler = func(clicontext *cli.Context, err error) { | ||
if span != nil { | ||
ext.Error.Set(span, true) | ||
span.SetStatus(codes.Error, err.Error()) | ||
Comment on lines
46
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. except in here it will be nil if the |
||
} | ||
cli.HandleExitCoder(err) | ||
} | ||
|
@@ -75,20 +56,13 @@ func AttachAppContext(app *cli.App) { | |
} | ||
} | ||
if span != nil { | ||
span.Finish() | ||
span.End() | ||
tonistiigi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return closer.Close() | ||
return detect.Shutdown(context.TODO()) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func CommandContext(c *cli.Context) context.Context { | ||
return c.App.Metadata["context"].(context.Context) | ||
} | ||
|
||
type nopCloser struct { | ||
} | ||
|
||
func (*nopCloser) Close() error { | ||
return nil | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a concrete
Tracer
here, I'd suggest having aTracerProvider
as this is dropping information coming from the instrumentation that should be recorded on spans created by the returnedTracer
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I used
Tracer
was because there was no way to get access to the current traceprovider from the context.