diff --git a/RELEASES.md b/RELEASES.md index cc87103218..49d4447121 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -637,7 +637,7 @@ The deprecated properties in [`config.toml`](./docs/cri/config.md) are shown in |`[plugins."io.containerd.grpc.v1.cri".registry]` | `mirrors` | containerd v1.5 | containerd v2.7 | Use [`config_path`](./docs/hosts.md) | |`[plugins."io.containerd.tracing.processor.v1.otlp"]` | `endpoint`, `protocol`, `insecure` | containerd v1.6.29 | containerd v2.4 ✅ | Use [OTLP environment variables](https://opentelemetry.io/docs/specs/otel/protocol/exporter/), e.g. OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, OTEL_EXPORTER_OTLP_PROTOCOL, OTEL_SDK_DISABLED | |`[plugins."io.containerd.cri.v1.runtime".cni]` | `bin_dir` | containerd v2.1 | containerd v2.4 ✅ | Use `bin_dirs`, which supports a list of directories | -|`[plugins."io.containerd.internal.v1.tracing"]` | `service_name`, `sampling_ratio` | containerd v1.6.29 | containerd v2.4 | Instead use [OTel environment variables](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/), e.g. OTEL_SERVICE_NAME, OTEL_TRACES_SAMPLER* | +|`[plugins."io.containerd.internal.v1.tracing"]` | `service_name`, `sampling_ratio` | containerd v1.6.29 | containerd v2.4 ✅ | Instead use [OTel environment variables](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/), e.g. OTEL_SERVICE_NAME, OTEL_TRACES_SAMPLER* | |`[plugins."io.containerd.cri.v1.runtime"]` | `enable_cdi` | containerd v2.2 | containerd v2.4 ✅ | CDI support will always be enabled | diff --git a/pkg/deprecation/deprecation.go b/pkg/deprecation/deprecation.go index 9921915e13..aa4b1b02af 100644 --- a/pkg/deprecation/deprecation.go +++ b/pkg/deprecation/deprecation.go @@ -27,8 +27,6 @@ const ( CRIRegistryAuths Warning = Prefix + "cri-registry-auths" // CRIRegistryConfigs is a warning for the use of the `configs` property CRIRegistryConfigs Warning = Prefix + "cri-registry-configs" - // TracingServiceConfig is a warning for the use of the `tracing` property - TracingServiceConfig Warning = Prefix + "tracing-service-config" // NRIV010Plugin is a warning for the use of NRI 0.1.0-style plugins NRIV010Plugin Warning = Prefix + "nri-v010-plugin" // NRIPluginInterface is a warning for the use of a deprecated NRI interface. @@ -53,8 +51,6 @@ var messages = map[Warning]string{ CRIRegistryConfigs: "The `configs` property of `[plugins.\"io.containerd.grpc.v1.cri\".registry]` is deprecated since containerd v1.5 and will be removed in containerd v2.7. " + "Use `config_path` instead.", - TracingServiceConfig: "The `tracing` property of `[plugins.\"io.containerd.internal.v1\".tracing]` is deprecated since containerd v1.6 and will be removed in containerd v2.4. " + - "Use OTEL environment variables instead: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/", NRIV010Plugin: "NRI 0.1.0-style plugins are deprecated since containerd 2.2 and should only be used through the v010-adapter plugin.", NRIPluginInterface: "NRI plugin uses a deprecated interface.", CgroupV1: "The support for cgroup v1 is deprecated since containerd v2.2 and will be removed by no later than May 2029. Upgrade the host to use cgroup v2.", diff --git a/pkg/tracing/plugin/otlp.go b/pkg/tracing/plugin/otlp.go index 2a41da4e37..73b01c9c6d 100644 --- a/pkg/tracing/plugin/otlp.go +++ b/pkg/tracing/plugin/otlp.go @@ -24,9 +24,7 @@ import ( "strconv" "time" - "github.com/containerd/containerd/v2/pkg/deprecation" "github.com/containerd/containerd/v2/plugins" - "github.com/containerd/containerd/v2/plugins/services/warning" "github.com/containerd/errdefs" "github.com/containerd/plugin" "github.com/containerd/plugin/registry" @@ -76,16 +74,12 @@ func init() { }, }) registry.Register(&plugin.Registration{ - ID: "tracing", - Type: plugins.InternalPlugin, - Config: &TraceConfig{}, + ID: "tracing", + Type: plugins.InternalPlugin, Requires: []plugin.Type{ plugins.TracingProcessorPlugin, }, InitFn: func(ic *plugin.InitContext) (any, error) { - if err := warnTraceConfig(ic); err != nil { - return nil, err - } if err := checkDisabled(); err != nil { return nil, err } @@ -106,12 +100,6 @@ func init() { }) } -// TraceConfig is the common configuration for open telemetry. -type TraceConfig struct { - ServiceName string `toml:"service_name,omitempty"` - TraceSamplingRatio float64 `toml:"sampling_ratio,omitempty"` -} - func checkDisabled() error { v := os.Getenv(sdkDisabledEnv) if v != "" { @@ -190,30 +178,3 @@ func newTracer(ctx context.Context, procs []trace.SpanProcessor) (io.Closer, err }), nil } - -func warnTraceConfig(ic *plugin.InitContext) error { - if ic.Config == nil { - return nil - } - ctx := ic.Context - cfg := ic.Config.(*TraceConfig) - var warn bool - if cfg.ServiceName != "" { - warn = true - } - if cfg.TraceSamplingRatio != 0 { - warn = true - } - - if !warn { - return nil - } - - wp, err := ic.GetSingle(plugins.WarningPlugin) - if err != nil { - return err - } - ws := wp.(warning.Service) - ws.Emit(ctx, deprecation.TracingServiceConfig) - return nil -} diff --git a/pkg/tracing/plugin/otlp_test.go b/pkg/tracing/plugin/otlp_test.go index fb3e0122d1..06dda261d3 100644 --- a/pkg/tracing/plugin/otlp_test.go +++ b/pkg/tracing/plugin/otlp_test.go @@ -19,66 +19,192 @@ package plugin import ( "context" "errors" + "os" + "os/exec" + "strings" "testing" "github.com/containerd/errdefs" "github.com/containerd/plugin" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" ) func TestNewExporter(t *testing.T) { for _, tc := range []struct { - name string - protocol string - tracesProtocol string - output error + name string + protocol string + traceProtocol string + expectedErr error }{ { - name: "Test http/protobuf protocol, expect no error", - protocol: "http/protobuf", - output: nil, + name: "default protocol", + expectedErr: nil, }, { - name: "Test default protocol, expect no error", - protocol: "", - output: nil, + name: "http/protobuf protocol", + protocol: "http/protobuf", + expectedErr: nil, }, { - name: "Test grpc protocol, expect no error", - protocol: "grpc", - output: nil, + name: "grpc protocol", + protocol: "grpc", + expectedErr: nil, }, { - name: "Test http/json protocol which is not supported, expect not implemented error", - protocol: "http/json", - output: errdefs.ErrNotImplemented, + name: "traces protocol overrides generic protocol", + protocol: "http/protobuf", + traceProtocol: "grpc", + expectedErr: nil, }, { - name: "Test traces protocol takes precedence over protocol", - protocol: "http/json", - tracesProtocol: "http/protobuf", - output: nil, + name: "unsupported protocol http/json", + protocol: "http/json", + expectedErr: errdefs.ErrNotImplemented, }, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(otlpProtocolEnv, tc.protocol) - t.Setenv(otlpTracesProtocolEnv, tc.tracesProtocol) + t.Setenv(otlpTracesProtocolEnv, tc.traceProtocol) - ctx := context.TODO() + ctx := context.Background() exp, err := newExporter(ctx) - if tc.output == nil { - if err != nil { - t.Fatalf("unexpected error: %v", err) + if tc.expectedErr != nil { + if !errors.Is(err, tc.expectedErr) { + t.Fatalf("expected error %v, got %v", tc.expectedErr, err) } - if exp == nil { - t.Fatal("expected exporter to be created, got nil") - } - } else { - if !errors.Is(err, tc.output) { - t.Fatalf("expected error %v, got %v", tc.output, err) + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exp == nil { + t.Fatal("expected exporter, got nil") + } + }) + } +} + +func TestNewTracerServiceName(t *testing.T) { + if os.Getenv("TEST_SERVICE_NAME_HELPER") == "1" { + expectedService := os.Getenv("EXPECTED_SERVICE_NAME") + rec := tracetest.NewSpanRecorder() + closer, err := newTracer(context.Background(), []trace.SpanProcessor{rec}) + if err != nil { + t.Fatalf("unexpected error creating tracer: %v", err) + } + defer closer.Close() + + ctx := context.Background() + tracer := otel.GetTracerProvider().Tracer("test-tracer") + _, span := tracer.Start(ctx, "test-span") + span.End() + + ended := rec.Ended() + if len(ended) != 1 { + t.Fatalf("expected 1 ended span, got %d", len(ended)) + } + + val, ok := ended[0].Resource().Set().Value("service.name") + if !ok { + t.Fatalf("expected service.name attribute on resource, but found none") + } + svc := val.AsString() + if svc != expectedService { + t.Fatalf("expected service name %q, got %q", expectedService, svc) + } + return + } + + for _, tc := range []struct { + name string + envServiceName string + expectedService string + }{ + { + name: "default service name", + envServiceName: "", + expectedService: "containerd", + }, + { + name: "custom service name", + envServiceName: "custom-containerd", + expectedService: "custom-containerd", + }, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=^TestNewTracerServiceName$") + var env []string + for _, e := range os.Environ() { + if !strings.HasPrefix(e, "OTEL_SERVICE_NAME=") && + !strings.HasPrefix(e, "TEST_SERVICE_NAME_HELPER=") && + !strings.HasPrefix(e, "EXPECTED_SERVICE_NAME=") { + env = append(env, e) } } + env = append(env, "TEST_SERVICE_NAME_HELPER=1", "EXPECTED_SERVICE_NAME="+tc.expectedService) + if tc.envServiceName != "" { + env = append(env, "OTEL_SERVICE_NAME="+tc.envServiceName) + } + cmd.Env = env + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("helper process failed: %v\nOutput: %s", err, string(out)) + } + }) + } +} + +func TestNewTracerSampling(t *testing.T) { + for _, tc := range []struct { + name string + sampler string + samplerArg string + wantSampled bool + }{ + { + name: "always_on sampler", + sampler: "always_on", + wantSampled: true, + }, + { + name: "always_off sampler", + sampler: "always_off", + wantSampled: false, + }, + { + name: "traceidratio 1.0", + sampler: "traceidratio", + samplerArg: "1.0", + wantSampled: true, + }, + { + name: "traceidratio 0.0", + sampler: "traceidratio", + samplerArg: "0.0", + wantSampled: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("OTEL_TRACES_SAMPLER", tc.sampler) + t.Setenv("OTEL_TRACES_SAMPLER_ARG", tc.samplerArg) + + rec := tracetest.NewSpanRecorder() + closer, err := newTracer(context.Background(), []trace.SpanProcessor{rec}) + if err != nil { + t.Fatalf("unexpected error creating tracer: %v", err) + } + defer closer.Close() + + ctx := context.Background() + tracer := otel.GetTracerProvider().Tracer("test-tracer") + _, span := tracer.Start(ctx, "test-span") + defer span.End() + + if got := span.SpanContext().IsSampled(); got != tc.wantSampled { + t.Fatalf("expected span sampled=%v, got %v", tc.wantSampled, got) + } }) } } @@ -86,78 +212,65 @@ func TestNewExporter(t *testing.T) { func TestCheckDisabled(t *testing.T) { for _, tc := range []struct { name string + sdkDisabled string endpoint string tracesEndpoint string - sdkDisabled string - output error + expectedErr error }{ { - name: "No endpoint configured, expect ErrSkipPlugin", - output: plugin.ErrSkipPlugin, + name: "endpoints not configured", + expectedErr: plugin.ErrSkipPlugin, }, { - name: "OTEL_EXPORTER_OTLP_ENDPOINT configured, expect no error", - endpoint: "http://localhost:4318", - output: nil, + name: "endpoint configured", + endpoint: "http://localhost:4318", + expectedErr: nil, }, { - name: "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT configured, expect no error", + name: "traces endpoint configured", tracesEndpoint: "http://localhost:4318", - output: nil, + expectedErr: nil, }, { - name: "OTEL_SDK_DISABLED=true with endpoint, expect ErrSkipPlugin", - endpoint: "http://localhost:4318", + name: "sdk disabled true", sdkDisabled: "true", - output: plugin.ErrSkipPlugin, + endpoint: "http://localhost:4318", + expectedErr: plugin.ErrSkipPlugin, }, { - name: "OTEL_SDK_DISABLED=1 with endpoint, expect ErrSkipPlugin", - endpoint: "http://localhost:4318", + name: "sdk disabled 1", sdkDisabled: "1", - output: plugin.ErrSkipPlugin, + endpoint: "http://localhost:4318", + expectedErr: plugin.ErrSkipPlugin, }, { - name: "OTEL_SDK_DISABLED=false with endpoint, expect no error", - endpoint: "http://localhost:4318", + name: "sdk disabled false", sdkDisabled: "false", - output: nil, + endpoint: "http://localhost:4318", + expectedErr: nil, }, { - name: "OTEL_SDK_DISABLED invalid value, expect ErrInvalidArgument", - endpoint: "http://localhost:4318", + name: "sdk disabled invalid", sdkDisabled: "invalid", - output: errdefs.ErrInvalidArgument, + endpoint: "http://localhost:4318", + expectedErr: errdefs.ErrInvalidArgument, }, } { t.Run(tc.name, func(t *testing.T) { + t.Setenv(sdkDisabledEnv, tc.sdkDisabled) t.Setenv(otlpEndpointEnv, tc.endpoint) t.Setenv(otlpTracesEndpointEnv, tc.tracesEndpoint) - t.Setenv(sdkDisabledEnv, tc.sdkDisabled) err := checkDisabled() - if tc.output == nil { - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - } else { - if !errors.Is(err, tc.output) { - t.Fatalf("expected error %v, got %v", tc.output, err) + if tc.expectedErr != nil { + if !errors.Is(err, tc.expectedErr) { + t.Fatalf("expected error %v, got %v", tc.expectedErr, err) } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) } }) } } - -func TestNewTracer(t *testing.T) { - exp := tracetest.NewInMemoryExporter() - proc := trace.NewBatchSpanProcessor(exp) - procs := []trace.SpanProcessor{proc} - - ctx := context.TODO() - tracerCloser, err := newTracer(ctx, procs) - if err != nil { - t.Fatalf("unexpected error creating tracer: %v", err) - } - defer tracerCloser.Close() -}