Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[builder] Remove builder default confmap providers #11126

Closed
wants to merge 1 commit into from
Closed

[builder] Remove builder default confmap providers #11126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .chloggen/fixbuilderproviders.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'breaking'

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: 'builder'

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Remove default config providers. This will require users to always specify the providers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add subtext with what users have to specify to keep the previous behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is going to break a lot of OCB users, I'd really like to make it as easy as possible to fix which means supplying as much help as possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is going to break a lot of OCB users, I'd really like to make it as easy as possible to fix which means supplying as much help as possible.

I don't believe it, recent data show that is not as big of a deal.


# One or more tracking issues or pull requests related to the change
issues: [11126]

# 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: [user]
61 changes: 25 additions & 36 deletions cmd/builder/internal/builder/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ type Config struct {
Receivers []Module `mapstructure:"receivers"`
Processors []Module `mapstructure:"processors"`
Connectors []Module `mapstructure:"connectors"`
Providers *[]Module `mapstructure:"providers"`
Providers []Module `mapstructure:"providers"`
Replaces []string `mapstructure:"replaces"`
Excludes []string `mapstructure:"excludes"`

Expand Down Expand Up @@ -79,6 +79,13 @@ type Module struct {
Path string `mapstructure:"path"` // an optional path to the local version of this module
}

func (mod *Module) Validate() error {
if mod.GoMod == "" {
return ErrMissingGoMod
}
return nil
}

type retry struct {
numRetries int
wait time.Duration
Expand Down Expand Up @@ -114,17 +121,13 @@ func NewDefaultConfig() Config {

// Validate checks whether the current configuration is valid
func (c *Config) Validate() error {
var providersError error
if c.Providers != nil {
providersError = validateModules("provider", *c.Providers)
}
return multierr.Combine(
validateModules("extension", c.Extensions),
validateModules("receiver", c.Receivers),
validateModules("exporter", c.Exporters),
validateModules("processor", c.Processors),
validateModules("connector", c.Connectors),
providersError,
validateProviders(c.Providers),
)
}

Expand Down Expand Up @@ -207,43 +210,29 @@ func (c *Config) ParseModules() error {
return err
}

if c.Providers != nil {
providers, err := parseModules(*c.Providers)
if err != nil {
return err
}
c.Providers = &providers
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it a problem if no providers are defined, should the builder error out in that case?

providers, err := parseModules([]Module{
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/envprovider v" + c.Distribution.OtelColVersion,
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/fileprovider v" + c.Distribution.OtelColVersion,
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/httpprovider v" + c.Distribution.OtelColVersion,
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/httpsprovider v" + c.Distribution.OtelColVersion,
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/yamlprovider v" + c.Distribution.OtelColVersion,
},
})
if err != nil {
return err
}
c.Providers = &providers
c.Providers, err = parseModules(c.Providers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear this will break most ocb users. If we go this route, is there a gentler approach we could take with feature gates or timelines?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem with this, is that the providers will start to have a different version than the builder and they will be independently mark as stable which will make it very hard to track all combination of builder versions with the "associated default" providers for that version.

This also fixes an issue, that for builder 0.109.0 the default providers will not work since the env and file are no longer using 0.109.0 version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have suggested to actually wait for all providers to be marked stabled in one version, that would have simplified a lot. Otherwise, I am not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear this will break most ocb users. If we go this route, is there a gentler approach we could take with feature gates or timelines?

Also, right now they are broke anyway. Because if they don't specify any provider, it will try to use env/file at 109 and will fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's much that prevents us from marking the other providers as 1.0, I will open the PR for those

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am 100% to add support in the future for featureflags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this particular change, I am fine with any of

  • an error message that tells you what do very explicitly
  • a feature gate
  • a dedicated config flag

The "at least one provider is required" may not be super clear to people (they may not be aware of what providers they used to put in there).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an error message that tells you what do very explicitly

Already there, tell me how you want the error to be, happy to change to a more clear. I think you only care about the message correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just something that tells you "see example on https://opentelemetry.io/docs/collector/custom-collector/" or "previous default was [snippet of code that you can almost copy-paste]" would work for me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Since this breaking change will affect end users of OCB who were not already setting providers, they'll be the most confused about what a provider is and what they need to set. We need to make it obvious what to do from the error message and the changelog entry (and I believe the OCB readme as well), so that they can learn how to fix their manifest.

if err != nil {
return err
}

return nil
}

func validateProviders(providers []Module) error {
if err := validateModules("provider", providers); err != nil {
return err
}
if len(providers) == 0 {
return errors.New("at least one provider is required")
}
return nil
}

func validateModules(name string, mods []Module) error {
var errs error
for i, mod := range mods {
if mod.GoMod == "" {
return fmt.Errorf("%s module at index %v: %w", name, i, ErrMissingGoMod)
if err := mod.Validate(); err != nil {
errs = multierr.Append(errs, fmt.Errorf("%s module at index %v: %w", name, i, err))
}
}
return nil
Expand Down
7 changes: 3 additions & 4 deletions cmd/builder/internal/builder/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func TestMissingModule(t *testing.T) {
{
cfg: Config{
Logger: zap.NewNop(),
Providers: &[]Module{{
Providers: []Module{{
Import: "invalid",
}},
},
Expand Down Expand Up @@ -324,11 +324,10 @@ func TestConfmapFactoryVersions(t *testing.T) {
}
}

func TestAddsDefaultProviders(t *testing.T) {
func TestNoProviders(t *testing.T) {
cfg := NewDefaultConfig()
cfg.Providers = nil
require.NoError(t, cfg.ParseModules())
assert.Len(t, *cfg.Providers, 5)
require.EqualError(t, cfg.Validate(), "at least one provider is required")
}

func TestSkipsNilFieldValidation(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion cmd/builder/internal/builder/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (c *Config) coreModuleAndVersion() (string, string) {

func (c *Config) allComponents() []Module {
return slices.Concat[[]Module](c.Exporters, c.Receivers, c.Processors,
c.Extensions, c.Connectors, *c.Providers)
c.Extensions, c.Connectors, c.Providers)
}

func (c *Config) readGoModFile() (string, map[string]string, error) {
Expand Down
26 changes: 22 additions & 4 deletions cmd/builder/internal/builder/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func TestVersioning(t *testing.T) {
},
})
require.NoError(t, err)
cfg.Providers = &providers
cfg.Providers = providers
return cfg
},
expectedErr: nil,
Expand All @@ -210,7 +210,7 @@ func TestVersioning(t *testing.T) {
GoMod: "go.opentelemetry.io/collector/exporter/otlpexporter v0.97.0",
},
}
cfg.Providers = &[]Module{}
cfg.Providers = []Module{}
cfg.Replaces = append(cfg.Replaces, replaces...)
return cfg
},
Expand All @@ -227,7 +227,7 @@ func TestVersioning(t *testing.T) {
GoMod: "go.opentelemetry.io/collector/exporter/otlpexporter v0.97.0",
},
}
cfg.Providers = &[]Module{}
cfg.Providers = []Module{}
cfg.Replaces = append(cfg.Replaces, replaces...)
return cfg
},
Expand Down Expand Up @@ -323,7 +323,7 @@ func TestGenerateAndCompile(t *testing.T) {
require.NoError(t, err)
cfg.Distribution.OutputPath = t.TempDir()
cfg.Replaces = append(cfg.Replaces, replaces...)
cfg.Providers = &[]Module{}
cfg.Providers = []Module{}
return cfg
},
},
Expand Down Expand Up @@ -456,6 +456,24 @@ func TestReplaceStatementsAreComplete(t *testing.T) {
},
})
require.NoError(t, err)
cfg.Providers, err = parseModules([]Module{
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/envprovider v1.9999.9999",
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/fileprovider v1.9999.9999",
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/httpprovider v1.9999.9999",
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/httpsprovider v1.9999.9999",
},
{
GoMod: "go.opentelemetry.io/collector/confmap/provider/yamlprovider v1.9999.9999",
},
})
require.NoError(t, err)

require.NoError(t, cfg.SetBackwardsCompatibility())
require.NoError(t, cfg.Validate())
Expand Down
7 changes: 7 additions & 0 deletions cmd/builder/test/core.builder.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ receivers:
exporters:
- gomod: go.opentelemetry.io/collector/exporter/debugexporter v0.111.0

providers:
- gomod: go.opentelemetry.io/collector/confmap/provider/envprovider v1.15.0
- gomod: go.opentelemetry.io/collector/confmap/provider/fileprovider v1.15.0
- gomod: go.opentelemetry.io/collector/confmap/provider/httpprovider v0.109.0
- gomod: go.opentelemetry.io/collector/confmap/provider/httpsprovider v0.109.0
- gomod: go.opentelemetry.io/collector/confmap/provider/yamlprovider v0.109.0

replaces:
- go.opentelemetry.io/collector => ${WORKSPACE_DIR}
- go.opentelemetry.io/collector/client => ${WORKSPACE_DIR}/client
Expand Down
Loading