fix exec span attrs (#316) (#318)

During previous refactors of exec for other things, the span creation got moved to the top of the function, and then later code would manipulate config.Attributes, which no longer got picked up.

Regression test added for #316.

Since this functionality was broken for a bit, the attributes are being renamed along the way to match the OTel semantic specs.

Detailed commitlog follows:

* fix exec span attrs

During previous refactors of exec for other things, the span creation
got moved to the top of the function, and then later code would
manipulate config.Attributes, which no longer got picked up.

Regression test added for #316.

Since this functionality was broken for a bit, the attributes are being
renamed along the way to match the OTel semantic specs.

Attributes are built up manually in protobuf structs because the helpers
don't support array values. This could move to a helper later but seems
fine as-is.

* remove duplicate types

* add regular expression capability for SpanData

* test new attributes, fix bugs

Adds a new RE test for generated attributes. Refactors exec.go to fix
some bugs discovered and add more attributes. Since the protobuf
expression of attributes is so verbose, it's now in its own funcs at the
bottom to keep the doExec() function readable.

* remove extra linefeed

* add CHANGELOG notes for bugfix

CHANGELOG.md

@@ -1,7 +1,17 @@
 ## [0.4.5] - 2024-01-01
 
-Cleaning up warnings, dead code, and updating dependencies. No user-visible
-changes.
+Fix exec attributes, cleanups, and dependency updates.
+
+`otel-cli exec` attributes were broken for the last few releases so @tobert
+felt it was ok to rename them to match the OTel semantic conventions
+now.
+
+### Changed
+
+- using latest deps for grpc, protobuf, and otel
+- exec attributes will now go out with the span
+- exec now sends process.command and process.command_args attributes
+- main_test.go now supports regular expression tests on span data
 
 ## [0.4.4] - 2024-03-11
 

README.md

@@ -41,7 +41,6 @@ brew tap equinix-labs/otel-cli
 brew install otel-cli
 ```
 
-
 Alternatively, clone the repo and build it locally:
 
 ```shell

data_for_test.go

@@ -548,6 +548,24 @@ var suites = []FixtureSuite{
 				Config:        otelcli.DefaultConfig().WithEndpoint("grpc://{{endpoint}}"),
 			},
 		},
+		{
+			Name: "#316 ensure process command and args are sent as attributes",
+			Config: FixtureConfig{
+				CliArgs: []string{"exec",
+					"--endpoint", "{{endpoint}}",
+					"--verbose", "--fail",
+					"--attrs", "zy=ab", // ensure CLI args still propagate
+					"--", "/bin/echo", "a", "z",
+				},
+			},
+			Expect: Results{
+				SpanCount: 1,
+				CliOutput: "a z\n",
+				SpanData: map[string]string{
+					"attributes": "/^process.command=/bin/echo,process.command_args=/bin/echo,a,z,process.owner=\\w+,process.parent_pid=\\d+,process.pid=\\d+,zy=ab/",
+				},
+			},
+		},
 	},
 	// otel-cli span with no OTLP config should do and print nothing
 	{

main_test.go

@@ -311,8 +311,8 @@ func checkSpanData(t *testing.T, fixture Fixture, results Results) {
 		}
 
 		if re, ok := spanRegexChecks[what]; ok {
-			// * means if the above RE returns cleanly then pass the test
 			if wantVal == "*" {
+				// * means if the above RE returns cleanly then pass the test
 				if re.MatchString(gotVal) {
 					delete(gotSpan, what)
 					delete(wantSpan, what)
@@ -323,8 +323,21 @@ func checkSpanData(t *testing.T, fixture Fixture, results Results) {
 		}
 	}
 
-	// do a diff on a generated map that sets values to * when the * check succeeded
 	injectMapVars(fixture.Endpoint, wantSpan, fixture.TlsData)
+
+	// a regular expression can be put in e.g. /^foo$/ to get evaluated as RE
+	for key, wantVal := range wantSpan {
+		if strings.HasPrefix(wantVal, "/") && strings.HasSuffix(wantVal, "/") {
+			re := regexp.MustCompile(wantVal[1 : len(wantVal)-1]) // slice strips the /'s off
+			if !re.MatchString(gotSpan[key]) {
+				t.Errorf("regular expression %q did not match on %q", wantVal, gotSpan[key])
+			}
+			delete(gotSpan, key) // delete from both maps so cmp.Diff ignores them
+			delete(wantSpan, key)
+		}
+	}
+
+	// do a diff on a generated map that sets values to * when the * check succeeded
 	if diff := cmp.Diff(wantSpan, gotSpan); diff != "" {
 		t.Errorf("[%s] otel span info did not match fixture (-want +got):\n%s", fixture.Name, diff)
 	}

otelcli/exec.go

@@ -1,19 +1,19 @@
 package otelcli
 
 import (
-	"bytes"
 	"context"
-	"encoding/csv"
 	"fmt"
 	"os"
 	"os/exec"
 	"os/signal"
+	"os/user"
 	"strings"
 	"time"
 
 	"github.com/equinix-labs/otel-cli/otlpclient"
 	"github.com/equinix-labs/otel-cli/w3c/traceparent"
 	"github.com/spf13/cobra"
+	commonpb "go.opentelemetry.io/proto/otlp/common/v1"
 	tracev1 "go.opentelemetry.io/proto/otlp/trace/v1"
 )
 
@@ -62,10 +62,7 @@ func doExec(cmd *cobra.Command, args []string) {
 	ctx := cmd.Context()
 	config := getConfig(ctx)
 	span := config.NewProtobufSpan()
-
-	// put the command in the attributes, before creating the span so it gets picked up
-	config.Attributes["command"] = args[0]
-	config.Attributes["arguments"] = ""
+	processAttrs := processArgAttrs(args) // might be overwritten in process setup
 
 	// no deadline if there is no command timeout set
 	cancelCtxDeadline := func() {}
@@ -95,7 +92,6 @@ func doExec(cmd *cobra.Command, args []string) {
 
 	var child *exec.Cmd
 	if len(args) > 1 {
-		buf := bytes.NewBuffer([]byte{})
 		tpArgs := make([]string, len(args)-1)
 
 		if config.ExecTpDisableInject {
@@ -105,11 +101,10 @@ func doExec(cmd *cobra.Command, args []string) {
 			for i, arg := range args[1:] {
 				tpArgs[i] = strings.Replace(arg, "{{traceparent}}", tp.Encode(), -1)
 			}
-		}
 
-		// CSV-join the arguments to send as an attribute
-		csv.NewWriter(buf).WriteAll([][]string{tpArgs})
-		config.Attributes["arguments"] = buf.String()
+			// overwrite process args attributes with the injected values
+			processAttrs = processArgAttrs(append([]string{args[0]}, tpArgs...))
+		}
 
 		child = exec.CommandContext(cmdCtx, args[0], tpArgs...)
 	} else {
@@ -149,6 +144,11 @@ func doExec(cmd *cobra.Command, args []string) {
 	}
 	span.EndTimeUnixNano = uint64(time.Now().UnixNano())
 
+	// append process attributes
+	span.Attributes = append(span.Attributes, processAttrs...)
+	pidAttrs := processPidAttrs(config, int64(child.Process.Pid), int64(os.Getpid()))
+	span.Attributes = append(span.Attributes, pidAttrs...)
+
 	cancelCtxDeadline()
 	close(signals)
 	<-signalsDone
@@ -173,3 +173,63 @@ func doExec(cmd *cobra.Command, args []string) {
 
 	config.PropagateTraceparent(span, os.Stdout)
 }
+
+// processArgAttrs turns the provided args list into OTel attributes
+// that can be appended to a protobuf span's span.Attributes.
+// https://opentelemetry.io/docs/specs/semconv/attributes-registry/process/
+func processArgAttrs(args []string) []*commonpb.KeyValue {
+	// convert args to an OpenTelemetry string list
+	avlist := make([]*commonpb.AnyValue, len(args))
+	for i, v := range args {
+		sv := commonpb.AnyValue_StringValue{StringValue: v}
+		av := commonpb.AnyValue{Value: &sv}
+		avlist[i] = &av
+	}
+
+	return []*commonpb.KeyValue{
+		{
+			Key: "process.command",
+			Value: &commonpb.AnyValue{
+				Value: &commonpb.AnyValue_StringValue{StringValue: args[0]},
+			},
+		},
+		{
+			Key: "process.command_args",
+			Value: &commonpb.AnyValue{
+				Value: &commonpb.AnyValue_ArrayValue{
+					ArrayValue: &commonpb.ArrayValue{
+						Values: avlist,
+					},
+				},
+			},
+		},
+	}
+}
+
+// processPidAttrs returns process.{owner,pid,parent_pid} attributes ready
+// to append to a protobuf span's span.Attributes.
+func processPidAttrs(config Config, ppid, pid int64) []*commonpb.KeyValue {
+	user, err := user.Current()
+	config.SoftLogIfErr(err)
+
+	return []*commonpb.KeyValue{
+		{
+			Key: "process.owner",
+			Value: &commonpb.AnyValue{
+				Value: &commonpb.AnyValue_StringValue{StringValue: user.Username},
+			},
+		},
+		{
+			Key: "process.pid",
+			Value: &commonpb.AnyValue{
+				Value: &commonpb.AnyValue_IntValue{IntValue: pid},
+			},
+		},
+		{
+			Key: "process.parent_pid",
+			Value: &commonpb.AnyValue{
+				Value: &commonpb.AnyValue_IntValue{IntValue: ppid},
+			},
+		},
+	}
+}

otlpclient/protobuf_span.go

@@ -11,6 +11,7 @@ import (
 	"encoding/hex"
 	"sort"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/equinix-labs/otel-cli/w3c/traceparent"
@@ -189,7 +190,7 @@ func StringMapAttrsToProtobuf(attributes map[string]string) []*commonpb.KeyValue
 func SpanAttributesToStringMap(span *tracepb.Span) map[string]string {
 	out := make(map[string]string)
 	for _, attr := range span.Attributes {
-		out[attr.Key] = AttrValueToString(attr)
+		out[attr.Key] = AnyValueToString(attr.GetValue())
 	}
 	return out
 }
@@ -203,22 +204,27 @@ func ResourceAttributesToStringMap(rss *tracepb.ResourceSpans) map[string]string
 
 	out := make(map[string]string)
 	for _, attr := range rss.Resource.Attributes {
-		out[attr.Key] = AttrValueToString(attr)
+		out[attr.Key] = AnyValueToString(attr.GetValue())
 	}
 	return out
 }
 
-// AttrValueToString coverts a commonpb.KeyValue attribute to a string.
-// Only used by tests for now.
-func AttrValueToString(attr *commonpb.KeyValue) string {
-	v := attr.GetValue()
-	v.GetIntValue()
+// AnyValueToString coverts a commonpb.KeyValue attribute to a string.
+func AnyValueToString(v *commonpb.AnyValue) string {
 	if _, ok := v.Value.(*commonpb.AnyValue_StringValue); ok {
 		return v.GetStringValue()
 	} else if _, ok := v.Value.(*commonpb.AnyValue_IntValue); ok {
 		return strconv.FormatInt(v.GetIntValue(), 10)
 	} else if _, ok := v.Value.(*commonpb.AnyValue_DoubleValue); ok {
 		return strconv.FormatFloat(v.GetDoubleValue(), byte('f'), -1, 64)
+	} else if _, ok := v.Value.(*commonpb.AnyValue_ArrayValue); ok {
+		values := v.GetArrayValue().GetValues()
+		strValues := make([]string, len(values))
+		for i, v := range values {
+			// recursively convert to string
+			strValues[i] = AnyValueToString(v)
+		}
+		return strings.Join(strValues, ",")
 	}
 
 	return ""