diff --git a/data_for_test.go b/data_for_test.go index 6b4fb23..abfe8d1 100644 --- a/data_for_test.go +++ b/data_for_test.go @@ -1070,6 +1070,28 @@ var suites = []FixtureSuite{ }, }, }, + // #390: otel-cli accepts a lenient (upper-case) TRACEPARENT but only ever + // emits strict, spec-valid lower-case W3C into the child process env. + { + { + Name: "otel-cli exec normalizes an upper-case TRACEPARENT into the child env", + Config: FixtureConfig{ + CliArgs: []string{ + "exec", "--endpoint", "{{endpoint}}", + "--force-span-id", "023eee2731392b4d", + "--", + "sh", "-c", "echo $TRACEPARENT"}, + Env: map[string]string{ + "TRACEPARENT": "00-E39280F2980AF3A8600AE98C74F2DABF-BBBBBBBBBBBBBBBB-01", + }, + }, + Expect: Results{ + Config: otelcli.DefaultConfig().WithEndpoint("{{endpoint}}"), + CliOutput: "00-e39280f2980af3a8600ae98c74f2dabf-023eee2731392b4d-01\n", + SpanCount: 1, + }, + }, + }, // validate OTEL_EXPORTER_OTLP_PROTOCOL / --protocol { // --protocol diff --git a/go.mod b/go.mod index 910f777..53afaae 100644 --- a/go.mod +++ b/go.mod @@ -6,8 +6,10 @@ require ( github.com/google/go-cmp v0.7.0 github.com/pterm/pterm v0.12.83 github.com/spf13/cobra v1.10.2 + go.opentelemetry.io/contrib/propagators/envcar v0.69.0 go.opentelemetry.io/otel v1.44.0 go.opentelemetry.io/otel/sdk v1.44.0 + go.opentelemetry.io/otel/trace v1.44.0 go.opentelemetry.io/proto/otlp v1.10.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20260622175928-b703f567277d google.golang.org/grpc v1.81.1 @@ -33,7 +35,6 @@ require ( github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/otel/metric v1.44.0 // indirect - go.opentelemetry.io/otel/trace v1.44.0 // indirect golang.org/x/net v0.56.0 // indirect golang.org/x/sys v0.46.0 // indirect golang.org/x/term v0.44.0 // indirect diff --git a/go.sum b/go.sum index 3e7b61e..0517055 100644 --- a/go.sum +++ b/go.sum @@ -61,6 +61,8 @@ github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJu github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= +go.opentelemetry.io/contrib/propagators/envcar v0.69.0 h1:AXf4SXMcQRyMJMgHKzcYA1bvp4PgyMzU6YiUu2xAD5I= +go.opentelemetry.io/contrib/propagators/envcar v0.69.0/go.mod h1:p2SyHfaQ06uU9wSNypRDRQu567RL/KUUUnz+KbMUs7U= go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= go.opentelemetry.io/otel v1.44.0/go.mod h1:BMgjTHL9WPRlRjL2oZCBTL4whCGtXch2H4BhOPIAyYc= go.opentelemetry.io/otel/metric v1.44.0 h1:1w0gILTcHdr3YI+ixLyjemwrVnsMURbTZFrSYCdDdmc= diff --git a/otelcli/config_span.go b/otelcli/config_span.go index b2d9202..750dca0 100644 --- a/otelcli/config_span.go +++ b/otelcli/config_span.go @@ -89,7 +89,7 @@ func (c Config) LoadTraceparent() traceparent.Traceparent { if !c.TraceparentIgnoreEnv { var err error - tp, err = traceparent.LoadFromEnv() + tp, err = envCarrierTraceparent() if err != nil { Diag.Error = err.Error() } diff --git a/otelcli/exec.go b/otelcli/exec.go index 0fc6dba..c8f7945 100644 --- a/otelcli/exec.go +++ b/otelcli/exec.go @@ -78,15 +78,16 @@ func doExec(cmd *cobra.Command, args []string) { childEnv := []string{} // set the traceparent to the current span to be available to the child process + appendChildEnv := func(key, value string) { childEnv = append(childEnv, key+"="+value) } var tp traceparent.Traceparent if config.GetIsRecording() { tp = otlpclient.TraceparentFromProtobufSpan(span, config.GetIsRecording()) - childEnv = append(childEnv, fmt.Sprintf("TRACEPARENT=%s", tp.Encode())) + injectTraceparent(tp, appendChildEnv) // when not recording, and a traceparent is available, pass it through } else if !config.TraceparentIgnoreEnv { - tp := config.LoadTraceparent() - if tp.Initialized { - childEnv = append(childEnv, fmt.Sprintf("TRACEPARENT=%s", tp.Encode())) + passthrough := config.LoadTraceparent() + if passthrough.Initialized { + injectTraceparent(passthrough, appendChildEnv) } } diff --git a/otelcli/propagation.go b/otelcli/propagation.go new file mode 100644 index 0000000..2121975 --- /dev/null +++ b/otelcli/propagation.go @@ -0,0 +1,65 @@ +package otelcli + +import ( + "context" + + "github.com/tobert/otel-cli/w3c/traceparent" + "go.opentelemetry.io/contrib/propagators/envcar" + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/trace" +) + +// propagator is the OpenTelemetry propagator otel-cli uses to emit context into +// child process environments. Routing emission through the standard W3C +// propagator guarantees the output is always spec-valid: this is the strict +// half of otel-cli's Postel's-law stance (accept leniently when parsing, only +// ever emit valid W3C). TRACESTATE and BAGGAGE join via a composite propagator +// in a follow-up. +var propagator propagation.TextMapPropagator = propagation.TraceContext{} + +// envCarrierTraceparent reads a traceparent from the process environment using +// the OpenTelemetry env-carrier key normalization, then parses the value with +// otel-cli's lenient parser. Parsing leniency is deliberate: we accept input +// the strict propagator would reject (e.g. upper-case hex) and canonicalize it. +// An absent carrier yields an uninitialized Traceparent with no error. +func envCarrierTraceparent() (traceparent.Traceparent, error) { + carrier := envcar.Carrier{} + raw := carrier.Get("traceparent") + if raw == "" { + return traceparent.Traceparent{}, nil + } + return traceparent.Parse(raw) +} + +// injectTraceparent writes tp into a child process environment via setEnv, +// routing through the standard W3C propagator so the emitted value is always +// spec-valid. An invalid (e.g. all-zero) span context is silently not emitted +// rather than propagating a context that downstream tooling would reject. +func injectTraceparent(tp traceparent.Traceparent, setEnv func(key, value string)) { + sc := spanContextFromTraceparent(tp) + if !sc.IsValid() { + return + } + ctx := trace.ContextWithSpanContext(context.Background(), sc) + propagator.Inject(ctx, &envcar.Carrier{SetEnvFunc: setEnv}) +} + +// spanContextFromTraceparent converts otel-cli's internal traceparent into the +// SDK trace.SpanContext the propagator operates on. +func spanContextFromTraceparent(tp traceparent.Traceparent) trace.SpanContext { + var traceID trace.TraceID + var spanID trace.SpanID + copy(traceID[:], tp.TraceId) + copy(spanID[:], tp.SpanId) + + flags := trace.TraceFlags(0) + if tp.Sampling { + flags = trace.FlagsSampled + } + + return trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: traceID, + SpanID: spanID, + TraceFlags: flags, + }) +} diff --git a/otelcli/propagation_test.go b/otelcli/propagation_test.go new file mode 100644 index 0000000..f92ded3 --- /dev/null +++ b/otelcli/propagation_test.go @@ -0,0 +1,74 @@ +package otelcli + +import ( + "testing" +) + +// TestEnvCarrierTraceparentGraceful proves Postel's law on the read side: we +// accept input that the strict W3C propagator would reject (upper-case hex) +// and downcase it into our canonical representation. +func TestEnvCarrierTraceparentGraceful(t *testing.T) { + t.Setenv("TRACEPARENT", "00-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA-BBBBBBBBBBBBBBBB-01") + + tp, err := envCarrierTraceparent() + if err != nil { + t.Fatalf("envCarrierTraceparent() returned an unexpected error: %s", err) + } + if !tp.Initialized { + t.Fatal("expected an initialized traceparent from an upper-case TRACEPARENT") + } + if got := tp.TraceIdString(); got != "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" { + t.Errorf("trace id not downcased, got %q", got) + } + if got := tp.SpanIdString(); got != "bbbbbbbbbbbbbbbb" { + t.Errorf("span id not downcased, got %q", got) + } + if !tp.Sampling { + t.Error("expected the sampling flag to be set") + } +} + +// TestEnvCarrierTraceparentEmpty proves an absent carrier yields an +// uninitialized traceparent with no error, matching the historical contract. +func TestEnvCarrierTraceparentEmpty(t *testing.T) { + t.Setenv("TRACEPARENT", "") + + tp, err := envCarrierTraceparent() + if err != nil { + t.Fatalf("envCarrierTraceparent() returned an unexpected error: %s", err) + } + if tp.Initialized { + t.Error("expected an uninitialized traceparent when TRACEPARENT is unset") + } +} + +// TestInjectTraceparentStrict proves the emit side only ever produces +// spec-valid, lower-case W3C, even when the source was parsed from upper-case. +func TestInjectTraceparentStrict(t *testing.T) { + t.Setenv("TRACEPARENT", "00-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA-BBBBBBBBBBBBBBBB-01") + tp, err := envCarrierTraceparent() + if err != nil { + t.Fatalf("setup parse failed: %s", err) + } + + got := map[string]string{} + injectTraceparent(tp, func(k, v string) { got[k] = v }) + + want := "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01" + if got["TRACEPARENT"] != want { + t.Errorf("expected child TRACEPARENT=%q, got %q (full map: %v)", want, got["TRACEPARENT"], got) + } +} + +// TestInjectTraceparentSkipsInvalid proves we never emit a traceparent that +// would fail otel spec validation (e.g. an all-zero context). +func TestInjectTraceparentSkipsInvalid(t *testing.T) { + // --tp-ignore-env yields a zeroed-but-Initialized traceparent, the realistic + // "initialized yet invalid" case we must not propagate. + zeroed := DefaultConfig().WithTraceparentIgnoreEnv(true).LoadTraceparent() + called := false + injectTraceparent(zeroed, func(k, v string) { called = true }) + if called { + t.Error("an all-zero/invalid traceparent must not be emitted into the child env") + } +}