WIP Start implementing feature flags for toy feature by mjkkirschner · Pull Request #12790 · DynamoDS/Dynamo

mjkkirschner · 2022-04-08T20:10:07Z

Purpose

This PR onboards Dynamo onto launch darkly, it controls a popup at startup. The feature flag is enabled for debug builds, and disabled for production (release) builds.

PR does the following:

adds a new project DynamoFeatureFlags that references the newest launch darkly client side dot net sdk nuget package.
~~updates compiler unsafe to avoid conflict at build time~~ ... latest changes revert this as LD is resolving the same version with Tibi's suggestion.
Starts the launch darkly client if - analytics is not disabled by the configuration file, or the DisableAnalytics flag - otherwise LD will start, so it's not controllable via any UI setting, only by commandline, config file etc.
We use the stable instrumentation id that is generated for analytics, if this cannot be created or loaded, we use a shared key. Shared key will also be used during testing etc.
All users are logged as anonymous, which means that we cannot see them on the LD dashboard, but we can do targeted role outs / AB testing as long as the shared key is not used.
mobile sdk keys are embedded into the config file for the new project - according to LD docs, this is safe with no risk, these keys are not full sdk keys, they're more like a url for our dev/prod environments.
To avoid a slow startup the LD client is started async, so you can't immediately check feature flags unless you want to block - I have not implemented that, but it should be possible to add to the CheckFeatureFlag<T> method.

New Binaries:

LaunchDarkly.ClientSdk.dll - manual netstandard2.0
LaunchDarkly.CommonSdk.dll - manual netstandard2.0
LaunchDarkly.EventSource.dll - manual netstandard2.0
LaunchDarkly.InternalSdk.dll - manual netstandard2.0
LaunchDarkly.JsonStream.dll - manual netstandard2.0
LaunchDarkly.Logging.dll - manual netstandard2.0
~~Microsoft.Bcl.AsyncInterfaces.dll~~
System.Buffers.dll - automatic net461
System.Memory.dll - automatic net461
System.Numerics.Vectors.dll - automatic net461
System.Runtime.CompilerServices.Unsafe.dll - automatic net461
~~System.Text.Encodings.Web.dll~~
~~System.Text.Json.dll~~
~~System.Threading.Tasks.Extensions.dll~~
~~System.ValueTuple.dll~~

package refs for the DynamoFeatureFlags Project:

'DynamoFeatureFlags' has the following package references
   [netstandard2.0]: 
   Top-level Package                Requested   Resolved
   > LaunchDarkly.ClientSdk         2.0.1       2.0.1   
   > NETStandard.Library      (A)   [2.0.3, )   2.0.3   

   Transitive Package                            Resolved
   > LaunchDarkly.CommonSdk                      5.5.0   
   > LaunchDarkly.EventSource                    4.1.3   
   > LaunchDarkly.InternalSdk                    2.3.2   
   > LaunchDarkly.JsonStream                     1.0.3   
   > LaunchDarkly.Logging                        1.0.1   
   > Microsoft.NETCore.Platforms                 1.1.0   
   > System.Buffers                              4.5.1   
   > System.Collections.Immutable                1.7.1   
   > System.Memory                               4.5.4   
   > System.Numerics.Vectors                     4.5.0   
   > System.Runtime.CompilerServices.Unsafe      4.5.3

TODO:

tests
in a host like Revit where Dynamo is stopped and restarted, need to test if disposal/restarting of the LD client is smooth.
if LD deps are missing, ensure that DynamoCore still starts fine.
satisfied with new binaries this PR pulls in.
compiler.unsafe version alignment?
reference to system.configuration.dll in new csproj looks very wrong. (hint path)
if the user is logged in, we might want to add more attributes like email - so we can target specific emails for some features - we should add the private attribute flag if so to avoid storing any PII in LD.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Release Notes

Add initial feature flags controls in Dynamo.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

instantiate feature flags manager in core test message box in view add feature flags man project with config file update compiler unsafe to v 5

wrap startup in try catch unsub logger handler add prod key use shared key add try catch and logging around LD api methods

src/DynamoFeatureFlags/DynamoFeatureFlags.csproj

pinzart90 · 2022-04-11T14:48:11Z

src/DynamoFeatureFlags/DynamoFeatureFlags.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="LaunchDarkly.ClientSdk" Version="2.0.1" />


are we OK with bringing in a bunch of extra dependencies to the dynamo build ?
(I know we tried to avoid this in the past)

I'm not sure, do you think it's worth exploring wrapping up the launch darkly client in a separate process the same way we do for the markdown2htmlCLI tool? This would isolate the dependencies from DynamoCore / the host process.

Performance might be a bit worse, but flags shouldn't be getting evaluated very frequently anyway.

From my point of view, LD in a separate process would make sense only if extra dll dependencies are deal breaker.
Some of the extra dependencies will be brought in by the move to dotnet6 anyway.

I did also see a more lightweight REST API client https://www.nuget.org/packages/LaunchDarkly.Api/
Not sure if this would a better alternative ...The API does look more difficult to use ...

mjkkirschner · 2022-04-12T04:28:52Z

I believe that api is a non starter in our case as it requires full sdk keys that must remain secret - ie it’s for a custom automation tool, not a client process - but I will double check. IMO we can start by referencing the ld client to keep things straight forward and then move to out of process if we run into conflicts in the future. It’s not clear how well features like local storage will work wrapped in another process- we might need to contend with other gotchas like that for conflicts that don’t exist.

…

On Apr 11, 2022, at 9:43 PM, pinzart90 ***@***.***> wrote: @pinzart90 commented on this pull request. In src/DynamoFeatureFlags/DynamoFeatureFlags.csproj: > @@ -0,0 +1,19 @@ +<Project Sdk="Microsoft.NET.Sdk"> + <ImportGroup Label="PropertySheets"> + <Import Project="$(SolutionDir)Config/CS_SDK.props" /> + </ImportGroup> + <PropertyGroup> + <TargetFramework>netstandard2.0</TargetFramework> + </PropertyGroup> + + <ItemGroup> + <PackageReference Include="LaunchDarkly.ClientSdk" Version="2.0.1" /> From my point of view, LD in a separate process would make sense only if extra dll dependencies are deal breaker. Some of the extra dependencies will be brought in by the move to dotnet6 anyway. I did also see a more lightweight REST API client https://www.nuget.org/packages/LaunchDarkly.Api/ Not sure if this would a better alternative ...The API does look more difficult to use ... — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

set owner

src/DynamoFeatureFlags/DynamoFeatureFlags.csproj

aparajit-pratap · 2022-04-17T22:21:49Z

src/DynamoFeatureFlags/FeatureFlagsManager.cs

+                object output = default(T);
+                switch (default(T))
+                {
+                    case bool _:


Are we testing for type here? If so, can we also use typeof as in:

switch (typeof(T)) { case typeof(bool): ... }

also fails on dotnet 6 unfortunately, mixing generic and switch case is not a great idea I guess.

mjkkirschner added 2 commits April 7, 2022 19:08

expose user id method to dynamocore

201792b

instantiate feature flags manager in core test message box in view add feature flags man project with config file update compiler unsafe to v 5

fix bug where user id was not captured

9eb65ab

wrap startup in try catch unsub logger handler add prod key use shared key add try catch and logging around LD api methods

mjkkirschner added the WIP label Apr 8, 2022

mjkkirschner commented Apr 8, 2022

View reviewed changes

src/DynamoFeatureFlags/DynamoFeatureFlags.csproj Outdated Show resolved Hide resolved

pinzart90 reviewed Apr 11, 2022

View reviewed changes

mjkkirschner added 4 commits April 12, 2022 13:19

switch to using xml to read config directly to fix build

ac36eb0

make internal

376ef6d

review suggestion

c82e987

reset to 4.5.3 :(

aef6e1e

set owner

pinzart90 reviewed Apr 13, 2022

View reviewed changes

src/DynamoFeatureFlags/DynamoFeatureFlags.csproj Show resolved Hide resolved

mjkkirschner mentioned this pull request Apr 14, 2022

wrap launch darkly in CLI to avoid bringing in new deps #12801

Merged

12 tasks

pinzart90 and others added 2 commits April 13, 2022 21:19

update dependencies

c6f8d4c

Update AnalyticsService.cs

e0f0518

aparajit-pratap reviewed Apr 17, 2022

View reviewed changes

mjkkirschner closed this Apr 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Start implementing feature flags for toy feature#12790

WIP Start implementing feature flags for toy feature#12790
mjkkirschner wants to merge 8 commits intoDynamoDS:masterfrom
mjkkirschner:featureflags

mjkkirschner commented Apr 8, 2022 •

edited by pinzart90

Loading

Uh oh!

Uh oh!

pinzart90 Apr 11, 2022

Uh oh!

mjkkirschner Apr 11, 2022

Uh oh!

pinzart90 Apr 12, 2022

Uh oh!

mjkkirschner commented Apr 12, 2022 via email

Uh oh!

Uh oh!

aparajit-pratap Apr 17, 2022

Uh oh!

mjkkirschner Apr 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mjkkirschner commented Apr 8, 2022 • edited by pinzart90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

New Binaries:

TODO:

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

Uh oh!

pinzart90 Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

pinzart90 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Apr 12, 2022 via email

Uh oh!

Uh oh!

aparajit-pratap Apr 17, 2022

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjkkirschner commented Apr 8, 2022 •

edited by pinzart90

Loading