spring cleaning

data_for_test.go

@@ -336,10 +336,19 @@ var suites = []FixtureSuite{
 				SpanCount: 0,
 			},
 		},
+	},
+	// regression tests
+	{
 		{
-			Name: "otel-cli exec sets span start time earlier than end time",
+			// The span end time was missing when #175 merged, which showed up
+			// as 0ms spans. CheckFuncs was added to make this possible. This
+			// test runs sleep for 10ms and checks the duration of the span is
+			// at least 10ms.
+			Name: "#189 otel-cli exec sets span start time earlier than end time",
 			Config: FixtureConfig{
-				CliArgs: []string{"exec", "--endpoint", "{{endpoint}}", "sleep", "0.05"},
+				// note: relies on system sleep command supporting floats
+				// note: 10ms is hardcoded in a few places for this test and commentary
+				CliArgs: []string{"exec", "--endpoint", "{{endpoint}}", "sleep", "0.01"},
 			},
 			Expect: Results{
 				SpanCount: 1,
@@ -348,8 +357,8 @@ var suites = []FixtureSuite{
 			CheckFuncs: []CheckFunc{
 				func(t *testing.T, f Fixture, r Results) {
 					elapsed := r.Span.End.Sub(r.Span.Start)
-					if elapsed.Milliseconds() < 50 {
-						t.Errorf("elapsed test time not long enough. Expected ~50ms, got %d ms", elapsed.Milliseconds())
+					if elapsed.Milliseconds() < 10 {
+						t.Errorf("elapsed test time not long enough. Expected 10ms, got %d ms", elapsed.Milliseconds())
 					}
 				},
 			},

main_test.go

@@ -62,9 +62,6 @@ func TestOtelCli(t *testing.T) {
 			if _, ok := fixture.Config.Env["PATH"]; ok {
 				t.Fatalf("fixture in file %s is not allowed to modify or test envvar PATH", fixture.Name)
 			}
-			/* if fixture.CheckFuncs == nil {
-				fixture.CheckFuncs = []CheckFunc{}
-			} */
 		}
 	}
 
@@ -246,19 +243,6 @@ func checkStatusData(t *testing.T, fixture Fixture, results Results) {
 	}
 }
 
-// spanRegexChecks is a map of field names and regexes for basic presence
-// and validity checking of span data fields with expected set to "*"
-var spanRegexChecks = map[string]*regexp.Regexp{
-	"trace_id":   regexp.MustCompile(`^[0-9a-fA-F]{32}$`),
-	"span_id":    regexp.MustCompile(`^[0-9a-fA-F]{16}$`),
-	"name":       regexp.MustCompile(`^\w+$`),
-	"parent":     regexp.MustCompile(`^[0-9a-fA-F]{32}$`),
-	"kind":       regexp.MustCompile(`^\w+$`), // TODO: can validate more here
-	"start":      regexp.MustCompile(`^\d+$`),
-	"end":        regexp.MustCompile(`^\d+$`),
-	"attributes": regexp.MustCompile(`\w+=.+`), // not great but should validate at least one k=v
-}
-
 // checkSpanData compares the data in spans received from otel-cli against the
 // fixture data.
 func checkSpanData(t *testing.T, fixture Fixture, results Results) {
@@ -277,6 +261,19 @@ func checkSpanData(t *testing.T, fixture Fixture, results Results) {
 		}
 	}
 
+	// spanRegexChecks is a map of field names and regexes for basic presence
+	// and validity checking of span data fields with expected set to "*"
+	spanRegexChecks := map[string]*regexp.Regexp{
+		"trace_id":   regexp.MustCompile(`^[0-9a-fA-F]{32}$`),
+		"span_id":    regexp.MustCompile(`^[0-9a-fA-F]{16}$`),
+		"name":       regexp.MustCompile(`^\w+$`),
+		"parent":     regexp.MustCompile(`^[0-9a-fA-F]{32}$`),
+		"kind":       regexp.MustCompile(`^\w+$`), // TODO: can validate more here
+		"start":      regexp.MustCompile(`^\d+$`),
+		"end":        regexp.MustCompile(`^\d+$`),
+		"attributes": regexp.MustCompile(`\w+=.+`), // not great but should validate at least one k=v
+	}
+
 	// go over all the keys in the received span and check against expected values
 	// in the fixture, and the spanRegexChecks
 	// modifies the gotSpan/wantSpan maps so cmp.Diff can do its thing

otelcli/completion.go

@@ -51,7 +51,7 @@ PowerShell:
 `,
 	DisableFlagsInUseLine: true,
 	ValidArgs:             []string{"bash", "zsh", "fish", "powershell"},
-	Args:                  cobra.ExactValidArgs(1),
+	Args:                  cobra.MatchAll(cobra.ExactArgs(1)),
 	Run: func(cmd *cobra.Command, args []string) {
 		switch args[0] {
 		case "bash":

otelcli/config.go

@@ -11,12 +11,9 @@ import (
 )
 
 // config is the global configuraiton used by all of otel-cli.
-// It is written to by Cobra and Viper.
+// global so that Cobra can access it
 var config Config
 
-// defaults is a Config set to default values used by Cobra
-var defaults = DefaultConfig()
-
 const defaultOtlpEndpoint = "grpc://localhost:4317"
 const spanBgSockfilename = "otel-cli-background.sock"
 
@@ -106,11 +103,11 @@ type Config struct {
 // LoadFile reads the file specified by -c/--config and overwrites the
 // current config values with any found in the file.
 func (c *Config) LoadFile() error {
-	if config.CfgFile == "" {
+	if c.CfgFile == "" {
 		return nil
 	}
 
-	js, err := os.ReadFile(config.CfgFile)
+	js, err := os.ReadFile(c.CfgFile)
 	if err != nil {
 		return errors.Wrapf(err, "failed to read file '%s'", c.CfgFile)
 	}

otelcli/diagnostics.go

@@ -21,6 +21,7 @@ type Diagnostics struct {
 	ParsedTimeoutMs    int64    `json:"parsed_timeout_ms"`
 	OtelError          string   `json:"otel_error"`
 	ExecExitCode       int      `json:"exec_exit_code"`
+	config             Config
 }
 
 // Handle complies with the otel error handler interface to capture errors
@@ -30,7 +31,7 @@ type Diagnostics struct {
 func (Diagnostics) Handle(err error) {
 	diagnostics.OtelError = err.Error() // write to the global
 
-	if config.Fail {
+	if diagnostics.config.Fail {
 		softFail("OpenTelemetry error: %s", err)
 	} else {
 		softLog("OpenTelemetry error: %s", err)

otelcli/exec.go

@@ -96,7 +96,7 @@ func doExec(cmd *cobra.Command, args []string) {
 	}
 	span.EndTimeUnixNano = uint64(time.Now().UnixNano())
 
-	err := SendSpan(context.Background(), span)
+	err := SendSpan(context.Background(), config, span)
 	if err != nil {
 		softFail("unable to send span: %s", err)
 	}

otelcli/helpers.go

@@ -113,7 +113,7 @@ func parseTime(ts, which string) time.Time {
 // parseCliTimeout parses the cliTimeout global string value to a time.Duration.
 // When no duration letter is provided (e.g. ms, s, m, h), seconds are assumed.
 // It logs an error and returns time.Duration(0) if the string is empty or unparseable.
-func parseCliTimeout() time.Duration {
+func parseCliTimeout(config Config) time.Duration {
 	var out time.Duration
 	if config.Timeout == "" {
 		out = time.Duration(0)

otelcli/helpers_test.go

@@ -145,8 +145,8 @@ func TestParseCliTime(t *testing.T) {
 		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
-			config = Config{Timeout: testcase.input}
-			got := parseCliTimeout()
+			config := DefaultConfig().WithTimeout(testcase.input)
+			got := parseCliTimeout(config)
 			if got != testcase.expected {
 				ed := testcase.expected.String()
 				gd := got.String()

otelcli/otlp_client.go

@@ -25,7 +25,7 @@ import (
 )
 
 // SendSpan connects to the OTLP server, sends the span, and disconnects.
-func SendSpan(ctx context.Context, span tracepb.Span) error {
+func SendSpan(ctx context.Context, config Config, span tracepb.Span) error {
 	if !config.IsRecording() {
 		return nil
 	}
@@ -41,9 +41,9 @@ func SendSpan(ctx context.Context, span tracepb.Span) error {
 		(strings.HasPrefix(config.Protocol, "http/") ||
 			strings.HasPrefix(config.Endpoint, "http://") ||
 			strings.HasPrefix(config.Endpoint, "https://")) {
-		client = otlptracehttp.NewClient(httpOptions()...)
+		client = otlptracehttp.NewClient(httpOptions(config)...)
 	} else {
-		client = otlptracegrpc.NewClient(grpcOptions()...)
+		client = otlptracegrpc.NewClient(grpcOptions(config)...)
 	}
 
 	err := client.Start(ctx)
@@ -88,7 +88,7 @@ func SendSpan(ctx context.Context, span tracepb.Span) error {
 
 // tlsConfig evaluates otel-cli configuration and returns a tls.Config
 // that can be used by grpc or https.
-func tlsConfig() *tls.Config {
+func tlsConfig(config Config) *tls.Config {
 	tlsConfig := &tls.Config{}
 
 	if config.TlsNoVerify {
@@ -134,7 +134,7 @@ func tlsConfig() *tls.Config {
 }
 
 // grpcOptions convets config settings to an otlpgrpc.Option list.
-func grpcOptions() []otlptracegrpc.Option {
+func grpcOptions(config Config) []otlptracegrpc.Option {
 	grpcOpts := []otlptracegrpc.Option{}
 
 	// per comment in initTracer(), grpc:// is specific to otel-cli
@@ -151,7 +151,7 @@ func grpcOptions() []otlptracegrpc.Option {
 	}
 
 	// set timeout if the duration is non-zero, otherwise just leave things to the defaults
-	if timeout := parseCliTimeout(); timeout > 0 {
+	if timeout := parseCliTimeout(config); timeout > 0 {
 		grpcOpts = append(grpcOpts, otlptracegrpc.WithTimeout(timeout))
 	}
 
@@ -163,7 +163,7 @@ func grpcOptions() []otlptracegrpc.Option {
 	if config.Insecure || (isLoopbackAddr(config.Endpoint) && !strings.HasPrefix(config.Endpoint, "https")) {
 		grpcOpts = append(grpcOpts, otlptracegrpc.WithInsecure())
 	} else if !isInsecureSchema(config.Endpoint) {
-		grpcOpts = append(grpcOpts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConfig())))
+		grpcOpts = append(grpcOpts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConfig(config))))
 	}
 
 	// support for OTLP headers, e.g. for authenticating to SaaS OTLP endpoints
@@ -184,7 +184,7 @@ func grpcOptions() []otlptracegrpc.Option {
 }
 
 // httpOptions converts config to an otlphttp.Option list.
-func httpOptions() []otlptracehttp.Option {
+func httpOptions(config Config) []otlptracehttp.Option {
 	endpointURL, err := url.Parse(config.Endpoint)
 	if err != nil {
 		softFail("error parsing provided HTTP URI '%s': %s", config.Endpoint, err)
@@ -209,7 +209,7 @@ func httpOptions() []otlptracehttp.Option {
 	httpOpts = append(httpOpts, otlptracehttp.WithURLPath(endpointURL.Path))
 
 	// set timeout if the duration is non-zero, otherwise just leave things to the defaults
-	if timeout := parseCliTimeout(); timeout > 0 {
+	if timeout := parseCliTimeout(config); timeout > 0 {
 		httpOpts = append(httpOpts, otlptracehttp.WithTimeout(timeout))
 	}
 
@@ -222,7 +222,7 @@ func httpOptions() []otlptracehttp.Option {
 	if config.Insecure || (isLoopbackAddr(config.Endpoint) && !strings.HasPrefix(config.Endpoint, "https")) {
 		httpOpts = append(httpOpts, otlptracehttp.WithInsecure())
 	} else if !isInsecureSchema(config.Endpoint) {
-		httpOpts = append(httpOpts, otlptracehttp.WithTLSClientConfig(tlsConfig()))
+		httpOpts = append(httpOpts, otlptracehttp.WithTLSClientConfig(tlsConfig(config)))
 	}
 
 	// support for OTLP headers, e.g. for authenticating to SaaS OTLP endpoints

otelcli/root.go

@@ -42,6 +42,8 @@ func init() {
 
 // addCommonParams adds the --config and --endpoint params to the command.
 func addCommonParams(cmd *cobra.Command) {
+	defaults := DefaultConfig()
+
 	// --config / -c a JSON configuration file
 	cmd.Flags().StringVarP(&config.CfgFile, "config", "c", defaults.CfgFile, "JSON configuration file")
 	// --endpoint an endpoint to send otlp output to
@@ -61,7 +63,9 @@ func addCommonParams(cmd *cobra.Command) {
 // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md
 // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
 func addClientParams(cmd *cobra.Command) {
+	defaults := DefaultConfig()
 	config.Headers = make(map[string]string)
+
 	// OTEL_EXPORTER standard env and variable params
 	cmd.Flags().StringToStringVar(&config.Headers, "otlp-headers", defaults.Headers, "a comma-sparated list of key=value headers to send on OTLP connection")
 	cmd.Flags().BoolVar(&config.Blocking, "otlp-blocking", defaults.Blocking, "block on connecting to the OTLP server before proceeding")
@@ -83,6 +87,7 @@ func addClientParams(cmd *cobra.Command) {
 }
 
 func addSpanParams(cmd *cobra.Command) {
+	defaults := DefaultConfig()
 	// --name / -s
 	cmd.Flags().StringVarP(&config.SpanName, "name", "n", defaults.SpanName, "set the name of the span")
 	// --service / -n
@@ -96,6 +101,7 @@ func addSpanParams(cmd *cobra.Command) {
 }
 
 func addAttrParams(cmd *cobra.Command) {
+	defaults := DefaultConfig()
 	// --attrs key=value,foo=bar
 	config.Attributes = make(map[string]string)
 	cmd.Flags().StringToStringVarP(&config.Attributes, "attrs", "a", defaults.Attributes, "a comma-separated list of key=value attributes")

otelcli/server_json.go

@@ -40,7 +40,7 @@ func doServerJson(cmd *cobra.Command, args []string) {
 	cs := otlpserver.NewGrpcServer(renderJson, stop)
 
 	// stops the grpc server after timeout
-	timeout := parseCliTimeout()
+	timeout := parseCliTimeout(config)
 	if timeout > 0 {
 		go func() {
 			time.Sleep(timeout)

otelcli/span.go

@@ -31,6 +31,8 @@ Example:
 var epochNanoTimeRE *regexp.Regexp
 
 func init() {
+	defaults := DefaultConfig()
+
 	rootCmd.AddCommand(spanCmd)
 	spanCmd.Flags().SortFlags = false
 
@@ -50,6 +52,6 @@ func init() {
 
 func doSpan(cmd *cobra.Command, args []string) {
 	span := NewProtobufSpanWithConfig(config)
-	SendSpan(context.Background(), span)
+	SendSpan(context.Background(), config, span)
 	propagateTraceparent(span, os.Stdout)
 }

otelcli/span_background.go

@@ -38,9 +38,7 @@ timeout, (catchable) signals, or deliberate exit.
 }
 
 func init() {
-	// this used to be a global const but now it's in Config
-	// TODO: does it make sense to make this configurable? 10ms might be too frequent...
-	config.BackgroundParentPollMs = 10
+	defaults := DefaultConfig()
 
 	spanCmd.AddCommand(spanBgCmd)
 	spanBgCmd.Flags().SortFlags = false
@@ -50,6 +48,7 @@ func init() {
 	// at the end to get an easy span
 	spanBgCmd.Flags().StringVar(&config.BackgroundSockdir, "sockdir", defaults.BackgroundSockdir, "a directory where a socket can be placed safely")
 
+	spanBgCmd.Flags().IntVar(&config.BackgroundParentPollMs, "parent-poll", defaults.BackgroundParentPollMs, "number of milliseconds to wait between checking for whether the parent process exited")
 	spanBgCmd.Flags().BoolVar(&config.BackgroundWait, "wait", defaults.BackgroundWait, "wait for background to be fully started and then return")
 	spanBgCmd.Flags().BoolVar(&config.BackgroundSkipParentPidCheck, "skip-pid-check", defaults.BackgroundSkipParentPidCheck, "disable checking parent pid")
 
@@ -121,7 +120,7 @@ func doSpanBackground(cmd *cobra.Command, args []string) {
 
 	// start the timeout goroutine, this is a little late but the server
 	// has to be up for this to make much sense
-	if timeout := parseCliTimeout(); timeout > 0 {
+	if timeout := parseCliTimeout(config); timeout > 0 {
 		go func() {
 			time.Sleep(timeout)
 			rt := time.Since(started)
@@ -134,7 +133,7 @@ func doSpanBackground(cmd *cobra.Command, args []string) {
 	bgs.Run()
 
 	span.EndTimeUnixNano = uint64(time.Now().UnixNano())
-	err := SendSpan(context.Background(), span)
+	err := SendSpan(context.Background(), config, span)
 	if err != nil {
 		softFail("Sending span failed: %s", err)
 	}

otelcli/span_background_server.go

@@ -150,7 +150,7 @@ func (bgs *bgServer) Shutdown() {
 func createBgClient() (*rpc.Client, func()) {
 	sockfile := spanBgSockfile()
 	started := time.Now()
-	timeout := parseCliTimeout()
+	timeout := parseCliTimeout(config)
 
 	// wait for the socket file to show up, polling every 25ms until it does or timeout
 	for {

otelcli/span_end.go

@@ -20,6 +20,7 @@ See: otel-cli span background
 }
 
 func init() {
+	defaults := DefaultConfig()
 	spanCmd.AddCommand(spanEndCmd)
 	spanEndCmd.Flags().BoolVar(&config.Verbose, "verbose", defaults.Verbose, "print errors on failure instead of always being silent")
 	// TODO

otelcli/span_event.go

@@ -27,6 +27,8 @@ See: otel-cli span background
 }
 
 func init() {
+	defaults := DefaultConfig()
+
 	spanCmd.AddCommand(spanEventCmd)
 	spanEventCmd.Flags().SortFlags = false
 

otelcli/status.go

@@ -66,7 +66,7 @@ func doStatus(cmd *cobra.Command, args []string) {
 	}
 
 	// send the span out before printing anything
-	err := SendSpan(context.Background(), span)
+	err := SendSpan(context.Background(), config, span)
 	if err != nil {
 		if config.Fail {
 			log.Fatalf("%s", err)