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.

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

[featuregates] Add fields for associated Issues and deprecated version #6447

Conversation

MovieStoreGuy
Copy link
Contributor

Description:
This PR extends the feature gate to add the fields IssueLinks and RemovalVersion to help provide context to users on:

  • Why a feature gate exists?
  • What is the associated issue(s)
  • When will the feature gate will be removed

Link to tracking Issue:

#6167

Testing:

Tests have been updated to include updated to Gate life cycle as per the README

Documentation:

The featureflag README has been updated with an example.

@MovieStoreGuy MovieStoreGuy requested review from a team and mx-psi October 31, 2022 06:37
@MovieStoreGuy
Copy link
Contributor Author

@bogdandrutu ,

I know it has been some time since we spoken about this, but does to align on how envisioned how featuregates would work?

@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Base: 91.89% // Head: 91.88% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (02fb8c5) compared to base (9a56290).
Patch coverage: 95.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6447      +/-   ##
==========================================
- Coverage   91.89%   91.88%   -0.02%     
==========================================
  Files         237      237              
  Lines       13534    13687     +153     
==========================================
+ Hits        12437    12576     +139     
- Misses        869      882      +13     
- Partials      228      229       +1     
Impacted Files Coverage Δ
service/internal/zpages/templates.go 41.66% <ø> (ø)
service/zpages.go 64.00% <0.00%> (-4.09%) ⬇️
obsreport/obsreport_exporter.go 97.90% <96.96%> (-2.10%) ⬇️
featuregate/gates.go 100.00% <100.00%> (ø)
internal/obsreportconfig/obsreportconfig.go 96.90% <100.00%> (ø)
obsreport/obsreporttest/obsreporttest.go 87.09% <100.00%> (-4.69%) ⬇️
obsreport/obsreporttest/otelprometheuschecker.go 93.40% <100.00%> (+1.73%) ⬆️
extension/zpagesextension/config.go 0.00% <0.00%> (-40.00%) ⬇️
extension/ballastextension/memory_ballast.go 82.14% <0.00%> (-7.15%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MovieStoreGuy
Copy link
Contributor Author

The patch is somewhat misleading here, it is not being directly tested, however, these new fields are being added to the templates which are being accessed.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

I think adding an issue link is a non-controversial change, but the new stage is a bit confusing.

featuregate/gates.go Outdated Show resolved Hide resolved
featuregate/gates.go Outdated Show resolved Hide resolved
featuregate/gates.go Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I like the change, but this makes me more nervous about the current API to register a gate. Feels a bit reversed that we construct a Gate pass a copy to the registry and then query the enable status by the registry.IsEnabled.

@MovieStoreGuy @mx-psi I don't know if you realized but the Enabled on the Gate does not return if the gate is enable or not in the current process (represents only the developer configuration). To get the runtime "configuration" need to call Registry.IsEnabled(id).

I find this more and more confusing, I wish we have something like:

type Registry struct {
}

func (r *Registry) MustRegister(id string, stage Stage, opts ..RegisterOptions) *Gate {
}

// Immutable object, owned by the `Registry`.
type Gate struct {
  ...
}

func (g *Gate) ID() string {
}

// Reflect the "runtime" status, so if somewhere else I have `reg.Apply(flags)` this will get changed since it is owned by the registry.
func (g *Gate) IsEnabled() bool {
}

@MovieStoreGuy
Copy link
Contributor Author

That makes sense to me, it make the interaction with the registry clear and it's intent.

featuregate/gates.go Outdated Show resolved Hide resolved
featuregate/gates.go Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the msg/add-issue-link-feature-gate branch from 0241b2a to d39894c Compare November 2, 2022 00:32
Extending feature gates to include reference URL and removal version.
@MovieStoreGuy MovieStoreGuy force-pushed the msg/add-issue-link-feature-gate branch from d39894c to 80a1592 Compare November 2, 2022 00:35
@bogdandrutu
Copy link
Member

bogdandrutu commented Nov 2, 2022

@MovieStoreGuy please add another chloggen entry for the deprecated fields. See https://github.com/open-telemetry/opentelemetry-collector/blob/main/.chloggen/providehelpers.yaml

@MovieStoreGuy MovieStoreGuy force-pushed the msg/add-issue-link-feature-gate branch from bc5214a to 02fb8c5 Compare November 2, 2022 01:15
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM, only a nit

featuregate/gates.go Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit 1bb41d4 into open-telemetry:main Nov 3, 2022
@MovieStoreGuy MovieStoreGuy deleted the msg/add-issue-link-feature-gate branch November 4, 2022 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants