Skip to content

Commit 2b4f9aa

Browse files
authored
Rename Telemetry Builder (because it's not really a Builder) (#1753)
* Uses the telemetrry provider strategys with refactor Signed-off-by: ChrisJBurns <[email protected]> * removes redundant type checks Signed-off-by: ChrisJBurns <[email protected]> * renames builder/build to assembler/assemble to remove builder connotation Signed-off-by: ChrisJBurns <[email protected]> * duplicate comment Signed-off-by: ChrisJBurns <[email protected]> --------- Signed-off-by: ChrisJBurns <[email protected]>
1 parent 6cab357 commit 2b4f9aa

File tree

10 files changed

+467
-453
lines changed

10 files changed

+467
-453
lines changed

docs/server/docs.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/server/swagger.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/server/swagger.yaml

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/telemetry/config.go

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -19,43 +19,43 @@ import (
1919
// Config holds the configuration for OpenTelemetry instrumentation.
2020
type Config struct {
2121
// Endpoint is the OTLP endpoint URL
22-
Endpoint string
22+
Endpoint string `json:"endpoint"`
2323

2424
// ServiceName is the service name for telemetry
25-
ServiceName string
25+
ServiceName string `json:"serviceName"`
2626

2727
// ServiceVersion is the service version for telemetry
28-
ServiceVersion string
28+
ServiceVersion string `json:"serviceVersion"`
2929

3030
// TracingEnabled controls whether distributed tracing is enabled
3131
// When false, no tracer provider is created even if an endpoint is configured
32-
TracingEnabled bool
32+
TracingEnabled bool `json:"tracingEnabled"`
3333

3434
// MetricsEnabled controls whether OTLP metrics are enabled
3535
// When false, OTLP metrics are not sent even if an endpoint is configured
3636
// This is independent of EnablePrometheusMetricsPath
37-
MetricsEnabled bool
37+
MetricsEnabled bool `json:"metricsEnabled"`
3838

3939
// SamplingRate is the trace sampling rate (0.0-1.0)
4040
// Only used when TracingEnabled is true
41-
SamplingRate float64
41+
SamplingRate float64 `json:"samplingRate"`
4242

4343
// Headers contains authentication headers for the OTLP endpoint
44-
Headers map[string]string
44+
Headers map[string]string `json:"headers"`
4545

4646
// Insecure indicates whether to use HTTP instead of HTTPS for the OTLP endpoint
47-
Insecure bool
47+
Insecure bool `json:"insecure"`
4848

4949
// EnablePrometheusMetricsPath controls whether to expose Prometheus-style /metrics endpoint
5050
// The metrics are served on the main transport port at /metrics
5151
// This is separate from OTLP metrics which are sent to the Endpoint
52-
EnablePrometheusMetricsPath bool
52+
EnablePrometheusMetricsPath bool `json:"enablePrometheusMetricsPath"`
5353

5454
// EnvironmentVariables is a list of environment variable names that should be
5555
// included in telemetry spans as attributes. Only variables in this list will
5656
// be read from the host machine and included in spans for observability.
5757
// Example: []string{"NODE_ENV", "DEPLOYMENT_ENV", "SERVICE_VERSION"}
58-
EnvironmentVariables []string
58+
EnvironmentVariables []string `json:"environmentVariables"`
5959
}
6060

6161
// DefaultConfig returns a default telemetry configuration.
@@ -90,20 +90,19 @@ func NewProvider(ctx context.Context, config Config) (*Provider, error) {
9090
return nil, err
9191
}
9292

93-
// Use the new factory pattern
94-
telemetryConfig := providers.Config{
95-
ServiceName: config.ServiceName,
96-
ServiceVersion: config.ServiceVersion,
97-
OTLPEndpoint: config.Endpoint,
98-
Headers: config.Headers,
99-
Insecure: config.Insecure,
100-
TracingEnabled: config.TracingEnabled,
101-
MetricsEnabled: config.MetricsEnabled,
102-
SamplingRate: config.SamplingRate,
103-
EnablePrometheusMetricsPath: config.EnablePrometheusMetricsPath,
93+
telemetryOptions := []providers.ProviderOption{
94+
providers.WithServiceName(config.ServiceName),
95+
providers.WithServiceVersion(config.ServiceVersion),
96+
providers.WithOTLPEndpoint(config.Endpoint),
97+
providers.WithHeaders(config.Headers),
98+
providers.WithInsecure(config.Insecure),
99+
providers.WithTracingEnabled(config.TracingEnabled),
100+
providers.WithMetricsEnabled(config.MetricsEnabled),
101+
providers.WithSamplingRate(config.SamplingRate),
102+
providers.WithEnablePrometheusMetricsPath(config.EnablePrometheusMetricsPath),
104103
}
105104

106-
telemetryProviders, err := providers.WithConfig(telemetryConfig).Build(ctx)
105+
telemetryProviders, err := providers.NewCompositeProvider(ctx, telemetryOptions...)
107106
if err != nil {
108107
return nil, fmt.Errorf("failed to build telemetry providers: %w", err)
109108
}

pkg/telemetry/config_test.go

Lines changed: 0 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,6 @@ import (
1010

1111
"github.com/stretchr/testify/assert"
1212
"github.com/stretchr/testify/require"
13-
"go.opentelemetry.io/otel/metric/noop"
14-
sdkmetric "go.opentelemetry.io/otel/sdk/metric"
15-
"go.opentelemetry.io/otel/sdk/resource"
16-
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
17-
18-
"github.com/stacklok/toolhive/pkg/telemetry/providers"
1913
)
2014

2115
// TestTelemetryProviderValidation tests the five main telemetry configuration scenarios
@@ -164,112 +158,6 @@ func TestTelemetryProviderValidation(t *testing.T) {
164158
}
165159
}
166160

167-
// TestUnifiedMeterStrategyConfiguration tests the unified meter strategy configuration
168-
func TestUnifiedMeterStrategyConfiguration(t *testing.T) {
169-
t.Parallel()
170-
ctx := context.Background()
171-
172-
tests := []struct {
173-
name string
174-
strategy *providers.UnifiedMeterStrategy
175-
config providers.Config
176-
expectError bool
177-
description string
178-
}{
179-
{
180-
name: "OTLP only",
181-
strategy: &providers.UnifiedMeterStrategy{
182-
EnableOTLP: true,
183-
EnablePrometheus: false,
184-
},
185-
config: providers.Config{
186-
ServiceName: "test",
187-
OTLPEndpoint: "localhost:4318",
188-
Insecure: true,
189-
},
190-
expectError: false,
191-
description: "Should create meter provider with OTLP reader only",
192-
},
193-
{
194-
name: "Prometheus only",
195-
strategy: &providers.UnifiedMeterStrategy{
196-
EnableOTLP: false,
197-
EnablePrometheus: true,
198-
},
199-
config: providers.Config{
200-
ServiceName: "test",
201-
},
202-
expectError: false,
203-
description: "Should create meter provider with Prometheus reader only",
204-
},
205-
{
206-
name: "Both OTLP and Prometheus",
207-
strategy: &providers.UnifiedMeterStrategy{
208-
EnableOTLP: true,
209-
EnablePrometheus: true,
210-
},
211-
config: providers.Config{
212-
ServiceName: "test",
213-
OTLPEndpoint: "localhost:4318",
214-
Insecure: true,
215-
},
216-
expectError: false,
217-
description: "Should create meter provider with both readers",
218-
},
219-
}
220-
221-
for _, tt := range tests {
222-
tt := tt // capture range variable
223-
t.Run(tt.name, func(t *testing.T) {
224-
t.Parallel()
225-
226-
// Create resource for testing
227-
res, err := createTestResource(ctx, tt.config)
228-
require.NoError(t, err)
229-
230-
// Test meter provider creation
231-
result, err := tt.strategy.CreateMeterProvider(ctx, tt.config, res)
232-
233-
if tt.expectError {
234-
require.Error(t, err)
235-
return
236-
}
237-
238-
require.NoError(t, err, tt.description)
239-
require.NotNil(t, result)
240-
require.NotNil(t, result.MeterProvider)
241-
242-
// Validate provider type
243-
if tt.strategy.EnableOTLP || tt.strategy.EnablePrometheus {
244-
// Should be SDK meter provider when any reader is enabled
245-
assert.IsType(t, &sdkmetric.MeterProvider{}, result.MeterProvider,
246-
"Should create SDK meter provider when readers are configured")
247-
} else {
248-
// Should be no-op when nothing is enabled
249-
assert.IsType(t, noop.MeterProvider{}, result.MeterProvider,
250-
"Should create no-op meter provider when no readers are configured")
251-
}
252-
253-
// Validate Prometheus handler
254-
if tt.strategy.EnablePrometheus {
255-
assert.NotNil(t, result.PrometheusHandler,
256-
"Should have Prometheus handler when Prometheus is enabled")
257-
} else {
258-
assert.Nil(t, result.PrometheusHandler,
259-
"Should not have Prometheus handler when Prometheus is disabled")
260-
}
261-
262-
// Test shutdown if available - ignore connection errors during test shutdown
263-
if result.ShutdownFunc != nil {
264-
err := result.ShutdownFunc(ctx)
265-
if err != nil && !isConnectionError(err) {
266-
t.Errorf("Shutdown error (non-connection): %v", err)
267-
}
268-
}
269-
})
270-
}
271-
}
272-
273161
// getProviderTypeName returns a readable type name for telemetry providers
274162
func getProviderTypeName(provider interface{}) string {
275163
t := reflect.TypeOf(provider)
@@ -279,16 +167,6 @@ func getProviderTypeName(provider interface{}) string {
279167
return t.PkgPath()[len("go.opentelemetry.io/otel/"):] + "." + t.Name()
280168
}
281169

282-
// createTestResource creates a resource for testing
283-
func createTestResource(ctx context.Context, config providers.Config) (*resource.Resource, error) {
284-
return resource.New(ctx,
285-
resource.WithAttributes(
286-
semconv.ServiceName(config.ServiceName),
287-
semconv.ServiceVersion("1.0.0"),
288-
),
289-
)
290-
}
291-
292170
// isConnectionError checks if the error is a connection-related error that can be ignored in tests
293171
func isConnectionError(err error) bool {
294172
errorStr := err.Error()

0 commit comments

Comments
 (0)