Do not aggregate node execution analytics anymore #11356

QilongTang · 2020-12-11T04:05:40Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Example Graph

Previously, node execution analytics data was aggregated with comma, e.g.

Code Block, String from Array, Watch

This makes data much less straight forward on backend with all kinds of combos of modified nodes during execution.

After this PR, each executed node will be tracked individually and no longer using timed event. Here are the cases:

Adding a new node, ModifiedNodes only contains the new node
Deleting a node, depending on which node we are deleting, ModifiedNodes only contains the downstream subgraph
Moving a node, no graph run will be triggered
Undo moving a node, ModifiedNodes only contains the node moved and downstream subgraph. This is a bug tracked.
Modifying a node, ModifiedNodes only contains the node modified and downstream subgraph

Individual node execution event will be tracked as DYN.NodeOperations.Run instead of the previous aggregated DYN.Performance.UpdateGraphAsyncTask. See related PR: https://git.autodesk.com/Dynamo/Analytics.NET/pull/26

About performance impact, I used DotTrace by Jetbrains to profile with a file with ~760 nodes, I did file opening and some other actions above which would trigger rerun with a lot of nodes executed. The profiling results did not mark any hot spots on analytics call but marked Python node view customization as hot spots. Since the analytics call do not involve system I/O by default which matches my expectation, I think adding this loop should be safe. FYI: @mmisol

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

Reviewers

@DynamoDS/dynamo

FYIs

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

QilongTang · 2020-12-11T04:50:25Z

src/DynamoCore/Models/DynamoModel.cs

+                                        Categories.Performance,
+                                        e.Task.GetType().Name,
+                                        executionTimeSpan, node.GetOriginalName());
+                                }


@mmisol @aparajit-pratap Thank you for your input. I would like to take more time to think through how we track real changes to nodes executed.

From what I can tell, adding/deleting/undo moving/modifying a node will all trigger execution so even a node showing as popularly executed, it might be just users modify them regularly.. Like Code Block node

Certain cases, undo/redo may trigger multiple runs..

Also @mmisol I dont think I can only look for ModifiedNodes.count = 1 case because when upstream node modified, we usually make downstream nodes dirty as well in Dynamo. e.g. with the following example, if user touches CBN, the all three nodes will be in ModifiedNodes set during execution.. Maybe the ModifiedNodes name itself is misleading..

maybe a stupid question... doesn't this send n http requests for each graph run? Doesn't that seem potentially slow with large graphs?

@mjkkirschner ADPSDK will handle the requests with optimization on their end. From my dottrace profiling, this do not seem impact Dynamo performance that much. I have a screen shot of that in PR description using DotTrace. Also, since delta computation works, we are not always sending n requests for each run, but only the downstream subset of the graph.

QilongTang · 2020-12-11T23:15:16Z

I looked at this change with @aparajit-pratap together and found it works great without Tuneup view extension, I will try to do some profiling to estimate the performance impact.

zeusongit

LGTM

mmisol

Just a couple of questions on my end. Maybe my comment in #11355 was misunderstood because I didn't say I had concerns about performance.

mmisol · 2020-12-17T16:15:19Z

src/DynamoCore/Models/DynamoModel.cs

+                                        Actions.Run, 
+                                        Categories.NodeOperations,
+                                        node.GetOriginalName());
+                                }


I see this is not a timed event anymore, which makes sense to me given we cannot separate the individual execution time for each node. Just checking if this was an intended change since it wasn't announced in the description @QilongTang

Thanks，yes it is intended and let me update description

mmisol · 2020-12-17T16:21:33Z

src/DynamoCore/Models/DynamoModel.cs

+                                foreach (var node in updateTask.ModifiedNodes)
+                                {
+                                    // Starting from Dynamo 2.11.0, tracking node execution as generic event so
+                                    // it is distinguished with the previous aggregated performance event


Regarding this comment, I would abstain from including Starting from Dynamo 2.11.0,. Since we are using git it's easy to traceback changes without having to add timestamps or version marks.

QilongTang · 2020-12-17T16:34:07Z

@mmisol Thanks for review, Addressed the comments, updated PR description and merging.

* Do not aggregate node execution analytics anymore * Update to use new event combo * Update Comments

* Dock and Undock view extension with Dynamic extension ID as edge case (#11394) * Initial Commit with settings null check * Add unit test * Add view extension loading warning * Update message * Initial Commit (#11366) * Do not aggregate node execution analytics anymore (#11356) * Do not aggregate node execution analytics anymore * Update to use new event combo * Update Comments * Make IntegerSlider the only AKA for migration (#11360) There were two classes that were declaring an 'AlsoKnownAs' attribute for 'DSCoreNodesUI.Input.IntegerSlider' in 'CoreNodeModels.Input': 'IntegerSlider' and 'IntegerSlider64Bit'. While the latter obsoletes the former, it is not possible to migrate to it from XML, so only 'IntegerSlider' should continue declaring an 'AlsoKnownAs' attribute. Note that currently both classes are trying to be registered, which results in only one of them being registered but not really making any guarantees of which one it is. This also generates an exception catched internally but shown in the console. * Revised icons for AnyTrue and AnyFalse nodes (#11361) * Implement ToString for DS Dictionary (#11358) This is done to provide meaningful output when a dictionary is provided to nodes like 'String from Array' and 'String from Object'. These nodes call built-in DS functions that end up calling ToString on the dictionary, which until now used the default implementation inherited from Object which just returned the class name. Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: Laurence Elsdon <[email protected]>

Do not aggregate node execution analytics anymore

786593d

QilongTang commented Dec 11, 2020

View reviewed changes

QilongTang marked this pull request as ready for review December 14, 2020 17:12

Update to use new event combo

06a351b

QilongTang requested review from aparajit-pratap, mmisol and pinzart December 16, 2020 05:45

aparajit-pratap approved these changes Dec 16, 2020

View reviewed changes

zeusongit approved these changes Dec 16, 2020

View reviewed changes

mmisol reviewed Dec 17, 2020

View reviewed changes

Update Comments

77fa808

QilongTang merged commit d812d26 into master Dec 17, 2020

QilongTang deleted the ExecutedNodesAnalytics branch December 17, 2020 16:34

QilongTang added a commit that referenced this pull request Jan 27, 2021

Do not aggregate node execution analytics anymore (#11356)

95ec7b2

* Do not aggregate node execution analytics anymore * Update to use new event combo * Update Comments

QilongTang mentioned this pull request Jan 27, 2021

Cherry picks for RC2.10.1 #11436

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not aggregate node execution analytics anymore #11356

Do not aggregate node execution analytics anymore #11356

QilongTang commented Dec 11, 2020 •

edited

Loading

QilongTang Dec 11, 2020 •

edited

Loading

QilongTang Dec 11, 2020

QilongTang Dec 11, 2020

mjkkirschner Dec 16, 2020

QilongTang Dec 16, 2020 •

edited

Loading

QilongTang commented Dec 11, 2020

zeusongit left a comment

mmisol left a comment

mmisol Dec 17, 2020

QilongTang Dec 17, 2020

mmisol Dec 17, 2020 •

edited

Loading

QilongTang commented Dec 17, 2020

Do not aggregate node execution analytics anymore #11356

Do not aggregate node execution analytics anymore #11356

Conversation

QilongTang commented Dec 11, 2020 • edited Loading

Purpose

Declarations

Reviewers

FYIs

QilongTang Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

QilongTang Dec 11, 2020

Choose a reason for hiding this comment

QilongTang Dec 11, 2020

Choose a reason for hiding this comment

mjkkirschner Dec 16, 2020

Choose a reason for hiding this comment

QilongTang Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

QilongTang commented Dec 11, 2020

zeusongit left a comment

Choose a reason for hiding this comment

mmisol left a comment

Choose a reason for hiding this comment

mmisol Dec 17, 2020

Choose a reason for hiding this comment

QilongTang Dec 17, 2020

Choose a reason for hiding this comment

mmisol Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

QilongTang commented Dec 17, 2020

QilongTang commented Dec 11, 2020 •

edited

Loading

QilongTang Dec 11, 2020 •

edited

Loading

QilongTang Dec 16, 2020 •

edited

Loading

mmisol Dec 17, 2020 •

edited

Loading