From 045f29da13d9e752124a5d719c042514d3c02072 Mon Sep 17 00:00:00 2001 From: Manendra Pal Singh Date: Tue, 2 Dec 2025 23:00:28 +0530 Subject: [PATCH] update as per the PR comment --- cmd/adapter/main.go | 4 +- cmd/adapter/metrics_integration_test.go | 5 +- .../implementation/otelsetup/cmd/plugin.go | 10 +- .../otelsetup/cmd/plugin_test.go | 43 ++++----- .../implementation/otelsetup/otelsetup.go | 30 ++++-- .../otelsetup/otelsetup_test.go | 25 +++-- pkg/telemetry/metrics_test.go | 7 +- pkg/telemetry/step_instrumentor_test.go | 14 +-- pkg/telemetry/step_metrics_test.go | 28 +----- pkg/telemetry/telemetry.go | 95 ------------------- pkg/telemetry/test_helper.go | 54 +++++++++++ 11 files changed, 128 insertions(+), 187 deletions(-) create mode 100644 pkg/telemetry/test_helper.go diff --git a/cmd/adapter/main.go b/cmd/adapter/main.go index 0bd4646..dbbaf4a 100644 --- a/cmd/adapter/main.go +++ b/cmd/adapter/main.go @@ -25,7 +25,7 @@ import ( type Config struct { AppName string `yaml:"appName"` Log log.Config `yaml:"log"` - Telemetry *telemetry.Config `yaml:"telemetry"` + Telemetry *otelsetup.Config `yaml:"telemetry"` PluginManager *plugin.ManagerConfig `yaml:"pluginManager"` Modules []module.Config `yaml:"modules"` HTTP httpConfig `yaml:"http"` @@ -95,7 +95,7 @@ func validateConfig(cfg *Config) error { } // initPlugins initializes application-level plugins including telemetry. -func initPlugins(ctx context.Context, mgr *plugin.Manager, telemetryCfg *telemetry.Config) (*telemetry.Provider, error) { +func initPlugins(ctx context.Context, mgr *plugin.Manager, telemetryCfg *otelsetup.Config) (*telemetry.Provider, error) { if telemetryCfg == nil { log.Info(ctx, "Telemetry config not provided; skipping OpenTelemetry setup") return nil, nil diff --git a/cmd/adapter/metrics_integration_test.go b/cmd/adapter/metrics_integration_test.go index b7648c7..16bc6b7 100644 --- a/cmd/adapter/metrics_integration_test.go +++ b/cmd/adapter/metrics_integration_test.go @@ -5,13 +5,14 @@ import ( "net/http/httptest" "testing" - "github.com/beckn-one/beckn-onix/pkg/telemetry" + "github.com/beckn-one/beckn-onix/pkg/plugin/implementation/otelsetup" "github.com/stretchr/testify/require" ) func TestMetricsEndpointExposesPrometheus(t *testing.T) { ctx := context.Background() - provider, err := telemetry.NewProvider(ctx, &telemetry.Config{ + setup := otelsetup.Setup{} + provider, err := setup.New(ctx, &otelsetup.Config{ ServiceName: "test-onix", ServiceVersion: "1.0.0", EnableMetrics: true, diff --git a/pkg/plugin/implementation/otelsetup/cmd/plugin.go b/pkg/plugin/implementation/otelsetup/cmd/plugin.go index 8aac8f1..30db907 100644 --- a/pkg/plugin/implementation/otelsetup/cmd/plugin.go +++ b/pkg/plugin/implementation/otelsetup/cmd/plugin.go @@ -22,8 +22,8 @@ func (m metricsProvider) New(ctx context.Context, config map[string]string) (*te return nil, nil, errors.New("context cannot be nil") } - // Convert map[string]string to telemetry.Config - telemetryConfig := &telemetry.Config{ + // Convert map[string]string to otelsetup.Config + telemetryConfig := &otelsetup.Config{ ServiceName: config["serviceName"], ServiceVersion: config["serviceVersion"], Environment: config["environment"], @@ -44,13 +44,13 @@ func (m metricsProvider) New(ctx context.Context, config map[string]string) (*te // Apply defaults if fields are empty if telemetryConfig.ServiceName == "" { - telemetryConfig.ServiceName = telemetry.DefaultConfig().ServiceName + telemetryConfig.ServiceName = otelsetup.DefaultConfig().ServiceName } if telemetryConfig.ServiceVersion == "" { - telemetryConfig.ServiceVersion = telemetry.DefaultConfig().ServiceVersion + telemetryConfig.ServiceVersion = otelsetup.DefaultConfig().ServiceVersion } if telemetryConfig.Environment == "" { - telemetryConfig.Environment = telemetry.DefaultConfig().Environment + telemetryConfig.Environment = otelsetup.DefaultConfig().Environment } log.Debugf(ctx, "Telemetry config mapped: %+v", telemetryConfig) diff --git a/pkg/plugin/implementation/otelsetup/cmd/plugin_test.go b/pkg/plugin/implementation/otelsetup/cmd/plugin_test.go index d10a784..2f35c17 100644 --- a/pkg/plugin/implementation/otelsetup/cmd/plugin_test.go +++ b/pkg/plugin/implementation/otelsetup/cmd/plugin_test.go @@ -4,7 +4,7 @@ import ( "context" "testing" - "github.com/beckn-one/beckn-onix/pkg/telemetry" + "github.com/beckn-one/beckn-onix/pkg/plugin/implementation/otelsetup" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -18,8 +18,8 @@ func TestMetricsProviderNew_Success(t *testing.T) { config map[string]string }{ { - name: "Valid config with all fields", - ctx: context.Background(), + name: "Valid config with all fields", + ctx: context.Background(), config: map[string]string{ "serviceName": "test-service", "serviceVersion": "1.0.0", @@ -33,15 +33,15 @@ func TestMetricsProviderNew_Success(t *testing.T) { config: map[string]string{}, }, { - name: "Valid config with enableMetrics false", - ctx: context.Background(), + name: "Valid config with enableMetrics false", + ctx: context.Background(), config: map[string]string{ "enableMetrics": "false", }, }, { - name: "Valid config with partial fields", - ctx: context.Background(), + name: "Valid config with partial fields", + ctx: context.Background(), config: map[string]string{ "serviceName": "custom-service", "serviceVersion": "2.0.0", @@ -105,7 +105,7 @@ func TestMetricsProviderNew_ConfigConversion(t *testing.T) { tests := []struct { name string config map[string]string - expectedConfig *telemetry.Config + expectedConfig *otelsetup.Config }{ { name: "All fields provided", @@ -115,7 +115,7 @@ func TestMetricsProviderNew_ConfigConversion(t *testing.T) { "enableMetrics": "true", "environment": "production", }, - expectedConfig: &telemetry.Config{ + expectedConfig: &otelsetup.Config{ ServiceName: "my-service", ServiceVersion: "3.0.0", EnableMetrics: true, @@ -125,11 +125,11 @@ func TestMetricsProviderNew_ConfigConversion(t *testing.T) { { name: "Empty config uses defaults", config: map[string]string{}, - expectedConfig: &telemetry.Config{ - ServiceName: telemetry.DefaultConfig().ServiceName, - ServiceVersion: telemetry.DefaultConfig().ServiceVersion, + expectedConfig: &otelsetup.Config{ + ServiceName: otelsetup.DefaultConfig().ServiceName, + ServiceVersion: otelsetup.DefaultConfig().ServiceVersion, EnableMetrics: true, // Default when not specified - Environment: telemetry.DefaultConfig().Environment, + Environment: otelsetup.DefaultConfig().Environment, }, }, { @@ -137,11 +137,11 @@ func TestMetricsProviderNew_ConfigConversion(t *testing.T) { config: map[string]string{ "enableMetrics": "false", }, - expectedConfig: &telemetry.Config{ - ServiceName: telemetry.DefaultConfig().ServiceName, - ServiceVersion: telemetry.DefaultConfig().ServiceVersion, + expectedConfig: &otelsetup.Config{ + ServiceName: otelsetup.DefaultConfig().ServiceName, + ServiceVersion: otelsetup.DefaultConfig().ServiceVersion, EnableMetrics: false, - Environment: telemetry.DefaultConfig().Environment, + Environment: otelsetup.DefaultConfig().Environment, }, }, { @@ -149,11 +149,11 @@ func TestMetricsProviderNew_ConfigConversion(t *testing.T) { config: map[string]string{ "enableMetrics": "invalid", }, - expectedConfig: &telemetry.Config{ - ServiceName: telemetry.DefaultConfig().ServiceName, - ServiceVersion: telemetry.DefaultConfig().ServiceVersion, + expectedConfig: &otelsetup.Config{ + ServiceName: otelsetup.DefaultConfig().ServiceName, + ServiceVersion: otelsetup.DefaultConfig().ServiceVersion, EnableMetrics: true, // Defaults to true on parse error - Environment: telemetry.DefaultConfig().Environment, + Environment: otelsetup.DefaultConfig().Environment, }, }, } @@ -308,4 +308,3 @@ func TestMetricsProviderNew_DefaultValues(t *testing.T) { assert.NoError(t, err, "cleanup() should not return error") } } - diff --git a/pkg/plugin/implementation/otelsetup/otelsetup.go b/pkg/plugin/implementation/otelsetup/otelsetup.go index 4419caa..8f7c0ce 100644 --- a/pkg/plugin/implementation/otelsetup/otelsetup.go +++ b/pkg/plugin/implementation/otelsetup/otelsetup.go @@ -22,8 +22,26 @@ import ( // behind the MetricsProvider interface. type Setup struct{} -// ToPluginConfig converts telemetry.Config to plugin.Config format. -func ToPluginConfig(cfg *telemetry.Config) *plugin.Config { +// Config represents OpenTelemetry related configuration. +type Config struct { + ServiceName string `yaml:"serviceName"` + ServiceVersion string `yaml:"serviceVersion"` + EnableMetrics bool `yaml:"enableMetrics"` + Environment string `yaml:"environment"` +} + +// DefaultConfig returns sensible defaults for telemetry configuration. +func DefaultConfig() *Config { + return &Config{ + ServiceName: "beckn-onix", + ServiceVersion: "dev", + EnableMetrics: true, + Environment: "development", + } +} + +// ToPluginConfig converts Config to plugin.Config format. +func ToPluginConfig(cfg *Config) *plugin.Config { return &plugin.Config{ ID: "otelsetup", Config: map[string]string{ @@ -38,20 +56,20 @@ func ToPluginConfig(cfg *telemetry.Config) *plugin.Config { // New initializes the underlying telemetry provider. The returned provider // exposes the HTTP handler and shutdown hooks that the core application can // manage directly. -func (Setup) New(ctx context.Context, cfg *telemetry.Config) (*telemetry.Provider, error) { +func (Setup) New(ctx context.Context, cfg *Config) (*telemetry.Provider, error) { if cfg == nil { return nil, fmt.Errorf("telemetry config cannot be nil") } // Apply defaults if fields are empty if cfg.ServiceName == "" { - cfg.ServiceName = telemetry.DefaultConfig().ServiceName + cfg.ServiceName = DefaultConfig().ServiceName } if cfg.ServiceVersion == "" { - cfg.ServiceVersion = telemetry.DefaultConfig().ServiceVersion + cfg.ServiceVersion = DefaultConfig().ServiceVersion } if cfg.Environment == "" { - cfg.Environment = telemetry.DefaultConfig().Environment + cfg.Environment = DefaultConfig().Environment } if !cfg.EnableMetrics { diff --git a/pkg/plugin/implementation/otelsetup/otelsetup_test.go b/pkg/plugin/implementation/otelsetup/otelsetup_test.go index 10732b0..98ce4eb 100644 --- a/pkg/plugin/implementation/otelsetup/otelsetup_test.go +++ b/pkg/plugin/implementation/otelsetup/otelsetup_test.go @@ -6,7 +6,6 @@ import ( "net/http/httptest" "testing" - "github.com/beckn-one/beckn-onix/pkg/telemetry" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -17,11 +16,11 @@ func TestSetup_New_Success(t *testing.T) { tests := []struct { name string - cfg *telemetry.Config + cfg *Config }{ { name: "Valid config with all fields", - cfg: &telemetry.Config{ + cfg: &Config{ ServiceName: "test-service", ServiceVersion: "1.0.0", EnableMetrics: true, @@ -30,7 +29,7 @@ func TestSetup_New_Success(t *testing.T) { }, { name: "Valid config with metrics disabled", - cfg: &telemetry.Config{ + cfg: &Config{ ServiceName: "test-service", ServiceVersion: "1.0.0", EnableMetrics: false, @@ -39,7 +38,7 @@ func TestSetup_New_Success(t *testing.T) { }, { name: "Config with empty fields uses defaults", - cfg: &telemetry.Config{ + cfg: &Config{ ServiceName: "", ServiceVersion: "", EnableMetrics: true, @@ -82,7 +81,7 @@ func TestSetup_New_Failure(t *testing.T) { tests := []struct { name string - cfg *telemetry.Config + cfg *Config wantErr bool }{ { @@ -112,7 +111,7 @@ func TestSetup_New_DefaultValues(t *testing.T) { ctx := context.Background() // Test with empty fields - should use defaults - cfg := &telemetry.Config{ + cfg := &Config{ ServiceName: "", ServiceVersion: "", EnableMetrics: true, @@ -142,7 +141,7 @@ func TestSetup_New_MetricsDisabled(t *testing.T) { setup := Setup{} ctx := context.Background() - cfg := &telemetry.Config{ + cfg := &Config{ ServiceName: "test-service", ServiceVersion: "1.0.0", EnableMetrics: false, @@ -165,13 +164,13 @@ func TestSetup_New_MetricsDisabled(t *testing.T) { func TestToPluginConfig_Success(t *testing.T) { tests := []struct { name string - cfg *telemetry.Config + cfg *Config expectedID string expectedConfig map[string]string }{ { name: "Valid config with all fields", - cfg: &telemetry.Config{ + cfg: &Config{ ServiceName: "test-service", ServiceVersion: "1.0.0", EnableMetrics: true, @@ -187,7 +186,7 @@ func TestToPluginConfig_Success(t *testing.T) { }, { name: "Config with enableMetrics false", - cfg: &telemetry.Config{ + cfg: &Config{ ServiceName: "my-service", ServiceVersion: "2.0.0", EnableMetrics: false, @@ -203,7 +202,7 @@ func TestToPluginConfig_Success(t *testing.T) { }, { name: "Config with empty fields", - cfg: &telemetry.Config{ + cfg: &Config{ ServiceName: "", ServiceVersion: "", EnableMetrics: true, @@ -259,7 +258,7 @@ func TestToPluginConfig_BooleanConversion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfg := &telemetry.Config{ + cfg := &Config{ ServiceName: "test", ServiceVersion: "1.0.0", EnableMetrics: tt.enableMetrics, diff --git a/pkg/telemetry/metrics_test.go b/pkg/telemetry/metrics_test.go index ee97c7f..1c3663a 100644 --- a/pkg/telemetry/metrics_test.go +++ b/pkg/telemetry/metrics_test.go @@ -10,12 +10,7 @@ import ( func TestNewProviderAndMetrics(t *testing.T) { ctx := context.Background() - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) require.NotNil(t, provider) require.NotNil(t, provider.MetricsHandler) diff --git a/pkg/telemetry/step_instrumentor_test.go b/pkg/telemetry/step_instrumentor_test.go index ea67934..02583ec 100644 --- a/pkg/telemetry/step_instrumentor_test.go +++ b/pkg/telemetry/step_instrumentor_test.go @@ -19,12 +19,7 @@ func (s stubStep) Run(ctx *model.StepContext) error { func TestInstrumentedStepSuccess(t *testing.T) { ctx := context.Background() - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) defer provider.Shutdown(context.Background()) @@ -40,12 +35,7 @@ func TestInstrumentedStepSuccess(t *testing.T) { func TestInstrumentedStepError(t *testing.T) { ctx := context.Background() - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) defer provider.Shutdown(context.Background()) diff --git a/pkg/telemetry/step_metrics_test.go b/pkg/telemetry/step_metrics_test.go index 45dbede..5345501 100644 --- a/pkg/telemetry/step_metrics_test.go +++ b/pkg/telemetry/step_metrics_test.go @@ -15,12 +15,7 @@ func TestGetStepMetrics_Success(t *testing.T) { ctx := context.Background() // Initialize telemetry provider first - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) defer provider.Shutdown(context.Background()) @@ -39,12 +34,7 @@ func TestGetStepMetrics_ConcurrentAccess(t *testing.T) { ctx := context.Background() // Initialize telemetry provider first - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) defer provider.Shutdown(context.Background()) @@ -81,12 +71,7 @@ func TestStepMetrics_Instruments(t *testing.T) { ctx := context.Background() // Initialize telemetry provider - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) defer provider.Shutdown(context.Background()) @@ -128,12 +113,7 @@ func TestStepMetrics_MultipleCalls(t *testing.T) { ctx := context.Background() // Initialize telemetry provider - provider, err := NewProvider(ctx, &Config{ - ServiceName: "test-service", - ServiceVersion: "1.0.0", - EnableMetrics: true, - Environment: "test", - }) + provider, err := NewTestProvider(ctx) require.NoError(t, err) defer provider.Shutdown(context.Background()) diff --git a/pkg/telemetry/telemetry.go b/pkg/telemetry/telemetry.go index 00f48b2..c5b70df 100644 --- a/pkg/telemetry/telemetry.go +++ b/pkg/telemetry/telemetry.go @@ -2,109 +2,14 @@ package telemetry import ( "context" - "fmt" "net/http" - clientprom "github.com/prometheus/client_golang/prometheus" - clientpromhttp "github.com/prometheus/client_golang/prometheus/promhttp" - "go.opentelemetry.io/contrib/instrumentation/runtime" - "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/attribute" - otelprom "go.opentelemetry.io/otel/exporters/prometheus" "go.opentelemetry.io/otel/sdk/metric" - "go.opentelemetry.io/otel/sdk/resource" - - "github.com/beckn-one/beckn-onix/pkg/log" ) -// Config represents OpenTelemetry related configuration. -type Config struct { - ServiceName string `yaml:"serviceName"` - ServiceVersion string `yaml:"serviceVersion"` - EnableMetrics bool `yaml:"enableMetrics"` - Environment string `yaml:"environment"` -} - // Provider holds references to telemetry components that need coordinated shutdown. type Provider struct { MeterProvider *metric.MeterProvider MetricsHandler http.Handler Shutdown func(context.Context) error } - -// DefaultConfig returns sensible defaults for telemetry configuration. -func DefaultConfig() *Config { - return &Config{ - ServiceName: "beckn-onix", - ServiceVersion: "dev", - EnableMetrics: true, - Environment: "development", - } -} - -// NewProvider wires OpenTelemetry with a Prometheus exporter and exposes /metrics handler. -func NewProvider(ctx context.Context, cfg *Config) (*Provider, error) { - if cfg == nil { - cfg = DefaultConfig() - } - if cfg.ServiceName == "" { - cfg.ServiceName = DefaultConfig().ServiceName - } - if cfg.ServiceVersion == "" { - cfg.ServiceVersion = DefaultConfig().ServiceVersion - } - if cfg.Environment == "" { - cfg.Environment = DefaultConfig().Environment - } - - if !cfg.EnableMetrics { - log.Info(ctx, "OpenTelemetry metrics disabled") - return &Provider{ - Shutdown: func(context.Context) error { return nil }, - }, nil - } - - res, err := resource.New( - ctx, - resource.WithAttributes( - attribute.String("service.name", cfg.ServiceName), - attribute.String("service.version", cfg.ServiceVersion), - attribute.String("deployment.environment", cfg.Environment), - ), - ) - if err != nil { - return nil, fmt.Errorf("failed to create telemetry resource: %w", err) - } - - registry := clientprom.NewRegistry() - - exporter, err := otelprom.New( - otelprom.WithRegisterer(registry), - otelprom.WithoutUnits(), - otelprom.WithoutScopeInfo(), - ) - if err != nil { - return nil, fmt.Errorf("failed to create prometheus exporter: %w", err) - } - - meterProvider := metric.NewMeterProvider( - metric.WithReader(exporter), - metric.WithResource(res), - ) - - otel.SetMeterProvider(meterProvider) - log.Infof(ctx, "OpenTelemetry metrics initialized for service=%s version=%s env=%s", - cfg.ServiceName, cfg.ServiceVersion, cfg.Environment) - - if err := runtime.Start(runtime.WithMinimumReadMemStatsInterval(0)); err != nil { - log.Warnf(ctx, "Failed to start Go runtime instrumentation: %v", err) - } - - return &Provider{ - MeterProvider: meterProvider, - MetricsHandler: clientpromhttp.HandlerFor(registry, clientpromhttp.HandlerOpts{}), - Shutdown: func(ctx context.Context) error { - return meterProvider.Shutdown(ctx) - }, - }, nil -} diff --git a/pkg/telemetry/test_helper.go b/pkg/telemetry/test_helper.go new file mode 100644 index 0000000..627965b --- /dev/null +++ b/pkg/telemetry/test_helper.go @@ -0,0 +1,54 @@ +package telemetry + +import ( + "context" + + clientprom "github.com/prometheus/client_golang/prometheus" + clientpromhttp "github.com/prometheus/client_golang/prometheus/promhttp" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + otelprom "go.opentelemetry.io/otel/exporters/prometheus" + "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/resource" +) + +// NewTestProvider creates a minimal telemetry provider for testing purposes. +// This avoids import cycles by not depending on the otelsetup package. +func NewTestProvider(ctx context.Context) (*Provider, error) { + res, err := resource.New( + ctx, + resource.WithAttributes( + attribute.String("service.name", "test-service"), + attribute.String("service.version", "test"), + attribute.String("deployment.environment", "test"), + ), + ) + if err != nil { + return nil, err + } + + registry := clientprom.NewRegistry() + exporter, err := otelprom.New( + otelprom.WithRegisterer(registry), + otelprom.WithoutUnits(), + otelprom.WithoutScopeInfo(), + ) + if err != nil { + return nil, err + } + + meterProvider := metric.NewMeterProvider( + metric.WithReader(exporter), + metric.WithResource(res), + ) + + otel.SetMeterProvider(meterProvider) + + return &Provider{ + MeterProvider: meterProvider, + MetricsHandler: clientpromhttp.HandlerFor(registry, clientpromhttp.HandlerOpts{}), + Shutdown: func(ctx context.Context) error { + return meterProvider.Shutdown(ctx) + }, + }, nil +}