Skip to content

Commit

Permalink
separate otlpclient code from CLI code (#223)
Browse files Browse the repository at this point in the history
The most important change in this PR is that the config global is now gone.
Along the way a bunch of sites where the global was still accessed got fixed.
In a few places, config is passed around more than snecessary but the PR had
to stop somewhere.

The code that relies on Cobra isn't as wound up with the OTLP client code
now. It's also using relatively recent features in Cobra that allow for passing
context.Context through to command functions, so now config can be plumbed
without package globals. Since the cobra.Command declarations have moved
to functions, they are no longer global, another win.

There are no new features in this PR. No major changes to the tests except to
 fix 2 gaps that were hiding 2 bugs, that are now fixed.

Squashed commitlog follows:

* move OTLP client files to otlpclient directory

* fix testing gap: test was underspecified

It passed before this refactor because there were bugs in the
traceparent file and localhost detection code. Fortunately localhost
detection failed closed, so no harm done, mostly.

Bug fixes will come in following commits.

* move helpers code to more specific files

* rework code so otlp client and cmd are separate

all tests pass now, no new functionality

A bunch of otelcli funcs were made public. The interface isn't meant to
protect anyone from themselves.

Also refactored Cobra code and setup funcs to pass ctx and config around
instead of relying on globals so the worst are gone.

Some files in otlpclient might make more sense in otelcli, but this is
already more than enough refactoring for one PR.

* update package names in tests

* remove vestiges of otelcmd

otelcmd was a bad idea in progress from another branch, somehow snuck
through

* cleanups

* remove unused variable
  • Loading branch information
tobert authored Jun 20, 2023
1 parent 2e507f5 commit f2ffe9c
Show file tree
Hide file tree
Showing 32 changed files with 1,052 additions and 927 deletions.
136 changes: 69 additions & 67 deletions data_for_test.go

Large diffs are not rendered by default.

9 changes: 5 additions & 4 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@ import (
"os"

"github.com/equinix-labs/otel-cli/otelcli"
"github.com/equinix-labs/otel-cli/otlpclient"
)

// these will be set by goreleaser & ldflags at build time
var (
version = ""
commit = ""
date = ""
version = ""
commit = ""
date = ""
)

func main() {
otelcli.Execute(otelcli.FormatVersion(version, commit, date))
os.Exit(otelcli.GetExitCode())
os.Exit(otlpclient.GetExitCode())
}
4 changes: 2 additions & 2 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
"testing"
"time"

"github.com/equinix-labs/otel-cli/otelcli"
"github.com/equinix-labs/otel-cli/otlpclient"
"github.com/equinix-labs/otel-cli/otlpserver"
"github.com/google/go-cmp/cmp"
tracepb "go.opentelemetry.io/proto/otlp/trace/v1"
Expand Down Expand Up @@ -266,7 +266,7 @@ func checkStatusData(t *testing.T, fixture Fixture, results Results) {
// fixture data.
func checkSpanData(t *testing.T, fixture Fixture, results Results) {
// check the expected span data against what was received by the OTLP server
gotSpan := otelcli.SpanToStringMap(results.Span, results.ResourceSpans)
gotSpan := otlpclient.SpanToStringMap(results.Span, results.ResourceSpans)
injectMapVars(fixture.Endpoint, gotSpan, fixture.TlsData)
wantSpan := map[string]string{} // to be passed to cmp.Diff

Expand Down
67 changes: 34 additions & 33 deletions otelcli/completion.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import (
"log"
"os"

"github.com/equinix-labs/otel-cli/otlpclient"
"github.com/spf13/cobra"
)

var completionCmd = &cobra.Command{
Use: "completion [bash|zsh|fish|powershell]",
Short: "Generate completion script",
Long: `To load completions:
func completionCmd(config *otlpclient.Config) *cobra.Command {
cmd := cobra.Command{
Use: "completion [bash|zsh|fish|powershell]",
Short: "Generate completion script",
Long: `To load completions:
Bash:
Expand Down Expand Up @@ -49,35 +51,34 @@ PowerShell:
PS> otel-cli completion powershell > otel-cli.ps1
# and source this file from your PowerShell profile.
`,
DisableFlagsInUseLine: true,
ValidArgs: []string{"bash", "zsh", "fish", "powershell"},
Args: cobra.MatchAll(cobra.ExactArgs(1)),
Run: func(cmd *cobra.Command, args []string) {
switch args[0] {
case "bash":
err := cmd.Root().GenBashCompletion(os.Stdout)
if err != nil {
log.Fatalf("failed to write completion to stdout")
DisableFlagsInUseLine: true,
ValidArgs: []string{"bash", "zsh", "fish", "powershell"},
Args: cobra.MatchAll(cobra.ExactArgs(1)),
Run: func(cmd *cobra.Command, args []string) {
switch args[0] {
case "bash":
err := cmd.Root().GenBashCompletion(os.Stdout)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
case "zsh":
err := cmd.Root().GenZshCompletion(os.Stdout)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
case "fish":
err := cmd.Root().GenFishCompletion(os.Stdout, true)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
case "powershell":
err := cmd.Root().GenPowerShellCompletionWithDesc(os.Stdout)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
}
case "zsh":
err := cmd.Root().GenZshCompletion(os.Stdout)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
case "fish":
err := cmd.Root().GenFishCompletion(os.Stdout, true)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
case "powershell":
err := cmd.Root().GenPowerShellCompletionWithDesc(os.Stdout)
if err != nil {
log.Fatalf("failed to write completion to stdout")
}
}
},
}
},
}

func init() {
rootCmd.AddCommand(completionCmd)
return &cmd
}
56 changes: 29 additions & 27 deletions otelcli/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import (
"strings"
"time"

"github.com/equinix-labs/otel-cli/otlpclient"
"github.com/spf13/cobra"
)

// execCmd represents the span command
var execCmd = &cobra.Command{
Use: "exec",
Short: "execute the command provided",
Long: `execute the command provided after the subcommand inside a span, measuring
// execCmd sets up the `otel-cli exec` command
func execCmd(config *otlpclient.Config) *cobra.Command {
cmd := cobra.Command{
Use: "exec",
Short: "execute the command provided",
Long: `execute the command provided after the subcommand inside a span, measuring
and reporting how long it took to run. The wrapping span's w3c traceparent is automatically
passed to the child process's environment as TRACEPARENT.
Expand All @@ -28,20 +30,22 @@ otel-cli exec -s "outer span" 'otel-cli exec -s "inner span" sleep 1'
WARNING: this does not clean or validate the command at all before passing it
to sh -c and should not be passed any untrusted input`,
Run: doExec,
Args: cobra.MinimumNArgs(1),
}
Run: doExec,
Args: cobra.MinimumNArgs(1),
}

addCommonParams(&cmd, config)
addSpanParams(&cmd, config)
addAttrParams(&cmd, config)
addClientParams(&cmd, config)

func init() {
rootCmd.AddCommand(execCmd)
addCommonParams(execCmd)
addSpanParams(execCmd)
addAttrParams(execCmd)
addClientParams(execCmd)
return &cmd
}

func doExec(cmd *cobra.Command, args []string) {
ctx, client := StartClient(config)
ctx := cmd.Context()
config := getConfig(ctx)
ctx, client := otlpclient.StartClient(ctx, config)

// put the command in the attributes, before creating the span so it gets picked up
config.Attributes["command"] = args[0]
Expand Down Expand Up @@ -69,40 +73,38 @@ func doExec(cmd *cobra.Command, args []string) {
child.Env = []string{}

// grab everything BUT the TRACEPARENT envvar
// read the cached env from config, because os.Environ() has to be modified
// to work around OTel libs reading envvars directly
for _, env := range config.envBackup {
for _, env := range os.Environ() {
if !strings.HasPrefix(env, "TRACEPARENT=") {
child.Env = append(child.Env, env)
}
}

span := NewProtobufSpanWithConfig(config)
span := otlpclient.NewProtobufSpanWithConfig(config)

// set the traceparent to the current span to be available to the child process
if config.IsRecording() {
tp := traceparentFromSpan(span)
tp := otlpclient.TraceparentFromSpan(span)
child.Env = append(child.Env, fmt.Sprintf("TRACEPARENT=%s", tp.Encode()))
// when not recording, and a traceparent is available, pass it through
} else if !config.TraceparentIgnoreEnv {
tp := loadTraceparent(config.TraceparentCarrierFile)
if tp.initialized {
tp := otlpclient.LoadTraceparent(config)
if tp.Initialized {
child.Env = append(child.Env, fmt.Sprintf("TRACEPARENT=%s", tp.Encode()))
}
}

if err := child.Run(); err != nil {
softFail("command failed: %s", err)
config.SoftFail("command failed: %s", err)
}
span.EndTimeUnixNano = uint64(time.Now().UnixNano())

err := SendSpan(ctx, client, config, span)
err := otlpclient.SendSpan(ctx, client, config, span)
if err != nil {
softFail("unable to send span: %s", err)
config.SoftFail("unable to send span: %s", err)
}

// set the global exit code so main() can grab it and os.Exit() properly
diagnostics.ExecExitCode = child.ProcessState.ExitCode()
otlpclient.Diag.ExecExitCode = child.ProcessState.ExitCode()

propagateTraceparent(span, os.Stdout)
otlpclient.PropagateTraceparent(config, span, os.Stdout)
}
Loading

0 comments on commit f2ffe9c

Please sign in to comment.