From 4fb36c9f5f6d69c6186b240d720a14d43aeafb97 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Mon, 3 Apr 2023 19:43:20 -0700 Subject: [PATCH] featuregate: Add deprecated Stage and to/from versions Fixes 2 out of the 3 tasks in https://github.com/open-telemetry/opentelemetry-collector/issues/7043 Signed-off-by: Bogdan Drutu --- .chloggen/deprecateremoval.yaml | 11 ++++++++ .chloggen/featuregatetasks.yaml | 11 ++++++++ .chloggen/tofromversion.yaml | 11 ++++++++ confmap/resolver.go | 2 +- featuregate/README.md | 5 ++-- featuregate/flag_test.go | 2 +- featuregate/gate.go | 27 +++++++++++++------ featuregate/gate_test.go | 16 ++++++----- featuregate/registry.go | 22 +++++++++++---- featuregate/registry_test.go | 6 ++--- featuregate/stage.go | 9 ++++++- service/internal/zpages/templates.go | 13 ++++----- .../zpages/templates/features_table.html | 22 ++++++++------- service/zpages.go | 13 ++++----- 14 files changed, 121 insertions(+), 49 deletions(-) create mode 100755 .chloggen/deprecateremoval.yaml create mode 100755 .chloggen/featuregatetasks.yaml create mode 100755 .chloggen/tofromversion.yaml diff --git a/.chloggen/deprecateremoval.yaml b/.chloggen/deprecateremoval.yaml new file mode 100755 index 00000000000..e57648043a5 --- /dev/null +++ b/.chloggen/deprecateremoval.yaml @@ -0,0 +1,11 @@ +# 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: featuregate + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Deprecate Gate.RemovalVersion and WithRegisterRemovalVersion in favor of ToVersion. + +# One or more tracking issues or pull requests related to the change +issues: [7043] diff --git a/.chloggen/featuregatetasks.yaml b/.chloggen/featuregatetasks.yaml new file mode 100755 index 00000000000..0ae52628919 --- /dev/null +++ b/.chloggen/featuregatetasks.yaml @@ -0,0 +1,11 @@ +# 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: featuregate + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add a new Deprecated stage for feature gates, when features are abandoned. + +# One or more tracking issues or pull requests related to the change +issues: [7043] diff --git a/.chloggen/tofromversion.yaml b/.chloggen/tofromversion.yaml new file mode 100755 index 00000000000..09ae18a4bee --- /dev/null +++ b/.chloggen/tofromversion.yaml @@ -0,0 +1,11 @@ +# 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: featuregate + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add concept of gate lifetime, [fromVersion, toVersion]. + +# One or more tracking issues or pull requests related to the change +issues: [7043] diff --git a/confmap/resolver.go b/confmap/resolver.go index f57e2995faa..b6755abfc29 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -50,7 +50,7 @@ var ( var _ = featuregate.GlobalRegistry().MustRegister( "confmap.expandEnabled", featuregate.StageStable, - featuregate.WithRegisterRemovalVersion("v0.75.0"), + featuregate.WithRegisterToVersion("v0.75.0"), featuregate.WithRegisterDescription("controls whether expanding embedded external config providers URIs")) // Resolver resolves a configuration as a Conf. diff --git a/featuregate/README.md b/featuregate/README.md index bbaf3770064..20e482e0a98 100644 --- a/featuregate/README.md +++ b/featuregate/README.md @@ -18,10 +18,11 @@ Once a `Gate` has been marked as `Stable`, it must have a `RemovalVersion` set. ```go var myFeatureGate = featuregate.GlobalRegistry().MustRegister( "namespaced.uniqueIdentifier", - featuregate.Stable, + featuregate.Stable, + featuregate.WithRegisterFromVersion("v0.65.0") 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")) + featuregate.WithRegisterToVersion("v0.70.0")) ``` The status of the gate may later be checked by interrogating the global diff --git a/featuregate/flag_test.go b/featuregate/flag_test.go index 09400a08446..ecf631bb7a9 100644 --- a/featuregate/flag_test.go +++ b/featuregate/flag_test.go @@ -110,7 +110,7 @@ func TestNewFlag(t *testing.T) { reg := NewRegistry() reg.MustRegister("alpha", StageAlpha) reg.MustRegister("beta", StageBeta) - reg.MustRegister("stable", StageStable, WithRegisterRemovalVersion("1.0.0")) + reg.MustRegister("stable", StageStable, WithRegisterToVersion("1.0.0")) v := NewFlag(reg) if tt.expectedSetErr { require.Error(t, v.Set(tt.input)) diff --git a/featuregate/gate.go b/featuregate/gate.go index 6428de07d5c..3c573f41285 100644 --- a/featuregate/gate.go +++ b/featuregate/gate.go @@ -19,12 +19,13 @@ import "sync/atomic" // Gate is an immutable object that is owned by the Registry and 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 - description string - referenceURL string - removalVersion string - stage Stage - enabled *atomic.Bool + id string + description string + referenceURL string + fromVersion string + toVersion string + stage Stage + enabled *atomic.Bool } // ID returns the id of the Gate. @@ -52,7 +53,17 @@ func (g *Gate) ReferenceURL() string { return g.referenceURL } -// RemovalVersion returns the removal version information for Gate's in StageStable. +// FromVersion returns the version information when the Gate's was added. +func (g *Gate) FromVersion() string { + return g.fromVersion +} + +// ToVersion returns the version information when Gate's in StageStable. +func (g *Gate) ToVersion() string { + return g.toVersion +} + +// Deprecated: [v0.76.0] use ToVersion(). func (g *Gate) RemovalVersion() string { - return g.removalVersion + return g.ToVersion() } diff --git a/featuregate/gate_test.go b/featuregate/gate_test.go index babbf333fef..7d13d0f7ae8 100644 --- a/featuregate/gate_test.go +++ b/featuregate/gate_test.go @@ -25,12 +25,13 @@ func TestGate(t *testing.T) { enabled := &atomic.Bool{} enabled.Store(true) g := &Gate{ - id: "test", - description: "test gate", - enabled: enabled, - stage: StageAlpha, - referenceURL: "http://example.com", - removalVersion: "v0.64.0", + id: "test", + description: "test gate", + enabled: enabled, + stage: StageAlpha, + referenceURL: "http://example.com", + fromVersion: "v0.61.0", + toVersion: "v0.64.0", } assert.Equal(t, "test", g.ID()) @@ -38,5 +39,6 @@ func TestGate(t *testing.T) { assert.True(t, g.IsEnabled()) assert.Equal(t, StageAlpha, g.Stage()) assert.Equal(t, "http://example.com", g.ReferenceURL()) - assert.Equal(t, "v0.64.0", g.RemovalVersion()) + assert.Equal(t, "v0.61.0", g.FromVersion()) + assert.Equal(t, "v0.64.0", g.ToVersion()) } diff --git a/featuregate/registry.go b/featuregate/registry.go index 8371b5e16ae..c6b5a2f89f4 100644 --- a/featuregate/registry.go +++ b/featuregate/registry.go @@ -62,11 +62,23 @@ func WithRegisterReferenceURL(url string) RegisterOption { }) } -// WithRegisterRemovalVersion is used when the Gate is considered StageStable, -// to inform users that referencing the gate is no longer needed. -func WithRegisterRemovalVersion(version string) RegisterOption { +// Deprecated: [v0.76.0] use WithRegisterToVersion. +var WithRegisterRemovalVersion = WithRegisterToVersion + +// WithRegisterFromVersion is used to set the Gate "FromVersion". +// The "FromVersion" contains the Collector release when a feature is introduced. +func WithRegisterFromVersion(fromVersion string) RegisterOption { + return registerOptionFunc(func(g *Gate) { + g.fromVersion = fromVersion + }) +} + +// WithRegisterToVersion is used to set the Gate "ToVersion". +// The "ToVersion", if not empty, contains the last Collector release in which you can still use a feature gate. +// If the feature stage is either "Deprecated" or "Stable", the "ToVersion" is the Collector release when the feature is removed. +func WithRegisterToVersion(toVersion string) RegisterOption { return registerOptionFunc(func(g *Gate) { - g.removalVersion = version + g.toVersion = toVersion }) } @@ -98,7 +110,7 @@ func (r *Registry) Register(id string, stage Stage, opts ...RegisterOption) (*Ga default: return nil, fmt.Errorf("unknown stage value %q for gate %q", stage, id) } - if g.stage == StageStable && g.removalVersion == "" { + if g.stage == StageStable && g.toVersion == "" { return nil, fmt.Errorf("no removal version set for stable gate %q", id) } if _, loaded := r.gates.LoadOrStore(id, g); loaded { diff --git a/featuregate/registry_test.go b/featuregate/registry_test.go index e0179333db6..3f5c5cae977 100644 --- a/featuregate/registry_test.go +++ b/featuregate/registry_test.go @@ -55,7 +55,7 @@ func TestRegistryApplyError(t *testing.T) { assert.Error(t, r.Set("foo", true)) r.MustRegister("bar", StageAlpha) assert.Error(t, r.Set("foo", true)) - r.MustRegister("foo", StageStable, WithRegisterRemovalVersion("next")) + r.MustRegister("foo", StageStable, WithRegisterToVersion("next")) assert.Error(t, r.Set("foo", true)) } @@ -90,7 +90,7 @@ func TestRegisterGateLifecycle(t *testing.T) { opts: []RegisterOption{ WithRegisterDescription("test-gate"), WithRegisterReferenceURL("http://example.com/issue/1"), - WithRegisterRemovalVersion(""), + WithRegisterToVersion(""), }, enabled: false, shouldErr: false, @@ -107,7 +107,7 @@ func TestRegisterGateLifecycle(t *testing.T) { id: "test-gate", stage: StageStable, opts: []RegisterOption{ - WithRegisterRemovalVersion("next"), + WithRegisterToVersion("next"), }, enabled: true, shouldErr: false, diff --git a/featuregate/stage.go b/featuregate/stage.go index 58dd0c721cd..9d5505422f8 100644 --- a/featuregate/stage.go +++ b/featuregate/stage.go @@ -29,10 +29,15 @@ const ( // The Gate will be enabled by default. StageBeta // StageStable 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. + // This value is used to provide feedback to the user that the gate will be removed in the next versions. // // The Gate will be enabled by default and will return an error if modified. StageStable + // StageDeprecated is used when feature is permanently disabled and can not be enabled by a Gate. + // This value is used to provide feedback to the user that the gate will be removed in the next versions. + // + // The Gate will be disabled by default and will return an error if modified. + StageDeprecated ) func (s Stage) String() string { @@ -43,6 +48,8 @@ func (s Stage) String() string { return "Beta" case StageStable: return "Stable" + case StageDeprecated: + return "Deprecated" } return "Unknown" } diff --git a/service/internal/zpages/templates.go b/service/internal/zpages/templates.go index e045fae73f8..e6a1af7d881 100644 --- a/service/internal/zpages/templates.go +++ b/service/internal/zpages/templates.go @@ -168,12 +168,13 @@ type FeatureGateTableData struct { // FeatureGateTableRowData contains data for one row in feature gate table template. type FeatureGateTableRowData struct { - ID string - Enabled bool - Description string - Stage string - ReferenceURL string - RemovalVersion string + ID string + Enabled bool + Description string + Stage string + FromVersion string + ToVersion string + ReferenceURL 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 28b32abc0a5..3f1a747bc2c 100644 --- a/service/internal/zpages/templates/features_table.html +++ b/service/internal/zpages/templates/features_table.html @@ -8,21 +8,25 @@   |   Stage   |   - Reference URL + From Version +   |   + To Version   |   - Removal Version + Reference URL {{range $rowindex, $row := .Rows}} {{- if even $rowindex}} {{else}} - {{end -}} - {{$row.ID}}  |   - {{$row.Enabled}}  |   - {{$row.Description}}  |   - {{$row.Stage}}  |   - {{$row.ReferenceURL}}  |   - {{$row.RemovalVersion}}  |   + + {{end -}} + {{$row.ID}}  |   + {{$row.Enabled}}  |   + {{$row.Description}}  |   + {{$row.Stage}}  |   + {{$row.FromVersion}}  |   + {{$row.ToVersion}}  |   + {{$row.ReferenceURL}}  |   {{end}} diff --git a/service/zpages.go b/service/zpages.go index e00f4689b0b..056653671b6 100644 --- a/service/zpages.go +++ b/service/zpages.go @@ -73,12 +73,13 @@ func getFeaturesTableData() zpages.FeatureGateTableData { data := zpages.FeatureGateTableData{} featuregate.GlobalRegistry().VisitAll(func(gate *featuregate.Gate) { data.Rows = append(data.Rows, zpages.FeatureGateTableRowData{ - ID: gate.ID(), - Enabled: gate.IsEnabled(), - Description: gate.Description(), - ReferenceURL: gate.ReferenceURL(), - Stage: gate.Stage().String(), - RemovalVersion: gate.RemovalVersion(), + ID: gate.ID(), + Enabled: gate.IsEnabled(), + Description: gate.Description(), + Stage: gate.Stage().String(), + FromVersion: gate.FromVersion(), + ToVersion: gate.ToVersion(), + ReferenceURL: gate.ReferenceURL(), }) }) return data