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

mjkkirschner · 2022-04-14T00:20:17Z

Purpose

Updated 4/18 to short lived CLI process.

An alternative to:
#12790

in this PR instead of pulling LD sdk and its dependencies into the same process as Dynamo/Host - it's wrapped in a CLI following a very similar pattern to the markdown2html conversion tool.

How it works:

currently, the CLI tool is started at dynamo startup, initializes a connection with LD, asks LD for all the current user's known flags and their values, and will immediately send back a json object containing that data.

launch darkly initalized
2022-04-19 18:50:24Z : LD startup time: 897 
2022-04-19 18:50:26Z : <<<<<InitDone>>>>>
2022-04-19 18:50:27Z : feature flag exe starting
2022-04-19 18:50:30Z : <<<<<Sod>>>>>
2022-04-19 18:50:32Z : {"EasterEggIcon1":true,"EasterEggMessage1":"Hello fellow dynamo developers, have a great day."}
2022-04-19 18:50:34Z : <<<<<Eod>>>>>

I've added the start of data messages so I can safely log other information.

The process then shuts down.

If somehow Dynamo shuts down during the brief window where the flags were being retrieved, the CLI will also shut down if the host process is no longer running. It checks this every 10 seconds.

The CLI tool/wrapper are started on another thread using Task.Run to avoid blocking the startup of DynamoModel/DynamoView - this makes synchronizing when to check flags a bit complicated, so I've added an event that can be subscribed to - still, it's not foolproof as the event may have fired before you have a chance to subscribe to it, in this case the checkFlag API can be queried directly.

If we encounter a case in the future where we absolutely must control DynamoModel/DynamoView constructors via feature flags, then we can just remove the Task.Run and wait for the flags to be retrieved synchronously, and get rid of the event.

issues/TODO:

The LaunchDarklyClient.Sdk refuses to run when its been merged with ILMerge - I have not tried merging only some of the deps- but it happily works if all the deps are in a separate DynamoFeatureFlags folder.
Tests
Test manually in Revit to make sure no issues with Task.Run/sync context.
std.error is redirected and read async to avoid the buffer filling up in a long running process.

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 FeatureFlagsManager internal component for querying live feature flags.

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

set owner

move under tools dir refactor out small base class

pinzart90 · 2022-04-14T01:28:54Z

Maybe we can have a self destruct functionality inside the launchDarkly process. In case it becomes a zombie..
It could periodically check for the existence of the parent process (the dynamo process). If the dynamo process is not alive anymore...then self destruct.
We can pass the dynamo process id or name when the LD process is created...

mjkkirschner · 2022-04-14T01:30:38Z

Maybe we can have a self destruct functionality inside the launchDarkly process. In case it becomes a zombie..
It could periodically check for the existence of the parent process (the dynamo process). If the dynamo process is not alive anymore...then self destruct.
We can pass the dynamo process id or name when the LD process is created...

good idea!

I'm looking at the build failure. It's not obvious to me which path doesn't exist.

pinzart90 · 2022-04-14T01:42:02Z

Regarding deadlocks or lack of timeouts...
One other option (not saying it is better/faster) would be to make the separate process an http server

src/Tools/DynamoFeatureFlags/DynamoFeatureFlags.csproj

sm6srw · 2022-04-14T13:27:09Z

Is there any fundamental difference in this implementation compared to the markdown tool? I kind of remember that I had very few issues with zombies in the end with the markdown tool. One thing that could cause this is if the process creates threads that is not properly destroyed. Maybe something that happens in the LD SDK?

src/Tools/DynamoFeatureFlags/Program.cs

dont log messages unless under debug - TBD

…dcli

mjkkirschner · 2022-04-15T16:48:39Z

moved to draft while I try to reproduce the test hang locally.

mjkkirschner · 2022-04-16T19:52:21Z

this is no longer timing out the build machines - but there is a single failure, looking at it.

dispose process to release handles to os.

add test mode to cli which just returns mock data

aparajit-pratap · 2022-04-20T20:37:54Z

src/DynamoCore/Models/DynamoModel.cs

+
+                //run process startup/reading on another thread so we don't block dynamo startup.
+                //if we end up needing to control aspects of dynamo model or view startup that we can't make
+                //event based/async then just run this on main thread - ie get rid of the Task.Run()


By if we end up needing to control aspects of dynamo model or view startup, do you mean a case where we'll need to check for feature flags at some point before the model is instantiated?

yes, and where we cannot recreate or reset that aspect of the model or view.

src/DynamoCore/Models/DynamoModel.cs

src/DynamoUtilities/DynamoFeatureFlagsManager.cs

src/DynamoUtilities/Md2Html.cs

mjkkirschner · 2022-04-21T00:48:29Z

one failure

MESSAGE:
TearDown : System.InvalidOperationException : No process is associated with this object.
+++++++++++++++++++
STACK TRACE:
--TearDown
   at System.Diagnostics.Process.EnsureState(State state)
   at System.Diagnostics.Process.EnsureState(State state)
   at System.Diagnostics.Process.GetProcessHandle(Int32 access, Boolean throwIfExited)
   at System.Diagnostics.Process.Kill()
   at Dynamo.Utilities.CLIWrapper.KillProcess() in c:\WorkspaceDynamo\src\DynamoUtilities\Md2Html.cs:line 77
   at Dynamo.Models.DynamoModel.Dispose() in c:\WorkspaceDynamo\src\DynamoCore\Models\DynamoModel.cs:line 1271
   at Dynamo.Models.DynamoModel.ShutDownCore(Boolean shutdownHost) in c:\WorkspaceDynamo\src\DynamoCore\Models\DynamoModel.cs:line 455
   at Dynamo.Models.DynamoModel.ShutDown(Boolean shutdownHost) in c:\WorkspaceDynamo\src\DynamoCore\Models\DynamoModel.cs:line 420
   at Dynamo.ViewModels.DynamoViewModel.PerformShutdownSequence(ShutdownParams shutdownParams) in c:\WorkspaceDynamo\src\DynamoCoreWpf\ViewModels\Core\DynamoViewModel.cs:line 2930
   at DynamoCoreWpfTests.DynamoTestUIBase.Exit() in c:\WorkspaceDynamo\test\DynamoCoreWpfTests\DynamoTestUIBase.cs:line 124
``` looking at it.

mjkkirschner · 2022-04-21T16:43:46Z

Tests passed now, running master-15 serial tests as well. PTAL!

pinzart90 · 2022-04-21T18:39:46Z

src/Dynamo.All.sln

+		{94CF053D-BB66-4FC7-883B-48F072701BA9}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{94CF053D-BB66-4FC7-883B-48F072701BA9}.Release|Any CPU.Build.0 = Release|Any CPU
+		{94CF053D-BB66-4FC7-883B-48F072701BA9}.Release|x64.ActiveCfg = Release|Any CPU
+		{94CF053D-BB66-4FC7-883B-48F072701BA9}.Release|x64.Build.0 = Release|Any CPU


do we want to build this tool by default ....with the rest of the solution ?

We do so for the other CLI tools, why wouldn't we?

I guess if the tool does not directly depend on any other Dynamo projects it may have a slower dev cycle than the rest.
But I guess it is a small project (few cs files) so it does not really slow down the build process that much

src/DynamoCore/Logging/AnalyticsService.cs

src/DynamoCore/Models/DynamoModel.cs

pinzart90 · 2022-04-21T19:21:40Z

src/DynamoCore/Models/DynamoModel.cs

@@ -873,6 +905,28 @@ protected DynamoModel(IStartConfiguration config)
            DynamoReady(new ReadyParams(this));
        }

+        private void CheckFeatureFlagTest()
+        {
+            if (DynamoModel.FeatureFlags.CheckFeatureFlag<bool>("EasterEggIcon1", false))


we will need to store the possible feature flag names in some way (config file?) right ?

why? To what end?

I should mention that the keys, once created in launch darkly, are immutable. (besides being deletable)

In our discussions it seemed necessary to provide a way for feature flags to work offline (Like for ALIAS)
I may have misunderstood.
It is fine for now regardless.

src/Dynamo.All.sln

QilongTang · 2022-04-21T20:29:35Z

src/Tools/DynamoFeatureFlags/DynamoFeatureFlags.csproj

+  </ImportGroup>
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <OutputPath>$(OutputPath)\DynamoFeatureFlags\</OutputPath>


Remind me, the output will not be part of DynamoCore right?

It will be placed in Bin\AnyCPU\{config}\DynamoFeatureFlags subfolder so it will be part of DynamoCoreRuntime release zip and artifactory build - but it won't pollute the root bin folder. The way this tool currently works, even sandbox users will have access to feature flags.

pinzart90

Some comments.
LGTM

sm6srw

LGTM!

mjkkirschner added 7 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

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

move feature flags to a CLI

8899ef7

move under tools dir refactor out small base class

mjkkirschner requested review from sm6srw and pinzart90 April 14, 2022 00:20

mjkkirschner added the WIP label Apr 14, 2022

try to fix build on GitHub action runner

beffd90

pinzart90 reviewed Apr 14, 2022

View reviewed changes

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

simplify

0889459

sm6srw reviewed Apr 14, 2022

View reviewed changes

src/Tools/DynamoFeatureFlags/Program.cs Show resolved Hide resolved

mjkkirschner added 6 commits April 14, 2022 14:10

add watchdog

9cee007

dont log messages unless under debug - TBD

Merge branch 'ldcli' of https://github.com/mjkkirschner/Dynamo into l…

78afd4c

…dcli

remove todo

c0628bb

fix bug

c2dc6dc

get rid of reflection

b66d21c

better null checking when analytics is not disabled, but in testmode

e550089

mjkkirschner marked this pull request as draft April 15, 2022 16:48

mjkkirschner added 2 commits April 15, 2022 15:45

fix bug where cli crashed immediately

0916d94

check has exited before trying to read data

e80688a

mjkkirschner marked this pull request as ready for review April 16, 2022 19:52

mjkkirschner changed the title ~~WIP experiment to wrap launch darkly in CLI to avoid bringing in new deps~~ WIP wrap launch darkly in CLI to avoid bringing in new deps Apr 19, 2022

mjkkirschner added 8 commits April 19, 2022 17:48

don't start cli in test mode

9e428ea

dispose process to release handles to os.

reverse logic

5f05748

refactor shared constructor

09bfab5

add test mode to cli which just returns mock data

skip config loading if in test mode

95be7b5

add tests

963c6a9

only show one messagebox

02c06d4

missed commit

a63e24a

bool flag fix

ef4b5d4

aparajit-pratap reviewed Apr 20, 2022

View reviewed changes

src/DynamoCore/Models/DynamoModel.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 20, 2022

View reviewed changes

src/DynamoUtilities/DynamoFeatureFlagsManager.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 20, 2022

View reviewed changes

src/DynamoUtilities/Md2Html.cs Outdated Show resolved Hide resolved

review comments

67aa344

mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Apr 21, 2022

mjkkirschner changed the title ~~WIP wrap launch darkly in CLI to avoid bringing in new deps~~ wrap launch darkly in CLI to avoid bringing in new deps Apr 21, 2022

pinzart90 reviewed Apr 21, 2022

View reviewed changes

src/DynamoCore/Logging/AnalyticsService.cs Outdated Show resolved Hide resolved

pinzart90 reviewed Apr 21, 2022

View reviewed changes

src/DynamoCore/Models/DynamoModel.cs Outdated Show resolved Hide resolved

pinzart90 reviewed Apr 21, 2022

View reviewed changes

src/DynamoCore/Models/DynamoModel.cs Outdated Show resolved Hide resolved

pinzart90 reviewed Apr 21, 2022

View reviewed changes

QilongTang reviewed Apr 21, 2022

View reviewed changes

src/Dynamo.All.sln Outdated Show resolved Hide resolved

QilongTang reviewed Apr 21, 2022

View reviewed changes

review comments

44b23ae

pinzart90 approved these changes Apr 22, 2022

View reviewed changes

sm6srw approved these changes Apr 22, 2022

View reviewed changes

mjkkirschner merged commit a24d3c1 into DynamoDS:master Apr 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mjkkirschner commented Apr 14, 2022 •

edited

Loading

pinzart90 commented Apr 14, 2022 •

edited

Loading

mjkkirschner commented Apr 14, 2022

pinzart90 commented Apr 14, 2022

sm6srw commented Apr 14, 2022

mjkkirschner commented Apr 15, 2022

mjkkirschner commented Apr 16, 2022

aparajit-pratap Apr 20, 2022

mjkkirschner Apr 21, 2022

mjkkirschner commented Apr 21, 2022

mjkkirschner commented Apr 21, 2022

pinzart90 Apr 21, 2022

mjkkirschner Apr 21, 2022

pinzart90 Apr 22, 2022

pinzart90 Apr 21, 2022

mjkkirschner Apr 21, 2022 •

edited

Loading

mjkkirschner Apr 21, 2022 •

edited

Loading

pinzart90 Apr 22, 2022

QilongTang Apr 21, 2022

mjkkirschner Apr 21, 2022 •

edited

Loading

pinzart90 left a comment

sm6srw left a comment

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

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

Conversation

mjkkirschner commented Apr 14, 2022 • edited Loading

Purpose

Updated 4/18 to short lived CLI process.

How it works:

issues/TODO:

Declarations

Release Notes

Reviewers

FYIs

pinzart90 commented Apr 14, 2022 • edited Loading

mjkkirschner commented Apr 14, 2022

pinzart90 commented Apr 14, 2022

sm6srw commented Apr 14, 2022

mjkkirschner commented Apr 15, 2022

mjkkirschner commented Apr 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Apr 21, 2022

mjkkirschner commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

pinzart90 left a comment

Choose a reason for hiding this comment

sm6srw left a comment

Choose a reason for hiding this comment

mjkkirschner commented Apr 14, 2022 •

edited

Loading

pinzart90 commented Apr 14, 2022 •

edited

Loading

mjkkirschner Apr 21, 2022 •

edited

Loading

mjkkirschner Apr 21, 2022 •

edited

Loading

mjkkirschner Apr 21, 2022 •

edited

Loading