DYN-6030 Log Heartbeat events to log active/idle states #14684

zeusongit · 2023-12-04T21:50:06Z

Purpose

https://jira.autodesk.com/browse/DYN-6030

Add heartbeat ADP API to log active/idle user state.
This PR build will fail since we need the analytics PR first:https://git.autodesk.com/Dynamo/Analytics.NET/pull/70

Note: The Analytics method to track active/idle status will not trigger the API at each call, 
instead it will send out one call for every 4 minutes for each user and machine type activity. 
For example, if the method gets called 100 times in 8 minutes, then it will only trigger the 
HeartBeat API twice. This is to avoid sending out too many calls as the API expects a call 
every 5 minutes, to mark the user/machine active.

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
This PR contains no files larger than 50 MB

Release Notes

Add active/idle state tracking for users

Reviewers

mjkkirschner · 2023-12-05T23:34:10Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

+            {
+                serviceInitialized.Wait();
+
+                lock (trackEventLockObj)


hmm

@pinzart90 @BogdanZavu - What is this lock protecting?
ReportingAnalytics? Is that really going to be changing?

Because this task gets run when a user clicks the left mouse button it seems we're going to kick off a bunch of tasks that attempt to acquire this lock and wait for each other until these API calls return... is the lock really needed in this case?

Do we know how long these API calls take @zeusongit ?

In debug build, it takes less than 1000 ticks (on average). First call was 3000, all proceeding calls were less than 500.

ReportingAnalytics? Is that really going to be changing? - mostlikely not but it is safer imo to have it like this. It does not buy us anything if it is outside, it will only exposes us to risk now and in the future if the implementation changes.

The lock is protecting from concurrent access into ADP api. In theory that is noted as being thread safe but there were reports with relatively simple use cases where surprisingly it is not. So to be on the safe side we have this lock.

pinzart90 · 2023-12-06T01:37:04Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using Autodesk.ADPDesktopSDK;


Where is this namespace coming from?

Was coming from AdpSDKCSharpWrapper.dll

If we are ok with bleeding AdpSDKCSharpWrapper types directly in Dynamo, then makes me wonder why we need an entire Analytics project in between Dynamo and AdpSDKCSharpWrapper. Maybe it made sense when we had multiple analytics providers...but now.
I think it would make it somewhat simpler to consume if we referenced the binary directly in Dynamo and gave up on all the complicated logic of the Analytics.Core. Of course AdpSDKCSharpWrapper is an internal nuget package, so we would have to handle its distribution directly in Dynamo.

Of course if we think we might get into a situation with multiple analytics providers again, then I suggest we keep layer of separation intact

FYI @QilongTang @mjkkirschner

I see what was going on. When consuming the Analytics nuget pacakge, we now have access to the AdpSDKCSharpWrapper.dll at compile time
So any dev writing code could potentially access types directly from that assembly
There are ways to avoid this, like removing the adpsdk binrary from libs, and adding a build.props with a copy target.

BogdanZavu · 2023-12-06T18:22:46Z

Just curious overall why do we need to track this ? What are we looking for or what decision can we made based on this data to improve Dynamo ?

src/DynamoCoreWpf/ViewModels/Core/StateMachine.cs

mjkkirschner · 2023-12-07T20:55:58Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

        public virtual IAnalyticsSession Session { get; private set; }

+        private DateTime LastMachineHBLogTime;
+        private DateTime LastUserHBLogTime;


since I don't see these fields set anywhere initially, won't they always be null and no events will ever be sent?

DateTime types are initiated with a default value just like booleans

ah it's a struct

mjkkirschner · 2023-12-07T20:56:56Z

src/NodeServices/IAnalyticsClient.cs

        Export
    }

+    public enum HeartBeatType


does this need to be public?

I guess it does if the method is public?

mjkkirschner · 2023-12-07T20:58:44Z

Just curious overall why do we need to track this ? What are we looking for or what decision can we made based on this data to improve Dynamo ?

I'm curious about the premise of using this data as well because I feel that impacts what "active" means.
Is a graph executing for 30 minutes active? the user will not be clicking during this time.

zeusongit · 2023-12-07T21:24:34Z

Just curious overall why do we need to track this ? What are we looking for or what decision can we made based on this data to improve Dynamo ?

I'm curious about the premise of using this data as well because I feel that impacts what "active" means. Is a graph executing for 30 minutes active? the user will not be clicking during this time.

@mjkkirschner There is a separate identifier for when the machine is active/idle, it will log exactly the cases you mentioned, i.e the machine is active but the user is or maybe idle.
We do not have a comprehensive implementation of this at the moment, but it should be the next step, that is when a graph is running for like 30 minutes, we should send a heart beat every 5 minutes to indicate that the machine is active, currently we are sending one beat per machine execution, so if the execution takes longer than 5 minutes we may still have a single heartbeat log indicating that the machine was active for 5 minutes, but we will improve that.

zeusongit · 2023-12-07T21:25:47Z

Re-triggered the Analytics build, after the nugets are generated build should pass.

github-actions · 2023-12-07T23:49:50Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-07T23:51:38Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

pinzart90 · 2023-12-08T14:48:01Z

src/DynamoCore/Logging/DynamoAnalyticsClient.cs

+        /// as the API expects a call every 5 minutes, to mark the user/machine active.
+        /// </summary>
+        /// <param name="activityType">Value must be either Machine or User. If no value is provided the API will default to user activity type.</param>
+        public void TrackActivityStatus(string activityType)


do we expect this API to be used by externally ?
shouldn;t this be internal to dynamo ?

It is declared in IAnalyticsClient interface
https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/types/interfaces#:~:text=To%20implement%20an%20interface%20member

mjkkirschner · 2023-12-12T21:03:41Z

@zeusongit I think you should add a test to ensure your code does what you think it does and that the behavior does not change.

zeusongit · 2023-12-12T22:47:18Z

@zeusongit I think you should add a test to ensure your code does what you think it does and that the behavior does not change.

@mjkkirschner I can add track activity status to the current analytics test, but I don't think the current Moq implementation of the Analytics will be able to completely test the functionality of this method, for e.g how to test that it will send off 1 log for each activity type in a 4 minute period.

…tbeat-log

mjkkirschner · 2023-12-12T23:07:24Z

@zeusongit I think you should add a test to ensure your code does what you think it does and that the behavior does not change.

@mjkkirschner I can add track activity status to the current analytics test, but I don't think the current Moq implementation of the Analytics will be able to completely test the functionality of this method, for e.g how to test that it will send off 1 log for each activity type in a 4 minute period.

you should make your code testable - you can have the timeout be a parameter and set it lower for the test.

github-actions · 2023-12-12T23:09:56Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-12T23:14:56Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-12T23:18:09Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-12T23:19:36Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

zeusongit · 2023-12-13T17:09:21Z

@zeusongit I think you should add a test to ensure your code does what you think it does and that the behavior does not change.

@mjkkirschner I can add track activity status to the current analytics test, but I don't think the current Moq implementation of the Analytics will be able to completely test the functionality of this method, for e.g how to test that it will send off 1 log for each activity type in a 4 minute period.

you should make your code testable - you can have the timeout be a parameter and set it lower for the test.

@mjkkirschner parametrization is not a problem here, the problem is to track if the ADP service actually recorded the log, verifying the Moq instance for invocation will not record the desired result. After discussion with the team we have decided to go forward with the current state, and file a separate task to improve the machine active/idle time tracking in the future release.

* Fix PostDiff job * log heartbeat * Optimize logging and add mouse and key press events log * Update DynamoAnalyticsClient.cs * add machine tracking status * comments * Update Analytics nuget * add to test

zeusongit added 23 commits August 11, 2023 10:14

Fix PostDiff job

56ba101

Merge branch 'master' of https://github.com/zeusongit/Dynamo

dc250de

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

1fecd29

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

90a6c41

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

64c202b

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

d6ac751

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

255feeb

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

4e55f7f

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

1886669

Merge branch 'master' of https://github.com/zeusongit/Dynamo

8ac6b74

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

41c461c

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

f9cf237

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

a0adaef

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

2ab3d49

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

558df01

Merge branch 'master' of https://github.com/zeusongit/Dynamo

efdb82b

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

9b712f2

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

7c2dc1e

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

83bcb52

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

30b861d

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

71d23c2

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

cd1c4f9

log heartbeat

5135512

mjkkirschner reviewed Dec 5, 2023

View reviewed changes

Optimize logging and add mouse and key press events log

3cfa16c

pinzart90 reviewed Dec 6, 2023

View reviewed changes

Update DynamoAnalyticsClient.cs

238cdc1

zeusongit marked this pull request as ready for review December 6, 2023 17:45

zeusongit requested a review from QilongTang December 6, 2023 17:46

QilongTang added this to the 3.0 milestone Dec 7, 2023

QilongTang reviewed Dec 7, 2023

View reviewed changes

src/DynamoCoreWpf/ViewModels/Core/StateMachine.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 7, 2023

View reviewed changes

add machine tracking status

1de9921

zeusongit added 3 commits December 7, 2023 16:31

comments

46a1a0a

Update Analytics nuget

26df8d2

Merge branch 'master' into heartbeat-log

f91cfbf

pinzart90 reviewed Dec 8, 2023

View reviewed changes

zeusongit requested review from mjkkirschner and pinzart90 December 12, 2023 16:39

zeusongit added 2 commits December 12, 2023 17:47

add to test

03ec383

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into hear…

4915414

…tbeat-log

QilongTang approved these changes Dec 13, 2023

View reviewed changes

zeusongit merged commit cfad5a1 into DynamoDS:master Dec 13, 2023

zeusongit mentioned this pull request Dec 13, 2023

Cherrypick Heartbeat Log and NodeAutocomplete crash fix #14756

Merged

9 tasks

DYN-6030 Log Heartbeat events to log active/idle states #14684

DYN-6030 Log Heartbeat events to log active/idle states #14684

Uh oh!

Conversation

zeusongit commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Release Notes

Reviewers

Uh oh!

mjkkirschner Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusongit Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BogdanZavu commented Dec 6, 2023

Uh oh!

Uh oh!

mjkkirschner Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 7, 2023

Uh oh!

zeusongit commented Dec 7, 2023

Uh oh!

zeusongit commented Dec 7, 2023

Uh oh!

github-actions bot commented Dec 7, 2023

UI Smoke Tests

Uh oh!

github-actions bot commented Dec 7, 2023

UI Smoke Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusongit Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 12, 2023

Uh oh!

zeusongit commented Dec 12, 2023

Uh oh!

mjkkirschner commented Dec 12, 2023

Uh oh!

github-actions bot commented Dec 12, 2023

UI Smoke Tests

Uh oh!

github-actions bot commented Dec 12, 2023

UI Smoke Tests

Uh oh!

github-actions bot commented Dec 12, 2023

UI Smoke Tests

Uh oh!

github-actions bot commented Dec 12, 2023

zeusongit commented Dec 4, 2023 •

edited

Loading

mjkkirschner Dec 5, 2023 •

edited

Loading

zeusongit Dec 5, 2023 •

edited

Loading

mjkkirschner Dec 7, 2023 •

edited

Loading

zeusongit Dec 11, 2023 •

edited

Loading