-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add OTEL support (carry 4556) #4698
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||
| package main | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||
| "net" | ||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||
|
|
@@ -22,6 +23,12 @@ import ( | |||||||||||||||||||||||||
| "github.com/sirupsen/logrus" | ||||||||||||||||||||||||||
| "github.com/spf13/cobra" | ||||||||||||||||||||||||||
| "github.com/spf13/pflag" | ||||||||||||||||||||||||||
| "go.opentelemetry.io/contrib/exporters/autoexport" | ||||||||||||||||||||||||||
| "go.opentelemetry.io/otel" | ||||||||||||||||||||||||||
| "go.opentelemetry.io/otel/propagation" | ||||||||||||||||||||||||||
| "go.opentelemetry.io/otel/sdk/resource" | ||||||||||||||||||||||||||
| sdktrace "go.opentelemetry.io/otel/sdk/trace" | ||||||||||||||||||||||||||
| "go.opentelemetry.io/otel/trace" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func main() { | ||||||||||||||||||||||||||
|
|
@@ -68,7 +75,23 @@ func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { | |||||||||||||||||||||||||
| return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", args[0]) | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| PersistentPreRunE: func(cmd *cobra.Command, args []string) error { | ||||||||||||||||||||||||||
| return isSupported(cmd, dockerCli) | ||||||||||||||||||||||||||
| if err := isSupported(cmd, dockerCli); err != nil { | ||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| name := cmd.Name() | ||||||||||||||||||||||||||
| for p := cmd.Parent(); p != nil && p != cmd.Root(); p = p.Parent() { | ||||||||||||||||||||||||||
| name = p.Name() + " " + name | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ctx, _ := otel.Tracer("").Start(cmd.Context(), name) | ||||||||||||||||||||||||||
|
Comment on lines
+82
to
+87
Member
Author
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. Just discussing with @milas that we don't have a good convention yet for passing the command name; compose currently uses this; ctx, cmdSpan := tracing.Tracer.Start(
ctx,
"cli/"+strings.Join(commandName(cmd), "-"),
)
cmdSpan.SetAttributes(attribute.StringSlice("cli.args", args))
cmdSpan.SetAttributes(attribute.StringSlice("cli.flags", getFlags(cmd.Flags())))that's what Compose is doing so it's slightly different, e.g.
Member
Author
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. We can keep this as-is for now, and look at improving later; I could add the flags and args if we think it's useful to add.
Contributor
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.
Member
Author
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.
OH! That looks great, and from the looks of it that pretty much describes the things we'd want to include (and we wouldn't have to come up with our own conventions, which is a big 💯) @milas @jsternberg You'll probably be interested in this (if you didn't know about these). From the looks of it; most of these we could set using a generic helper (if none exists for it). We MAY have to consider if we should sanitise arguments though (i.e. do we want If we decide to sanitise, perhaps we need an option to opt-out of sanitising (for those that DO want to include all of that in their traces). Let me add a screenshot to save others from having to follow the link, and to add context to my comment;
|
||||||||||||||||||||||||||
| cmd.SetContext(ctx) | ||||||||||||||||||||||||||
| _ = dockerCli.Apply(command.WithBaseContext(ctx)) | ||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| PersistentPostRun: func(cmd *cobra.Command, args []string) { | ||||||||||||||||||||||||||
| // TODO: There doesn't seem to be a way to determine if the command returned an error, so we can set the span status here. | ||||||||||||||||||||||||||
| trace.SpanFromContext(cmd.Context()).End() | ||||||||||||||||||||||||||
|
Comment on lines
+93
to
+94
Contributor
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. You're fighting an uphill trying to use cobra lifecycle hooks for this. See below. |
||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| Version: fmt.Sprintf("%s, build %s", version.Version, version.GitCommit), | ||||||||||||||||||||||||||
| DisableFlagsInUseLine: true, | ||||||||||||||||||||||||||
|
|
@@ -290,6 +313,14 @@ func runDocker(dockerCli *command.DockerCli) error { | |||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if v := os.Getenv("OTEL_SERVICE_NAME"); v == "" { | ||||||||||||||||||||||||||
| _ = os.Setenv("OTEL_SERVICE_NAME", cmd.Root().Name()) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if err := initializeTracing(cmd.Context()); err != nil { | ||||||||||||||||||||||||||
|
Member
Author
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. And passed
Contributor
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. Docs on it are minimal - it appears to be intended for exporter setup, e.g. if it needs to dial a connection up-front or something I guess? [see my other comment though, it's actually unused currently by both the HTTP/gRPC exporters]
Contributor
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. Initialize tracing in |
||||||||||||||||||||||||||
| logrus.WithError(err).Debug("Failed to initialize tracing") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var envs []string | ||||||||||||||||||||||||||
| args, os.Args, envs, err = processAliases(dockerCli, cmd, args, os.Args) | ||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||
|
|
@@ -325,6 +356,22 @@ func runDocker(dockerCli *command.DockerCli) error { | |||||||||||||||||||||||||
| return cmd.Execute() | ||||||||||||||||||||||||||
|
Contributor
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.
Suggested change
It'd be even better to start the span at the top of the function, so the latency before the command is executed gets captured in the traces. |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func initializeTracing(ctx context.Context) error { | ||||||||||||||||||||||||||
|
Member
Author
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. I added a context here |
||||||||||||||||||||||||||
| otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{})) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| exporter, err := autoexport.NewSpanExporter(ctx) | ||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||
| return fmt.Errorf("creating span exporter: %w", err) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| otel.SetTracerProvider(sdktrace.NewTracerProvider( | ||||||||||||||||||||||||||
| sdktrace.WithSpanProcessor(sdktrace.NewBatchSpanProcessor(exporter)), | ||||||||||||||||||||||||||
| sdktrace.WithResource(resource.Default()), | ||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||
|
Comment on lines
+367
to
+370
Contributor
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 tracer provider needs to be shut down before the process exits to ensure the recorded spans are flushed to the exporter. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| type versionDetails interface { | ||||||||||||||||||||||||||
| CurrentVersion() string | ||||||||||||||||||||||||||
| ServerInfo() command.ServerInfo | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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.
When in doubt, create a nested span. It provides more detailed tracing insights than reusing a single top-level span for the entire lifetime of the process.