From 549ee7220f3e3826ee11d69ea6a6145b4f158152 Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Thu, 22 Aug 2024 11:04:43 +0200 Subject: [PATCH] Move extension builder into internal service (#10785) #### Description This moves the connector builder out of the `connector` package, and into `service/internal/builders`. There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it. #### Link to tracking issue https://github.com/open-telemetry/opentelemetry-collector/pull/10375#pullrequestreview-2144929463 While this is not technically required for the profiles work (there is no notion of signals in extensions), this PR is here to keep things consistent. --- .chloggen/private-extension-builder.yaml | 25 ++++++ extension/extension.go | 6 ++ extension/extensiontest/nop_extension.go | 3 + internal/e2e/status_test.go | 13 ++- otelcol/collector.go | 9 +- service/extensions/extensions.go | 9 +- service/extensions/extensions_test.go | 9 +- service/internal/builders/extension.go | 69 +++++++++++++++ service/internal/builders/extension_test.go | 97 +++++++++++++++++++++ service/internal/graph/host.go | 2 +- service/service.go | 13 ++- service/service_test.go | 26 +++--- 12 files changed, 248 insertions(+), 33 deletions(-) create mode 100644 .chloggen/private-extension-builder.yaml create mode 100644 service/internal/builders/extension.go create mode 100644 service/internal/builders/extension_test.go diff --git a/.chloggen/private-extension-builder.yaml b/.chloggen/private-extension-builder.yaml new file mode 100644 index 00000000000..ae4a99e3524 --- /dev/null +++ b/.chloggen/private-extension-builder.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: deprecation + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: extension + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Deprecate extension.Builder, and move it into an internal package of the service module + +# One or more tracking issues or pull requests related to the change +issues: [10785] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/extension/extension.go b/extension/extension.go index df4c5432d32..9fc9bdd600b 100644 --- a/extension/extension.go +++ b/extension/extension.go @@ -139,12 +139,18 @@ func MakeFactoryMap(factories ...Factory) (map[component.Type]Factory, error) { } // Builder extension is a helper struct that given a set of Configs and Factories helps with creating extensions. +// +// Deprecated: [v0.108.0] this builder is being internalized within the service module, +// and will be removed soon. type Builder struct { cfgs map[component.ID]component.Config factories map[component.Type]Factory } // NewBuilder creates a new extension.Builder to help with creating components form a set of configs and factories. +// +// Deprecated: [v0.108.0] this builder is being internalized within the service module, +// and will be removed soon. func NewBuilder(cfgs map[component.ID]component.Config, factories map[component.Type]Factory) *Builder { return &Builder{cfgs: cfgs, factories: factories} } diff --git a/extension/extensiontest/nop_extension.go b/extension/extensiontest/nop_extension.go index 1fd483e5e4f..bb4a4493db6 100644 --- a/extension/extensiontest/nop_extension.go +++ b/extension/extensiontest/nop_extension.go @@ -48,6 +48,9 @@ type nopExtension struct { } // NewNopBuilder returns a extension.Builder that constructs nop extension. +// +// Deprecated: [v0.108.0] this builder is being internalized within the service module, +// and will be removed soon. func NewNopBuilder() *extension.Builder { nopFactory := NewNopFactory() return extension.NewBuilder( diff --git a/internal/e2e/status_test.go b/internal/e2e/status_test.go index d57bd6baa76..ab629d0741d 100644 --- a/internal/e2e/status_test.go +++ b/internal/e2e/status_test.go @@ -58,13 +58,12 @@ func Test_ComponentStatusReporting_SharedInstance(t *testing.T) { ConnectorsFactories: map[component.Type]connector.Factory{ nopType: connectorFactory, }, - Extensions: extension.NewBuilder( - map[component.ID]component.Config{ - component.NewID(component.MustNewType("watcher")): &extensionConfig{eventsReceived}, - }, - map[component.Type]extension.Factory{ - component.MustNewType("watcher"): newExtensionFactory(), - }), + ExtensionsConfigs: map[component.ID]component.Config{ + component.NewID(component.MustNewType("watcher")): &extensionConfig{eventsReceived}, + }, + ExtensionsFactories: map[component.Type]extension.Factory{ + component.MustNewType("watcher"): newExtensionFactory(), + }, } set.BuildInfo = component.BuildInfo{Version: "test version", Command: "otelcoltest"} diff --git a/otelcol/collector.go b/otelcol/collector.go index f62cf486bb6..3cabb7f4e29 100644 --- a/otelcol/collector.go +++ b/otelcol/collector.go @@ -183,15 +183,18 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error { } col.service, err = service.New(ctx, service.Settings{ - BuildInfo: col.set.BuildInfo, - CollectorConf: conf, + BuildInfo: col.set.BuildInfo, + CollectorConf: conf, + ReceiversConfigs: cfg.Receivers, ReceiversFactories: factories.Receivers, Processors: processor.NewBuilder(cfg.Processors, factories.Processors), Exporters: exporter.NewBuilder(cfg.Exporters, factories.Exporters), ConnectorsConfigs: cfg.Connectors, ConnectorsFactories: factories.Connectors, - Extensions: extension.NewBuilder(cfg.Extensions, factories.Extensions), + ExtensionsConfigs: cfg.Extensions, + ExtensionsFactories: factories.Extensions, + ModuleInfo: extension.ModuleInfo{ Receiver: factories.ReceiverModules, Processor: factories.ProcessorModules, diff --git a/service/extensions/extensions.go b/service/extensions/extensions.go index e9f4a531611..c3a83192158 100644 --- a/service/extensions/extensions.go +++ b/service/extensions/extensions.go @@ -16,6 +16,7 @@ import ( "go.opentelemetry.io/collector/component/componentstatus" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/extension" + "go.opentelemetry.io/collector/service/internal/builders" "go.opentelemetry.io/collector/service/internal/components" "go.opentelemetry.io/collector/service/internal/status" "go.opentelemetry.io/collector/service/internal/zpages" @@ -168,12 +169,12 @@ func (bes *Extensions) HandleZPages(w http.ResponseWriter, r *http.Request) { // Settings holds configuration for building Extensions. type Settings struct { - Telemetry component.TelemetrySettings - BuildInfo component.BuildInfo + Telemetry component.TelemetrySettings + BuildInfo component.BuildInfo + ModuleInfo extension.ModuleInfo // Extensions builder for extensions. - Extensions *extension.Builder - ModuleInfo extension.ModuleInfo + Extensions builders.Extension } type Option func(*Extensions) diff --git a/service/extensions/extensions_test.go b/service/extensions/extensions_test.go index c7c94033b5a..96bfa36071d 100644 --- a/service/extensions/extensions_test.go +++ b/service/extensions/extensions_test.go @@ -18,6 +18,7 @@ import ( "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/extension" "go.opentelemetry.io/collector/extension/extensiontest" + "go.opentelemetry.io/collector/service/internal/builders" "go.opentelemetry.io/collector/service/internal/status" ) @@ -86,7 +87,7 @@ func TestBuildExtensions(t *testing.T) { _, err := New(context.Background(), Settings{ Telemetry: componenttest.NewNopTelemetrySettings(), BuildInfo: component.NewDefaultBuildInfo(), - Extensions: extension.NewBuilder(tt.extensionsConfigs, tt.factories), + Extensions: builders.NewExtension(tt.extensionsConfigs, tt.factories), }, tt.config) require.Error(t, err) assert.EqualError(t, err, tt.wantErrMsg) @@ -178,7 +179,7 @@ func (tc testOrderCase) testOrdering(t *testing.T) { exts, err := New(context.Background(), Settings{ Telemetry: componenttest.NewNopTelemetrySettings(), BuildInfo: component.NewDefaultBuildInfo(), - Extensions: extension.NewBuilder( + Extensions: builders.NewExtension( extCfgs, map[component.Type]extension.Factory{ recordingExtensionFactory.Type(): recordingExtensionFactory, @@ -280,7 +281,7 @@ func TestNotifyConfig(t *testing.T) { extensions, err := New(context.Background(), Settings{ Telemetry: componenttest.NewNopTelemetrySettings(), BuildInfo: component.NewDefaultBuildInfo(), - Extensions: extension.NewBuilder(tt.extensionsConfigs, tt.factories), + Extensions: builders.NewExtension(tt.extensionsConfigs, tt.factories), }, tt.serviceExtensions) assert.NoError(t, err) errs := extensions.NotifyConfig(context.Background(), confmap.NewFromStringMap(map[string]interface{}{})) @@ -427,7 +428,7 @@ func TestStatusReportedOnStartupShutdown(t *testing.T) { Settings{ Telemetry: componenttest.NewNopTelemetrySettings(), BuildInfo: component.NewDefaultBuildInfo(), - Extensions: extension.NewBuilder(extensionsConfigs, factories), + Extensions: builders.NewExtension(extensionsConfigs, factories), }, []component.ID{compID}, WithReporter(rep), diff --git a/service/internal/builders/extension.go b/service/internal/builders/extension.go new file mode 100644 index 00000000000..9e4eb912dd5 --- /dev/null +++ b/service/internal/builders/extension.go @@ -0,0 +1,69 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package builders // import "go.opentelemetry.io/collector/service/internal/builders" + +import ( + "context" + "fmt" + + "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/extension" + "go.opentelemetry.io/collector/extension/extensiontest" +) + +// Extension is an interface that allows using implementations of the builder +// from different packages. +type Extension interface { + Create(context.Context, extension.Settings) (extension.Extension, error) + Factory(component.Type) component.Factory +} + +// ExtensionBuilder is a helper struct that given a set of Configs and Factories helps with creating extensions. +type ExtensionBuilder struct { + cfgs map[component.ID]component.Config + factories map[component.Type]extension.Factory +} + +// NewExtension creates a new ExtensionBuilder to help with creating +// components form a set of configs and factories. +func NewExtension(cfgs map[component.ID]component.Config, factories map[component.Type]extension.Factory) *ExtensionBuilder { + return &ExtensionBuilder{cfgs: cfgs, factories: factories} +} + +// Create creates an extension based on the settings and configs available. +func (b *ExtensionBuilder) Create(ctx context.Context, set extension.Settings) (extension.Extension, error) { + cfg, existsCfg := b.cfgs[set.ID] + if !existsCfg { + return nil, fmt.Errorf("extension %q is not configured", set.ID) + } + + f, existsFactory := b.factories[set.ID.Type()] + if !existsFactory { + return nil, fmt.Errorf("extension factory not available for: %q", set.ID) + } + + sl := f.ExtensionStability() + if sl >= component.StabilityLevelAlpha { + set.Logger.Debug(sl.LogMessage()) + } else { + set.Logger.Info(sl.LogMessage()) + } + return f.CreateExtension(ctx, set, cfg) +} + +func (b *ExtensionBuilder) Factory(componentType component.Type) component.Factory { + return b.factories[componentType] +} + +// NewNopProcessorConfigsAndFactories returns a configuration and factories that allows building a new nop processor. +func NewNopExtensionConfigsAndFactories() (map[component.ID]component.Config, map[component.Type]extension.Factory) { + nopFactory := extensiontest.NewNopFactory() + configs := map[component.ID]component.Config{ + component.NewID(nopType): nopFactory.CreateDefaultConfig(), + } + factories := map[component.Type]extension.Factory{ + nopType: nopFactory, + } + return configs, factories +} diff --git a/service/internal/builders/extension_test.go b/service/internal/builders/extension_test.go new file mode 100644 index 00000000000..4baa3a8d193 --- /dev/null +++ b/service/internal/builders/extension_test.go @@ -0,0 +1,97 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package builders + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/extension" + "go.opentelemetry.io/collector/extension/extensiontest" +) + +func TestExtensionBuilder(t *testing.T) { + var testType = component.MustNewType("test") + defaultCfg := struct{}{} + testID := component.NewID(testType) + unknownID := component.MustNewID("unknown") + + factories, err := extension.MakeFactoryMap([]extension.Factory{ + extension.NewFactory( + testType, + func() component.Config { return &defaultCfg }, + func(_ context.Context, settings extension.Settings, _ component.Config) (extension.Extension, error) { + return nopExtension{Settings: settings}, nil + }, + component.StabilityLevelDevelopment), + }...) + require.NoError(t, err) + + cfgs := map[component.ID]component.Config{testID: defaultCfg, unknownID: defaultCfg} + b := NewExtension(cfgs, factories) + + e, err := b.Create(context.Background(), createExtensionSettings(testID)) + assert.NoError(t, err) + assert.NotNil(t, e) + + // Check that the extension has access to the resource attributes. + nop, ok := e.(nopExtension) + assert.True(t, ok) + assert.Equal(t, nop.Settings.Resource.Attributes().Len(), 0) + + missingType, err := b.Create(context.Background(), createExtensionSettings(unknownID)) + assert.EqualError(t, err, "extension factory not available for: \"unknown\"") + assert.Nil(t, missingType) + + missingCfg, err := b.Create(context.Background(), createExtensionSettings(component.NewIDWithName(testType, "foo"))) + assert.EqualError(t, err, "extension \"test/foo\" is not configured") + assert.Nil(t, missingCfg) +} + +func TestExtensionBuilderFactory(t *testing.T) { + factories, err := extension.MakeFactoryMap([]extension.Factory{extension.NewFactory(component.MustNewType("foo"), nil, nil, component.StabilityLevelDevelopment)}...) + require.NoError(t, err) + + cfgs := map[component.ID]component.Config{component.MustNewID("foo"): struct{}{}} + b := NewExtension(cfgs, factories) + + assert.NotNil(t, b.Factory(component.MustNewID("foo").Type())) + assert.Nil(t, b.Factory(component.MustNewID("bar").Type())) +} + +func TestNewNopExtensionConfigsAndFactories(t *testing.T) { + configs, factories := NewNopExtensionConfigsAndFactories() + builder := NewExtension(configs, factories) + require.NotNil(t, builder) + + factory := extensiontest.NewNopFactory() + cfg := factory.CreateDefaultConfig() + set := extensiontest.NewNopSettings() + set.ID = component.NewID(nopType) + + ext, err := factory.CreateExtension(context.Background(), set, cfg) + require.NoError(t, err) + bExt, err := builder.Create(context.Background(), set) + require.NoError(t, err) + assert.IsType(t, ext, bExt) +} + +type nopExtension struct { + component.StartFunc + component.ShutdownFunc + extension.Settings +} + +func createExtensionSettings(id component.ID) extension.Settings { + return extension.Settings{ + ID: id, + TelemetrySettings: componenttest.NewNopTelemetrySettings(), + BuildInfo: component.NewDefaultBuildInfo(), + } +} diff --git a/service/internal/graph/host.go b/service/internal/graph/host.go index 692f6913cf9..c03d1f58924 100644 --- a/service/internal/graph/host.go +++ b/service/internal/graph/host.go @@ -35,7 +35,7 @@ type Host struct { Processors *processor.Builder Exporters *exporter.Builder Connectors builders.Connector - Extensions *extension.Builder + Extensions builders.Extension ModuleInfo extension.ModuleInfo BuildInfo component.BuildInfo diff --git a/service/service.go b/service/service.go index 7b3378f5476..a377bd27fa7 100644 --- a/service/service.go +++ b/service/service.go @@ -72,7 +72,11 @@ type Settings struct { ConnectorsFactories map[component.Type]connector.Factory // Extensions builder for extensions. - Extensions *extension.Builder + Extensions builders.Extension + + // Extensions configuration to its builder. + ExtensionsConfigs map[component.ID]component.Config + ExtensionsFactories map[component.Type]extension.Factory // ModuleInfo describes the go module for each component. ModuleInfo extension.ModuleInfo @@ -107,6 +111,11 @@ func New(ctx context.Context, set Settings, cfg Config) (*Service, error) { connectors = builders.NewConnector(set.ConnectorsConfigs, set.ConnectorsFactories) } + extensions := set.Extensions + if extensions == nil { + extensions = builders.NewExtension(set.ExtensionsConfigs, set.ExtensionsFactories) + } + srv := &Service{ buildInfo: set.BuildInfo, host: &graph.Host{ @@ -114,7 +123,7 @@ func New(ctx context.Context, set Settings, cfg Config) (*Service, error) { Processors: set.Processors, Exporters: set.Exporters, Connectors: connectors, - Extensions: set.Extensions, + Extensions: extensions, ModuleInfo: set.ModuleInfo, BuildInfo: set.BuildInfo, AsyncErrorChannel: set.AsyncErrorChannel, diff --git a/service/service_test.go b/service/service_test.go index ce8583b83de..559beee305f 100644 --- a/service/service_test.go +++ b/service/service_test.go @@ -27,7 +27,6 @@ import ( "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/exporter/exportertest" "go.opentelemetry.io/collector/extension" - "go.opentelemetry.io/collector/extension/extensiontest" "go.opentelemetry.io/collector/extension/zpagesextension" "go.opentelemetry.io/collector/internal/testutil" "go.opentelemetry.io/collector/pdata/pcommon" @@ -197,7 +196,7 @@ func TestServiceGetFactory(t *testing.T) { assert.Equal(t, srv.host.Connectors.Factory(nopType), srv.host.GetFactory(component.KindConnector, nopType)) assert.Nil(t, srv.host.GetFactory(component.KindExtension, wrongType)) - assert.Equal(t, set.Extensions.Factory(nopType), srv.host.GetFactory(component.KindExtension, nopType)) + assert.Equal(t, srv.host.Extensions.Factory(nopType), srv.host.GetFactory(component.KindExtension, nopType)) // Try retrieve non existing component.Kind. assert.Nil(t, srv.host.GetFactory(42, nopType)) @@ -291,9 +290,12 @@ func testCollectorStartHelper(t *testing.T, tc ownMetricsTestCase, network strin set := newNopSettings() set.BuildInfo = component.BuildInfo{Version: "test version", Command: otelCommand} - set.Extensions = extension.NewBuilder( - map[component.ID]component.Config{component.MustNewID("zpages"): &zpagesextension.Config{ServerConfig: confighttp.ServerConfig{Endpoint: zpagesAddr}}}, - map[component.Type]extension.Factory{component.MustNewType("zpages"): zpagesextension.NewFactory()}) + set.ExtensionsConfigs = map[component.ID]component.Config{ + component.MustNewID("zpages"): &zpagesextension.Config{ + ServerConfig: confighttp.ServerConfig{Endpoint: zpagesAddr}, + }, + } + set.ExtensionsFactories = map[component.Type]extension.Factory{component.MustNewType("zpages"): zpagesextension.NewFactory()} set.LoggingOptions = []zap.Option{zap.Hooks(hook)} cfg := newNopConfig() @@ -380,9 +382,8 @@ func TestExtensionNotificationFailure(t *testing.T) { var extName = component.MustNewType("configWatcher") configWatcherExtensionFactory := newConfigWatcherExtensionFactory(extName) - set.Extensions = extension.NewBuilder( - map[component.ID]component.Config{component.NewID(extName): configWatcherExtensionFactory.CreateDefaultConfig()}, - map[component.Type]extension.Factory{extName: configWatcherExtensionFactory}) + set.ExtensionsConfigs = map[component.ID]component.Config{component.NewID(extName): configWatcherExtensionFactory.CreateDefaultConfig()} + set.ExtensionsFactories = map[component.Type]extension.Factory{extName: configWatcherExtensionFactory} cfg.Extensions = []component.ID{component.NewID(extName)} // Create a service @@ -403,9 +404,8 @@ func TestNilCollectorEffectiveConfig(t *testing.T) { var extName = component.MustNewType("configWatcher") configWatcherExtensionFactory := newConfigWatcherExtensionFactory(extName) - set.Extensions = extension.NewBuilder( - map[component.ID]component.Config{component.NewID(extName): configWatcherExtensionFactory.CreateDefaultConfig()}, - map[component.Type]extension.Factory{extName: configWatcherExtensionFactory}) + set.ExtensionsConfigs = map[component.ID]component.Config{component.NewID(extName): configWatcherExtensionFactory.CreateDefaultConfig()} + set.ExtensionsFactories = map[component.Type]extension.Factory{extName: configWatcherExtensionFactory} cfg.Extensions = []component.ID{component.NewID(extName)} // Create a service @@ -543,6 +543,7 @@ func assertZPages(t *testing.T, zpagesAddr string) { func newNopSettings() Settings { receiversConfigs, receiversFactories := builders.NewNopReceiverConfigsAndFactories() connectorsConfigs, connectorsFactories := builders.NewNopConnectorConfigsAndFactories() + extensionsConfigs, extensionsFactories := builders.NewNopExtensionConfigsAndFactories() return Settings{ BuildInfo: component.NewDefaultBuildInfo(), @@ -553,7 +554,8 @@ func newNopSettings() Settings { Exporters: exportertest.NewNopBuilder(), ConnectorsConfigs: connectorsConfigs, ConnectorsFactories: connectorsFactories, - Extensions: extensiontest.NewNopBuilder(), + ExtensionsConfigs: extensionsConfigs, + ExtensionsFactories: extensionsFactories, AsyncErrorChannel: make(chan error), } }