Skip to content

Experimentation options#55902

Merged
tmat merged 6 commits intodotnet:mainfrom
tmat:ExperimentationOptions
Sep 1, 2021
Merged

Experimentation options#55902
tmat merged 6 commits intodotnet:mainfrom
tmat:ExperimentationOptions

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Aug 25, 2021

Implements option persisters for feature flags and replaces calls to IExperimentationService with reads/writes of the corresponding options.
As with other options, feature flag options defined in Workspaces and Features layers are serialized and available OOP via Solution.Options. All other feature flag options are only available in-proc. This means the feature flag options need to be defined along the feature implementation, not in central location like WellKnownExperimentNames.

The previous implementation of IExperimentationService in VS layer conflated feature flags and experimental flights.

My understanding is following:
IVsExperimentationService.IsCachedFlightEnabled checks whether the user is selected to participate in a specified flight name. It has nothing to do with feature flags.
IVsFeatureFlags.IsFeatureEnabled checks a feature flag that's stored in VS feature flag storage.

VS provides two services:

  1. Control Tower that defines flights.
  2. Targeted Notifications that allows us to set feature flags. A specified feature flag can be set for a given flight name (among other criteria).

I believe we should not check flight names directly in order to enable/disable a feature but always use a feature flag. This PR deletes calls to IVsExperimentationService.IsCachedFlightEnabled, which checks for active flight name.

Active feature flags:

Roslyn.AsynchronousQuickActions (used by a/b experiment)
Roslyn.CloudCache (used by a/b experiment)
Roslyn.OOPServerGC (used by a/b experiment)
Roslyn.RemoveUnusedReferences
Roslyn.TypeImportCompletion
Roslyn.TargetTypedCompletionFilter
Roslyn.UnnamedSymbolCompletionDisabled
Roslyn.LSP.Completion (listed in PackageRegistration.pkgdef)
Roslyn.LSP.Editor (listed in PackageRegistration.pkgdef)
Roslyn.ProgressionForceLegacySearch
Roslyn.SourceGeneratorsEnableOpeningInWorkspace
Roslyn.OOPCoreClr (listed in PackageRegistration.pkgdef)
Lsp.PullDiagnostics (listed in PackageRegistration.pkgdef)
Roslyn.PartialLoadMode (listed in FeatureFlags.xml)
Xaml.EnableLspIntelliSense (listed in FeatureFlags.xml)

Only two feature flags are listed in src/vscommon/featureflags/FeatureFlags.xml 4 are listed in PackageRegistration.pkgdef These are displayed in Options > Preview Features. Should we present all feature flags there instead of having some of them displayed in C#/VB specific option panes? Follow up: #55974

Unused feature flags (deleted by this PR):

Roslyn.InsertFullMethodCall
Roslyn.ImportsOnPasteDefaultEnabled
Roslyn.LspTextSyncEnabled
Roslyn.InheritanceMargin

Flight names (Control Tower reports as completed and no longer running, deleted by this PR) - not clear if we still need KeybindingResetDetector at all. Following up.

keybindgoldbarext
keybindgoldbarint

Fixes #43477

@ghost ghost added the Area-IDE label Aug 25, 2021
@tmat tmat force-pushed the ExperimentationOptions branch 4 times, most recently from 0631b99 to 139bc3e Compare August 26, 2021 22:57
@tmat tmat marked this pull request as ready for review August 27, 2021 21:26
@tmat tmat requested a review from a team as a code owner August 27, 2021 21:26
@tmat tmat requested a review from a team August 27, 2021 21:26
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Aug 27, 2021

@ryzngard @jasonmalinowski @CyrusNajmabadi @dotnet/roslyn-ide PTAL

Copy link
Copy Markdown
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

🎉

@dibarbet
Copy link
Copy Markdown
Member

dibarbet commented Aug 27, 2021

Only two feature flags are listed in src/vscommon/featureflags/FeatureFlags.xml. These are displayed in Options > Preview Features. Should we present all feature flags there instead of having some of them displayed in C#/VB specific option panes? Follow up: #55974

They don't have to be added to src/vscommon/featureflags/FeatureFlags.xml to be presented in Options -> Preview Features. This is how we add to that page in roslyn - https://github.com/dotnet/roslyn/blob/main/src/VisualStudio/Core/Def/PackageRegistration.pkgdef#L16.

Imho it seems to be a generally appropriate location for feature flags.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Aug 27, 2021

They don't have to added to src/vscommon/featureflags/FeatureFlags.xml to be presented in Options -> Preview Features. This is how we add to that page in roslyn - https://github.com/dotnet/roslyn/blob/main/src/VisualStudio/Core/Def/PackageRegistration.pkgdef#L16.

Imho it seems to be a generally appropriate location for feature flags.

I see. That makes sense. I think we should remove our options from FeatureFlags.xml and put them in PackageRegistration.pkgdef then. It'd be good to have a single place for them.

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Approved given confirmation about the threadign questions, and a strong ask that you make the change here: https://github.com/dotnet/roslyn/pull/55902/files#r698729640

@tmat tmat force-pushed the ExperimentationOptions branch from c3e743b to ea6a640 Compare August 30, 2021 19:08
@tmat tmat force-pushed the ExperimentationOptions branch from 3a176a6 to 3125c4c Compare August 30, 2021 21:48
@tmat tmat merged commit ac39ffa into dotnet:main Sep 1, 2021
@ghost ghost added this to the Next milestone Sep 1, 2021
@tmat tmat deleted the ExperimentationOptions branch September 1, 2021 00:00
@Cosifne Cosifne modified the milestones: Next, 17.0.P5 Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge ExperimentationService into OptionService

6 participants