Add ADP opt-in ui under DebugMode #10638

pinzart · 2020-05-11T16:13:45Z

Purpose

Added UI for ADP (replaces the Instrumentation consent UI)
Still fully under a debug mode.
Exposed the ADP opt-in/opt-out API in Preferences as a non-serializable property,
Google and ADP are handled separately.

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.

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

pinzart · 2020-05-11T16:33:26Z

ADP opt-in/opt-out values are common to all ADSK products (not per individual product). This means that Dynamo will not have exclusive control of ADP opt-in/opt-out.
This complicates how we can expose ADP optin/optout at API level. GoogleAnalytics is fully under Dynamo's control...and is exposed in Preferences (also serialized)...This might not make sense for ADP.
Maybe we can wrap both Google and ADP opt-in/opt-out under a Common Dynamo Analytics Toggle. This would ensure that ADP is under Dynamo full control. So something like this:
[X] Enable Analytics Reporting
--- [X] Enable Google Analytics
--- [X] Enable ADP Analytics

@QilongTang @mjkkirschner thoughts?

mjkkirschner · 2020-05-11T16:39:31Z

This needs to be reviewed carefully, I see you undid some recent changes by @QilongTang which stopped instrumentation and analytics from being started/loaded at all if disabled. These were put in to investigate http errors in specific proxy environments.

Was that needed?

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

src/DynamoCore/Configuration/PreferenceSettings.cs

pinzart · 2020-05-12T19:58:11Z

@mjkkirschner Regarding
I see you undid some recent changes by @QilongTang which stopped instrumentation and analytics from being started/loaded at all if disabled. These were put in to investigate http errors in specific proxy environments.
THat check did not make a lot of sense to me...unless it was as you said...for some edge case ...
I added the check back (with adp in the condition)

src/DynamoCore/Configuration/PreferenceSettings.cs

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

pinzart · 2020-05-12T20:03:38Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

            {
                //If not ReportingAnalytics, then set the idle time as infinite so idle state is not recorded.
-                Service.StartUp(product, new UserInfo(Session.UserId), preferences.IsAnalyticsReportingApproved ? TimeSpan.FromMinutes(30) : TimeSpan.MaxValue);


I removed the backup value TimeSpan.MaxValue since it seems it was never hit before...

@pinzart Yes, now I remember because we made the change that if GA is not agreed, we do not initialize the client at all. It seems this change will undo that previous change though. e.g. if the ADP is agreed on, the service will be started regardless

@QilongTang
ADP is agreed on, the service will be started regardless
That is correct. My intention was to make ADP and Google independent of each other. So if a user opts into ADP (but not Google) ...then Analytics service will be started (but only ADPTracker will process events). Similar if Google is opted in but ADP is not.

pinzart · 2020-05-12T20:04:56Z

I did not refactor the code to remove the Instrumentation option (for logging PII). That can be done in a separate PR

pinzart · 2020-05-12T20:05:35Z

In this PR, ADP analytics are still fully under a debug mode

pinzart · 2020-05-12T20:19:11Z

mjkkirschner · 2020-05-12T21:56:07Z

@QilongTang - I'm having trouble remembering the option added by Alias team, but we should make sure that this option to completely disable the UI for analytics (and also disable all analytics) continues to work.

mjkkirschner · 2020-05-12T22:09:45Z

@pinzart this is the functionality I mean:
#10217

pinzart · 2020-05-12T22:46:07Z

@mjkkirschner Regarding #10217
I just tested that..and it works as expected...Just hides the Agreement Menu Item and the First Run prompt window.

src/DynamoCore/Models/DynamoModel.cs

QilongTang

LGTM other than the comments we have not reached agreement.

mjkkirschner · 2020-05-14T19:51:24Z

@pinzart @QilongTang - I think the host name is unneeded - as confusing as it is the AppVersion contains the process name.

Dynamo/src/DynamoCore/Models/DynamoModel.cs

Line 331 in 31cb44e

return Process.GetCurrentProcess().ProcessName + "-"

QilongTang · 2020-05-14T20:22:26Z

@mjkkirschner hmm process name for DynamoForAdavanceSteel and DynamoForCivil3D is same, both Acad. This could be a chance for us to fix that. What do you think?

src/DynamoCore/Models/DynamoModel.cs

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

* separate functions for tracker registrations Use separate functions so that we do not load GA dll at runtime if it is not opted in..

pinzart · 2020-05-17T21:55:21Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

+            // If the Analytics Client was initialized, shut it down.
+            // Otherwise skip this step because it would cause an exception.
+            if (Service.IsInitialized)
+                Service.ShutDown();


Service.ShutDown() will clean all factories and all tracker related data

pinzart · 2020-05-17T21:57:21Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

@@ -30,20 +23,6 @@ public DynamoAnalyticsSession()

        public void Start(DynamoModel model)
        {
-            //Whether enabled or not, we still record the startup.


Moved this tracker registration/cleanup code in the Client instance. (to have access to client's data)

src/DynamoCore/Models/DynamoModel.cs

QilongTang

LGTM with one last comment

pinzart added 5 commits May 8, 2020 16:12

add ADP UI consent + first step to remove Instrumentation

3d3cb7d

update

e738bf7

update for debug mode

3298176

Update DynamoAnalyticsClient.cs

a169a3f

Update DynamoAnalyticsClient.cs

d0df4ad

pinzart added the DNM Do not merge. For PRs. label May 11, 2020

QilongTang reviewed May 11, 2020

View reviewed changes

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

update

3b90524

QilongTang reviewed May 12, 2020

View reviewed changes

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

pinzart added 2 commits May 12, 2020 13:56

update

c1a6dba

Update DynamoAnalyticsClient.cs

1c42eb1

QilongTang reviewed May 12, 2020

View reviewed changes

src/DynamoCore/Configuration/PreferenceSettings.cs Show resolved Hide resolved

pinzart commented May 12, 2020

View reviewed changes

src/DynamoCore/Configuration/PreferenceSettings.cs Show resolved Hide resolved

pinzart commented May 12, 2020

View reviewed changes

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

pinzart commented May 12, 2020

View reviewed changes

pinzart requested review from mjkkirschner and QilongTang May 12, 2020 20:05

pinzart added 2 commits May 12, 2020 16:45

update

73fa05c

update dlls

e35903e

QilongTang reviewed May 14, 2020

View reviewed changes

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

QilongTang reviewed May 14, 2020

View reviewed changes

add configurable Host Name to analytics

40a11da

add configuration based DisableAnalytics option

666cda0

pinzart commented May 14, 2020

View reviewed changes

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

QilongTang reviewed May 15, 2020

View reviewed changes

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

Load analytics factories only when user has opted in (#10665)

e628ee7

* separate functions for tracker registrations Use separate functions so that we do not load GA dll at runtime if it is not opted in..

pinzart commented May 17, 2020

View reviewed changes

pinzart added 3 commits May 17, 2020 17:59

Merge branch 'master' into adp_optin_ui

3c75668

use latest Analytics release

7f2e0e4

simplify adp optin interaction and add tests

d1062da

QilongTang reviewed May 18, 2020

View reviewed changes

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

QilongTang approved these changes May 18, 2020

View reviewed changes

QilongTang added LGTM Looks good to me and removed DNM Do not merge. For PRs. labels May 18, 2020

pinzart added 2 commits May 18, 2020 13:28

update from reviews

c6a0a7a

rename form ADP to Autodesk

b0a957a

pinzart merged commit 6320bd8 into master May 18, 2020

pinzart deleted the adp_optin_ui branch May 18, 2020 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ADP opt-in ui under DebugMode #10638

Add ADP opt-in ui under DebugMode #10638

pinzart commented May 11, 2020 •

edited

Loading

pinzart commented May 11, 2020 •

edited

Loading

mjkkirschner commented May 11, 2020 •

edited

Loading

pinzart commented May 12, 2020

pinzart May 12, 2020

QilongTang May 14, 2020

pinzart May 14, 2020

pinzart commented May 12, 2020

pinzart commented May 12, 2020

pinzart commented May 12, 2020

mjkkirschner commented May 12, 2020

mjkkirschner commented May 12, 2020

pinzart commented May 12, 2020

QilongTang left a comment

mjkkirschner commented May 14, 2020

QilongTang commented May 14, 2020

pinzart May 17, 2020

pinzart May 17, 2020

QilongTang left a comment

Add ADP opt-in ui under DebugMode #10638

Add ADP opt-in ui under DebugMode #10638

Conversation

pinzart commented May 11, 2020 • edited Loading

Purpose

Declarations

Reviewers

FYIs

pinzart commented May 11, 2020 • edited Loading

mjkkirschner commented May 11, 2020 • edited Loading

pinzart commented May 12, 2020

pinzart May 12, 2020

Choose a reason for hiding this comment

QilongTang May 14, 2020

Choose a reason for hiding this comment

pinzart May 14, 2020

Choose a reason for hiding this comment

pinzart commented May 12, 2020

pinzart commented May 12, 2020

pinzart commented May 12, 2020

mjkkirschner commented May 12, 2020

mjkkirschner commented May 12, 2020

pinzart commented May 12, 2020

QilongTang left a comment

Choose a reason for hiding this comment

mjkkirschner commented May 14, 2020

QilongTang commented May 14, 2020

pinzart May 17, 2020

Choose a reason for hiding this comment

pinzart May 17, 2020

Choose a reason for hiding this comment

QilongTang left a comment

Choose a reason for hiding this comment

pinzart commented May 11, 2020 •

edited

Loading

pinzart commented May 11, 2020 •

edited

Loading

mjkkirschner commented May 11, 2020 •

edited

Loading