Skip to content

[PoC] Test alternative configoptional implementation #13060

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.

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 26 additions & 0 deletions .chloggen/mx-psi_configoptional-add.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@

# Use this changelog template to create an entry for release notes.

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

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

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add a new configoptional module to support optional configuration fields.

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

# (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: []
1 change: 1 addition & 0 deletions .github/workflows/utils/cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
"configmodels",
"confignet",
"configopaque",
"configoptional",
"configparser",
"configretry",
"configrpc",
Expand Down
1 change: 1 addition & 0 deletions cmd/builder/internal/builder/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ var replaceModules = []string{
"/config/configmiddleware",
"/config/confignet",
"/config/configopaque",
"/config/configoptional",
"/config/configretry",
"/config/configtelemetry",
"/config/configtls",
Expand Down
1 change: 1 addition & 0 deletions cmd/otelcorecol/builder-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ replaces:
- go.opentelemetry.io/collector/config/configmiddleware => ../../config/configmiddleware
- go.opentelemetry.io/collector/config/confignet => ../../config/confignet
- go.opentelemetry.io/collector/config/configopaque => ../../config/configopaque
- go.opentelemetry.io/collector/config/configoptional => ../../config/configoptional
- go.opentelemetry.io/collector/config/configretry => ../../config/configretry
- go.opentelemetry.io/collector/config/configtelemetry => ../../config/configtelemetry
- go.opentelemetry.io/collector/config/configtls => ../../config/configtls
Expand Down
3 changes: 3 additions & 0 deletions cmd/otelcorecol/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ require (
go.opentelemetry.io/collector/config/configmiddleware v0.126.0 // indirect
go.opentelemetry.io/collector/config/confignet v1.32.0 // indirect
go.opentelemetry.io/collector/config/configopaque v1.32.0 // indirect
go.opentelemetry.io/collector/config/configoptional v0.0.0-00010101000000-000000000000 // indirect
go.opentelemetry.io/collector/config/configretry v1.32.0 // indirect
go.opentelemetry.io/collector/config/configtelemetry v0.126.0 // indirect
go.opentelemetry.io/collector/config/configtls v1.32.0 // indirect
Expand Down Expand Up @@ -197,6 +198,8 @@ replace go.opentelemetry.io/collector/config/confignet => ../../config/confignet

replace go.opentelemetry.io/collector/config/configopaque => ../../config/configopaque

replace go.opentelemetry.io/collector/config/configoptional => ../../config/configoptional

replace go.opentelemetry.io/collector/config/configretry => ../../config/configretry

replace go.opentelemetry.io/collector/config/configtelemetry => ../../config/configtelemetry
Expand Down
1 change: 1 addition & 0 deletions config/configoptional/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include ../../Makefile.Common
31 changes: 31 additions & 0 deletions config/configoptional/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
module go.opentelemetry.io/collector/config/configoptional

go 1.23.0

require (
github.com/stretchr/testify v1.10.0
go.opentelemetry.io/collector/confmap v1.31.0
go.uber.org/goleak v1.3.0
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
github.com/gobwas/glob v0.2.3 // indirect
github.com/hashicorp/go-version v1.7.0 // indirect
github.com/knadh/koanf/maps v0.1.2 // indirect
github.com/knadh/koanf/providers/confmap v1.0.0 // indirect
github.com/knadh/koanf/v2 v2.2.0 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
go.opentelemetry.io/collector/featuregate v1.32.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.27.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
sigs.k8s.io/yaml v1.4.0 // indirect
)

replace go.opentelemetry.io/collector/confmap => ../../confmap

replace go.opentelemetry.io/collector/featuregate => ../../featuregate
42 changes: 42 additions & 0 deletions config/configoptional/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

119 changes: 119 additions & 0 deletions config/configoptional/optional.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package configoptional // import "go.opentelemetry.io/collector/config/configoptional"

import (
"fmt"
"reflect"

"go.opentelemetry.io/collector/confmap"
)

// Optional represents a value that may or may not be present.
// It supports three flavors: Some(value), Default(value), and None.
// The zero value of Optional is None.
type Optional[T any] struct {
// Enabled indicates if the value is present.
Enabled bool `mapstructure:"enabled"`
Copy link
Member

Choose a reason for hiding this comment

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

Left a response on the original PR #13044 (comment). IMO adding enabled directly to the optional type is an abstraction leak, it does not need to be done at the framework level, the user always has the option of adding it explicitly to T.


value T
}

// deref a reflect.Type to its underlying type.
func deref(t reflect.Type) reflect.Type {
for t.Kind() == reflect.Ptr {
t = t.Elem()
}
return t
}

// assertNoEnabledField checks that a struct type
// does not have a field with a mapstructure tag "enabled".
func assertNoEnabledField[T any]() {
var i T
t := deref(reflect.TypeOf(i))

if t.Kind() != reflect.Struct {
// Not a struct, no need to check for "enabled" field.
return
}

// Check if the struct has a field with the name "enabled".
for i := 0; i < t.NumField(); i++ {
field := t.Field(i)
if field.Tag.Get("mapstructure") == "enabled" {
panic("configoptional: underlying type cannot have a field with mapstructure tag 'enabled'")
}
}
}

// assertStruct checks that a given type is of struct kind.
func assertStruct[T any]() {
var i T
t := deref(reflect.TypeOf(i))

if t.Kind() != reflect.Struct {
panic(fmt.Sprintf("configoptional: %q does not have a struct kind", t))
}
}

// Some creates an Optional with a set value.
// It panics if T has a field with the mapstructure tag "enabled".
func Some[T any](value T) Optional[T] {
assertNoEnabledField[T]()
return Optional[T]{value: value, Enabled: true}
}

// Default creates an Optional with a default value for unmarshaling.
// It panics if
// - T is not a struct OR
// - T has a field with the mapstructure tag "enabled".
func Default[T any](value T) Optional[T] {
assertStruct[T]()
assertNoEnabledField[T]()
return Optional[T]{value: value, Enabled: false}
}

// None has no value. It is equivalent to Default(zero value) for struct types.
// It panics if T has a field with the mapstructure tag "enabled".
func None[T any]() Optional[T] {
assertNoEnabledField[T]()
return Optional[T]{}
}

// Get returns the value of the Optional.
// If the value is not present, it returns nil.
func (o *Optional[T]) Get() *T {
if !o.Enabled {
return nil
}
return &o.value
}

var _ confmap.Unmarshaler = (*Optional[any])(nil)

// Unmarshal implements the confmap.Unmarshaler interface.
// Unmarshaling from an empty map will set Enabled to true.
//
// It only works for struct types.
// It panics if T has a field with the mapstructure tag "enabled".
func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error {
assertNoEnabledField[T]()

if !conf.IsSet("enabled") {
o.Enabled = true
} else {
if b, ok := conf.Get("enabled").(bool); ok {
o.Enabled = b
} else {
return fmt.Errorf("configoptional: expected bool for 'enabled' field, got %T", conf.Get("enabled"))
}

m := conf.ToStringMap()
delete(m, "enabled")
conf = confmap.NewFromStringMap(m)
}

return conf.Unmarshal(&o.value)
}
Loading
Loading