Add new configuration option to disable ADP #11513

mjkkirschner · 2021-02-22T23:04:45Z

Purpose

Adds a configuration option for clients to use to disable ADP for dynamo and any other API access within the process.
Does some simple checks for invalid configuration options, as well as asserting that the disable call actually worked.
updates ADPSDK to 3.x and updates analytics.net to 2.11.1 binaries.

TODO:

~~- [ ] add test~~ I found it's difficult to add a test here without refactoring the analyticsServices classes because static types cannot be mocked, and during testing we may not actually have the ADP core binaries around. Because we have tests in the ADP project, and the test here would basically just be asserting the configuration bool is set correctly, I'm thinking it's not worth it - open to suggestions though.

adpwrapper binary must be updated in this pr

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

use it to call disable raise warnings or throw if we fail to disable adp

src/DynamoCore/Logging/AnalyticsService.cs

…method more logging at startup update adp/analytics binaries

mjkkirschner · 2021-02-23T21:31:38Z

src/DynamoCore/Logging/AnalyticsService.cs

+                }
+            }
+
+            if (isHeadless)


I modified how late we bail out if isHeadless is true because some clients need to disable ADP and still use Dynamo as a headless runner.

Can you remind me, the isHeadless is set to true only when using command line, right? For Dynamo Player and GD, analytics is on because we are using Dynamo as a server.

I believe player does not use headless, I am not sure about GD.

mjkkirschner · 2021-02-23T21:35:04Z

@pinzart90 @aparajit-pratap - please see my note above on difficulties testing this - one approach I do think would work is to create an internal enum to record the state of the disable call - something like:

enum disableResult { success, failure, not_attempted} - then check this from a test, the issue is that during a test we can't guarantee we will have access to adp core, but we might still want to make sure the disable call was attempted.

what do you think?

src/DynamoCore/Logging/AnalyticsService.cs

pinzart90

See comment ..then looks good

QilongTang · 2021-02-25T16:09:14Z

src/DynamoCore/Models/DynamoModel.cs

+
+            if (areAnalyticsDisabledFromConfig && disableADP)
+            {
+                throw new ConfigurationException("Incompatible configuration: could not start Dynamo with both [Analytics disabled] and [ADP disabled] config options enabled");


QilongTang · 2021-02-25T16:09:55Z

src/DynamoCore/Models/DynamoModel.cs

+
+            if(IsTestMode && disableADP)
+            {
+                this.Logger.Log("Incompatible configuration: [IsTestMode] and [ADP disabled] ");


what does this case mean?

this is the case where testmode is active (during most tests) and some test might try to disable ADP - but we can't guarantee that we have access to the core sdks in different testing scenarios, and I could not find a nice way to mock this dependency, so thought it would be valuable to log the case, because it's likely adp is not really disabled.

I forgot the most important part!!
Also - we don't initialize analytics in test mode, so downstream of this in AnalyticsService.Start() we break out if we're in test mode and don't even try to call disable.

That is exactly what I was going to mention. We always bail out during test mode so I dont think ADP will be loaded at all. https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Models/DynamoModel.cs#L1429 Do we really need this warning?

we don't need it - but I think it might help avoid wasted time. It would be nice if Dynamo provided feedback on incompatible options since to a client it's not obvious what headless and testmode actually control.

Thanks @mjkkirschner If that's the case, it seems this warning can be moved into beginning of AnalyticsService.Start(). What do you think?

yes, can do.

QilongTang · 2021-02-25T16:39:57Z

Thank you. Looks good to me

* add new configuration option use it to call disable raise warnings or throw if we fail to disable adp * modify analytics startup to check testmode and headless inside start method more logging at startup update adp/analytics binaries * review comments * review comments, unify name * move test mode error log to analytics start Co-authored-by: michael kirschner <[email protected]>

* add new configuration option use it to call disable raise warnings or throw if we fail to disable adp * modify analytics startup to check testmode and headless inside start method more logging at startup update adp/analytics binaries * review comments * review comments, unify name * move test mode error log to analytics start Co-authored-by: michael kirschner <[email protected]> Co-authored-by: michael kirschner <[email protected]>

aparajit-pratap · 2021-02-25T19:37:17Z

Why are these empty files added:
extern/Analytics.NET/AdpSDKCSharpWrapper.dll, ...

mjkkirschner · 2021-02-25T20:34:05Z

@aparajit-pratap they are not empty or added - they are updated binaries:

new adpsdk
our updated wrapper

check the history of the files, it shows "empty file" since it was added, not sure why.

add new configuration option

b02c659

use it to call disable raise warnings or throw if we fail to disable adp

mjkkirschner added the WIP label Feb 22, 2021

mjkkirschner commented Feb 22, 2021

View reviewed changes

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

modify analytics startup to check testmode and headless inside start …

6421466

…method more logging at startup update adp/analytics binaries

mjkkirschner changed the title ~~WIP add new configuration option to disable ADP~~ Add new configuration option to disable ADP Feb 23, 2021

mjkkirschner requested review from pinzart, aparajit-pratap and pinzart90 February 23, 2021 21:25

mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Feb 23, 2021

mjkkirschner commented Feb 23, 2021

View reviewed changes

pinzart90 reviewed Feb 23, 2021

View reviewed changes

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

review comments

6094b0f

pinzart90 reviewed Feb 25, 2021

View reviewed changes

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

pinzart90 approved these changes Feb 25, 2021

View reviewed changes

review comments, unify name

0d6bf19

pinzart90 approved these changes Feb 25, 2021

View reviewed changes

QilongTang reviewed Feb 25, 2021

View reviewed changes

move test mode error log to analytics start

f4a85ed

mjkkirschner merged commit 2435479 into DynamoDS:master Feb 25, 2021

mjkkirschner mentioned this pull request Feb 25, 2021

Add new configuration option to disable ADP (#11513) cp to 2.11 #11518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new configuration option to disable ADP #11513

Add new configuration option to disable ADP #11513

mjkkirschner commented Feb 22, 2021 •

edited

Loading

mjkkirschner Feb 23, 2021

QilongTang Feb 25, 2021

mjkkirschner Feb 25, 2021

mjkkirschner commented Feb 23, 2021 •

edited

Loading

pinzart90 left a comment

QilongTang Feb 25, 2021

QilongTang Feb 25, 2021

mjkkirschner Feb 25, 2021 •

edited

Loading

QilongTang Feb 25, 2021 •

edited

Loading

mjkkirschner Feb 25, 2021

QilongTang Feb 25, 2021 •

edited

Loading

mjkkirschner Feb 25, 2021

QilongTang commented Feb 25, 2021

aparajit-pratap commented Feb 25, 2021

mjkkirschner commented Feb 25, 2021

Add new configuration option to disable ADP #11513

Add new configuration option to disable ADP #11513

Conversation

mjkkirschner commented Feb 22, 2021 • edited Loading

Purpose

TODO:

Declarations

mjkkirschner Feb 23, 2021

Choose a reason for hiding this comment

QilongTang Feb 25, 2021

Choose a reason for hiding this comment

mjkkirschner Feb 25, 2021

Choose a reason for hiding this comment

mjkkirschner commented Feb 23, 2021 • edited Loading

pinzart90 left a comment

Choose a reason for hiding this comment

QilongTang Feb 25, 2021

Choose a reason for hiding this comment

QilongTang Feb 25, 2021

Choose a reason for hiding this comment

mjkkirschner Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

QilongTang Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Feb 25, 2021

Choose a reason for hiding this comment

QilongTang Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Feb 25, 2021

Choose a reason for hiding this comment

QilongTang commented Feb 25, 2021

aparajit-pratap commented Feb 25, 2021

mjkkirschner commented Feb 25, 2021

mjkkirschner commented Feb 22, 2021 •

edited

Loading

mjkkirschner commented Feb 23, 2021 •

edited

Loading

mjkkirschner Feb 25, 2021 •

edited

Loading

QilongTang Feb 25, 2021 •

edited

Loading

QilongTang Feb 25, 2021 •

edited

Loading