diff --git a/.chloggen/msg_add-issue-link-feature-gate.yaml b/.chloggen/msg_add-issue-link-feature-gate.yaml new file mode 100755 index 00000000000..129c338285a --- /dev/null +++ b/.chloggen/msg_add-issue-link-feature-gate.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: 'enhancement' + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: featuregates + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: 'Extend feature gate definition to include support for issue links and expected deprecated version' + +# One or more tracking issues or pull requests related to the change +issues: [6167] + +# (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: diff --git a/extension/ballastextension/go.mod b/extension/ballastextension/go.mod index 1ee4e9aec96..cff1dc016da 100644 --- a/extension/ballastextension/go.mod +++ b/extension/ballastextension/go.mod @@ -23,7 +23,7 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c // indirect - github.com/shirou/gopsutil/v3 v3.22.9 // indirect + github.com/shirou/gopsutil/v3 v3.22.10 // indirect github.com/yusufpapurcu/wmi v1.2.2 // indirect go.opentelemetry.io/collector/pdata v0.63.1 // indirect go.opentelemetry.io/otel v1.11.1 // indirect diff --git a/extension/ballastextension/go.sum b/extension/ballastextension/go.sum index 752fe195af8..0f20d027aef 100644 --- a/extension/ballastextension/go.sum +++ b/extension/ballastextension/go.sum @@ -239,8 +239,8 @@ github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb github.com/ryanuber/columnize v2.1.0+incompatible/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts= github.com/ryanuber/go-glob v1.0.0/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIHxIXzX/Yc= github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc= -github.com/shirou/gopsutil/v3 v3.22.9 h1:yibtJhIVEMcdw+tCTbOPiF1VcsuDeTE4utJ8Dm4c5eA= -github.com/shirou/gopsutil/v3 v3.22.9/go.mod h1:bBYl1kjgEJpWpxeHmLI+dVHWtyAwfcmSBLDsp2TNT8A= +github.com/shirou/gopsutil/v3 v3.22.10 h1:4KMHdfBRYXGF9skjDWiL4RA2N+E8dRdodU/bOZpPoVg= +github.com/shirou/gopsutil/v3 v3.22.10/go.mod h1:QNza6r4YQoydyCfo6rH0blGfKahgibh4dQmV5xdFkQk= github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo= github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE= github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88= diff --git a/featuregate/README.md b/featuregate/README.md index f71dd54384c..7e849ceb482 100644 --- a/featuregate/README.md +++ b/featuregate/README.md @@ -10,17 +10,25 @@ based on flags at the component level. Feature gates must be defined and registered with the global registry in an `init()` function. This makes the `Gate` available to be configured and -queried with a default value of its `Enabled` property. +queried with the defined [`Stage`](#feature-lifecycle) default value. +A `Gate` can have a list of associated issues that allow users to refer to +the issue and report any additional problems or understand the context of the `Gate`. +Once a `Gate` has been marked as `Stable`, it must have a `RemovalVersion` set. ```go -const myFeatureGateID = "namespaced.uniqueIdentifier" +const ( + myFeatureGateID = "namespaced.uniqueIdentifier" + myFeatureStage = featuregate.Stable +) func init() { - featuregate.Register(featuregate.Gate{ - ID: myFeatureGateID, - Description: "A brief description of what the gate controls", - Enabled: false, - }) + featuregate.MustRegisterID( + myFeatureGateID, + myFeatureStage, + featuregate.WithRegisterDescription("A brief description of what the gate controls"), + featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector/issues/6167"), + featuregate.WithRegisterRemovalVersion("v0.70.0"), + ) } ``` diff --git a/featuregate/gates.go b/featuregate/gates.go index 0817a5b316d..c31eeb770d1 100644 --- a/featuregate/gates.go +++ b/featuregate/gates.go @@ -19,12 +19,106 @@ import ( "sync" ) -// Gate represents an individual feature that may be enabled or disabled based +// Stage represents the Gate's lifecycle and what is the expected state of it. +type Stage int8 + +const ( + // Alpha is used when creating a new feature and the Gate must be explicitly enabled + // by the operator. + // + // The Gate will be disabled by default. + Alpha Stage = iota + // Beta is used when the feature flag is well tested and is enabled by default, + // but can be disabled by a Gate. + // + // The Gate will be enabled by default. + Beta + // Stable is used when feature is permanently enabled and can not be disabled by a Gate. + // This value is used to provide feedback to the user that the gate will be removed in the next version. + // + // The Gate will be enabled by default and will return an error if modified. + Stable +) + +func (s Stage) String() string { + switch s { + case Alpha: + return "Alpha" + case Beta: + return "Beta" + case Stable: + return "Stable" + } + return "unknown" +} + +// Gate is an immutable object that is owned by the `Registry` +// to represents an individual feature that may be enabled or disabled based // on the lifecycle state of the feature and CLI flags specified by the user. type Gate struct { - ID string + // Deprecated: [v0.64.0] Use GetID() instead to read, + // use `Registry.RegisterID` to set value. + ID string + // Deprecated: [v0.64.0] use GetDescription to read, + // use `WithRegisterDescription` to set using `Registry.RegisterID`. Description string - Enabled bool + // Deprecated: [v0.64.0] use `IsEnabled(id)` to read, + // use `Registry.Apply` to set. + Enabled bool + stage Stage + referenceURL string + removalVersion string +} + +// RegistryOption allows for configuration additional information +// about a gate that can be exposed throughout the application +type RegistryOption func(g *Gate) + +// WithRegisterDescription adds the description to the provided `Gate“. +func WithRegisterDescription(description string) RegistryOption { + return func(g *Gate) { + g.Description = description + } +} + +// WithRegisterReferenceURL adds an URL that has +// all the contextual information about the `Gate`. +func WithRegisterReferenceURL(url string) RegistryOption { + return func(g *Gate) { + g.referenceURL = url + } +} + +// WithRegisterRemovalVersion is used when the `Gate` is considered `Stable`, +// to inform users that referencing the gate is no longer needed. +func WithRegisterRemovalVersion(version string) RegistryOption { + return func(g *Gate) { + g.removalVersion = version + } +} + +func (g *Gate) GetID() string { + return g.ID +} + +func (g *Gate) IsEnabled() bool { + return g.Enabled +} + +func (g *Gate) GetDescription() string { + return g.Description +} + +func (g *Gate) GetStage() Stage { + return g.stage +} + +func (g *Gate) GetReferenceURL() string { + return g.referenceURL +} + +func (g *Gate) GetRemovalVersion() string { + return g.removalVersion } var reg = NewRegistry() @@ -54,6 +148,9 @@ func (r *Registry) Apply(cfg map[string]bool) error { if !ok { return fmt.Errorf("feature gate %s is unregistered", id) } + if g.stage == Stable { + return fmt.Errorf("feature gate %s is stable, can not be modified", id) + } g.Enabled = val r.gates[g.ID] = g } @@ -69,6 +166,7 @@ func (r *Registry) IsEnabled(id string) bool { } // MustRegister like Register but panics if a Gate with the same ID is already registered. +// Deprecated: [v0.64.0] Use MustRegisterID instead. func (r *Registry) MustRegister(g Gate) { if err := r.Register(g); err != nil { panic(err) @@ -76,6 +174,7 @@ func (r *Registry) MustRegister(g Gate) { } // Register registers a Gate. May only be called in an init() function. +// Deprecated: [v0.64.0] Use RegisterID instead. func (r *Registry) Register(g Gate) error { r.mu.Lock() defer r.mu.Unlock() @@ -86,6 +185,41 @@ func (r *Registry) Register(g Gate) error { return nil } +// MustRegisterID like RegisterID but panics if an invalid ID or gate options are provided. +func (r *Registry) MustRegisterID(id string, stage Stage, opts ...RegistryOption) { + if err := r.RegisterID(id, stage, opts...); err != nil { + panic(err) + } +} + +func (r *Registry) RegisterID(id string, stage Stage, opts ...RegistryOption) error { + r.mu.Lock() + defer r.mu.Unlock() + if _, ok := r.gates[id]; ok { + return fmt.Errorf("attempted to add pre-existing gate %q", id) + } + g := Gate{ + ID: id, + stage: stage, + } + for _, opt := range opts { + opt(&g) + } + switch g.stage { + case Alpha: + g.Enabled = false + case Beta, Stable: + g.Enabled = true + default: + return fmt.Errorf("unknown stage value %q for gate %q", stage, id) + } + if g.stage == Stable && g.removalVersion == "" { + return fmt.Errorf("no removal version set for stable gate %q", id) + } + r.gates[id] = g + return nil +} + // List returns a slice of copies of all registered Gates. func (r *Registry) List() []Gate { r.mu.RLock() diff --git a/featuregate/gates_test.go b/featuregate/gates_test.go index 0ea1e058493..878cad36967 100644 --- a/featuregate/gates_test.go +++ b/featuregate/gates_test.go @@ -47,12 +47,18 @@ func TestRegistry(t *testing.T) { func TestRegistryWithErrorApply(t *testing.T) { r := Registry{gates: map[string]Gate{}} - gate := Gate{ + + assert.NoError(t, r.Register(Gate{ ID: "foo", Description: "Test Gate", - Enabled: true, - } - assert.NoError(t, r.Register(gate)) + stage: Alpha, + })) + assert.NoError(t, r.Register(Gate{ + ID: "stable-foo", + Description: "Test Gate", + stage: Stable, + removalVersion: "next", + })) tests := []struct { name string @@ -72,6 +78,12 @@ func TestRegistryWithErrorApply(t *testing.T) { enabled: false, shouldError: true, }, + { + name: "stable gate modified", + gate: "stable-foo", + enabled: false, + shouldError: true, + }, } for _, tt := range tests { @@ -85,3 +97,105 @@ func TestRegistryWithErrorApply(t *testing.T) { }) } } + +func TestRegisterGateLifecycle(t *testing.T) { + for _, tc := range []struct { + name string + id string + stage Stage + opts []RegistryOption + enabled bool + shouldErr bool + }{ + { + name: "Alpha Flag", + id: "test-gate", + stage: Alpha, + enabled: false, + shouldErr: false, + }, + { + name: "Alpha Flag with all options", + id: "test-gate", + stage: Alpha, + opts: []RegistryOption{ + WithRegisterDescription("test-gate"), + WithRegisterReferenceURL("http://example.com/issue/1"), + WithRegisterRemovalVersion(""), + }, + enabled: false, + shouldErr: false, + }, + { + name: "Beta Flag", + id: "test-gate", + stage: Beta, + enabled: true, + shouldErr: false, + }, + { + name: "Stable Flag", + id: "test-gate", + stage: Stable, + opts: []RegistryOption{ + WithRegisterRemovalVersion("next"), + }, + enabled: true, + shouldErr: false, + }, + { + name: "Invalid stage", + id: "test-gate", + stage: Stage(-1), + shouldErr: true, + }, + { + name: "Stable gate missing removal version", + id: "test-gate", + stage: Stable, + shouldErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + r := NewRegistry() + if tc.shouldErr { + assert.Error(t, r.RegisterID(tc.id, tc.stage, tc.opts...), "Must error when registering gate") + assert.Panics(t, func() { + r.MustRegisterID(tc.id, tc.stage, tc.opts...) + }) + return + } + assert.NoError(t, r.RegisterID(tc.id, tc.stage, tc.opts...), "Must not error when registering feature gate") + assert.Equal(t, tc.enabled, r.IsEnabled(tc.id), "Must match the expected enabled value") + }) + } +} + +func TestGateMethods(t *testing.T) { + g := &Gate{ + ID: "test", + Description: "test gate", + Enabled: false, + stage: Alpha, + referenceURL: "http://example.com", + removalVersion: "v0.64.0", + } + + assert.Equal(t, "test", g.GetID()) + assert.Equal(t, "test gate", g.GetDescription()) + assert.Equal(t, false, g.IsEnabled()) + assert.Equal(t, Alpha, g.GetStage()) + assert.Equal(t, "http://example.com", g.GetReferenceURL()) + assert.Equal(t, "v0.64.0", g.GetRemovalVersion()) +} + +func TestStageNames(t *testing.T) { + for expected, s := range map[string]Stage{ + "Alpha": Alpha, + "Beta": Beta, + "Stable": Stable, + "unknown": Stage(-1), + } { + assert.Equal(t, expected, s.String()) + } +} diff --git a/internal/obsreportconfig/obsreportconfig.go b/internal/obsreportconfig/obsreportconfig.go index ba8f482130c..a7d72876bbc 100644 --- a/internal/obsreportconfig/obsreportconfig.go +++ b/internal/obsreportconfig/obsreportconfig.go @@ -37,11 +37,11 @@ func init() { // RegisterInternalMetricFeatureGate registers the Internal Metric feature gate to the passed in registry func RegisterInternalMetricFeatureGate(registry *featuregate.Registry) { - registry.MustRegister(featuregate.Gate{ - ID: UseOtelForInternalMetricsfeatureGateID, - Description: "controls whether the collector uses OpenTelemetry for internal metrics", - Enabled: false, - }) + registry.MustRegisterID( + UseOtelForInternalMetricsfeatureGateID, + featuregate.Alpha, + featuregate.WithRegisterDescription("controls whether the collector uses OpenTelemetry for internal metrics"), + ) } // ObsMetrics wraps OpenCensus View for Collector observability metrics diff --git a/service/internal/zpages/templates.go b/service/internal/zpages/templates.go index 9388e802355..e045fae73f8 100644 --- a/service/internal/zpages/templates.go +++ b/service/internal/zpages/templates.go @@ -168,9 +168,12 @@ type FeatureGateTableData struct { // FeatureGateTableRowData contains data for one row in feature gate table template. type FeatureGateTableRowData struct { - ID string - Enabled bool - Description string + ID string + Enabled bool + Description string + Stage string + ReferenceURL string + RemovalVersion string } // WriteHTMLFeaturesTable writes a table summarizing registered feature gates. diff --git a/service/internal/zpages/templates/features_table.html b/service/internal/zpages/templates/features_table.html index 4aced349adb..28b32abc0a5 100644 --- a/service/internal/zpages/templates/features_table.html +++ b/service/internal/zpages/templates/features_table.html @@ -5,6 +5,12 @@ Enabled   |   Description +   |   + Stage +   |   + Reference URL +   |   + Removal Version {{range $rowindex, $row := .Rows}} {{- if even $rowindex}} @@ -14,6 +20,9 @@ {{$row.ID}}  |   {{$row.Enabled}}  |   {{$row.Description}}  |   + {{$row.Stage}}  |   + {{$row.ReferenceURL}}  |   + {{$row.RemovalVersion}}  |   {{end}} diff --git a/service/zpages.go b/service/zpages.go index cb625bd53e2..c38e3911b51 100644 --- a/service/zpages.go +++ b/service/zpages.go @@ -72,9 +72,12 @@ func getFeaturesTableData() zpages.FeatureGateTableData { data := zpages.FeatureGateTableData{} for _, g := range featuregate.GetRegistry().List() { data.Rows = append(data.Rows, zpages.FeatureGateTableRowData{ - ID: g.ID, - Enabled: g.Enabled, - Description: g.Description, + ID: g.GetID(), + Enabled: g.IsEnabled(), + Description: g.GetDescription(), + ReferenceURL: g.GetReferenceURL(), + Stage: g.GetStage().String(), + RemovalVersion: g.GetRemovalVersion(), }) }