Convert Trace Serialization mechanism from SOAP to JSON #14530

saintentropy · 2023-10-29T05:43:27Z

Purpose

Experiment with alternative to the SOAP Formatter utilized in trace and Element binding data. This PR makes two primary changes related to trace:

Modify the public TraceSupport APIs to utilize string as the storage mechanism vs ISerializable. These are the primary mechanism for implementing Trace within ZT implementation.

ISerializable GetTraceData(string key) -> sting GetTraceData(string key)
SetTraceData(string key, ISerializable value) -> SetTraceData(string key, string value)

Modify the public Callsite APIs to return strings vs ISerializables. The following APIs are impacted. These are typically used by host integration for more advance manipulation of the trace data.

IList<ISerializable> GetOrphanedSerializables() -> IList<string> GetOrphanedSerializables()
IList<ISerializable> GetAllSerializablesFromSingleRunTraceData ->
IList<string> GetAllSerializablesFromSingleRunTraceData

Modify the public virtual method on DynamoModel. I believe that DynamoRevit is the only downstream host that utilizes this mechanism.

PostTraceReconciliation(Dictionary<Guid, List<ISerializable>> orphanedSerializables) ->
PostTraceReconciliation(Dictionary<Guid, List<string>> orphanedSerializables)

The second change to this PR is modification of the serialization mechanism used to store Element binding data in the file. Although these are public, I don't know of any other consumer. The primary change is modifying the SingleRunTraceData object main property public ISerializable Data -> public string Data. There are other methods on this class that change but all are ISerializable to string

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

Trace mechanism has dropped support for insecure SOAP deserialization, and now requires clients to use strings. Data is still intended to be opaque for users.

Reviewers

FYIs

@aparajit-pratap @mjkkirschner

src/Engine/ProtoCore/Lang/CallSite.cs

mjkkirschner · 2023-10-31T04:35:53Z

Two more thoughts

What happens if you load a graph with a lot of old format trace data- do we get tons of Json exceptions? Can we dump old data from v2 graphs instead?
I think there are some element binding performance graphs and tests floating around - I guess we’ll need up to update them as well.

src/DynamoCore/Models/DynamoModel.cs

test/Engine/FFITarget/MinimalTraceTargets.cs

src/Engine/ProtoCore/Lang/CallSite.cs

…eSerializationUpdate

fix compression fix 1 test

aparajit-pratap · 2023-11-16T18:11:13Z

src/Engine/ProtoCore/Lang/CallSite.cs

        /// </summary>
        /// <param name="serializedTraceData">The data to load</param>
        public void LoadSerializedDataIntoTraceCache(string serializedTraceData)
        {
-            if (serializedTraceData == null)
+            if (serializedTraceData == null || CheckIfTraceDataIsLegacySOAPFormat(serializedTraceData))


What happens if you skip the CheckIfTraceDataIsLegacy...?

then the try catch below will get hit and we'll end up doing a full base64 decode and catching an exception.

The thing we can do regarding your question about earlier catches and optimization of this check is handle something in here -> https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Graph/Workspaces/SerializationConverters.cs. This is the only place dynamo core encounters the Soap data. Maybe you can catch the old data there and then here the input string would be null. That might also help you with your task @aparajit-pratap . The nice thing is that the SerializationConverter is doing the whole Bindings section of the graph at once vs here were it is per callsite

In the D4C3D case we may just need to communicate the same pattern as the load data from dwg file.

This optimization can be done later too. This function could stay the same.

Actually even better, if you catch bad data at the file deserialization then even the GetOrphanedSerializablesAndClearHistoricalTraceData pass would be short circuited. It is looking at HomeWorkSpace.PreloadedTraceData which is set during deserialization.

Maybe it's just as simple as checking the DynamoVersion? Not full proof but in theory you should never have XML graph or a 2.x graph with good data.

src/Engine/ProtoCore/Lang/CallSite.cs

saintentropy · 2023-11-17T01:30:48Z

Hey @mjkkirschner I also forgot we should remove the Soap Formatter
Here -> https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/ProtoCore.csproj#L26

log shorten legacy check

saintentropy · 2023-11-17T01:32:57Z

Here -> https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/Lang/CallSite.cs#L9
and here -> https://github.com/DynamoDS/Dynamo/blob/master/doc/distrib/License.rtf#L1667

src/NodeServices/TraceSupport.cs

src/Engine/ProtoCore/Lang/TraceUtils.cs

remove use of ISerializable

github-actions · 2023-11-17T01:54:17Z

⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::1 file(s) have been deleted!
(Updated: 2023-11-17-02:07:25)

saintentropy · 2023-11-17T02:31:56Z

src/Engine/ProtoCore/Lang/CallSite.cs

-        /// <returns>A flat collection of ISerializable objects.</returns>
-        public static IList<ISerializable> GetAllSerializablesFromSingleRunTraceData(
+        /// <returns>A flat collection of strings.</returns>
+        public static IList<string> GetAllSerializablesFromSingleRunTraceData(


On thing we may be able to verify is does this need to be public. Internal it is only called one place GetOrphanedSerializablesAndClearHistoricalTraceData... Maybe in terms of quick validation of old data we can do it upstream and escape calling this one by one per node guid.

Hmmm. Looks like D4C3D calls it in their own version of GetAllSerializablesFromSingleRunTraceData.

So maybe stays public but we can also escape calls upstream and discuss with D4C3D the same optimization

aparajit-pratap · 2023-11-17T15:57:33Z

test/DynamoCoreTests/CallsiteTests.cs

+            AssertPreviewValue("da39dbe5f59649b18c2fb6ca54acba7b", 6);
+            AssertPreviewValue("2366239164a9441a8c4dcd981d9cf542", 7);
+            AssertPreviewValue("8cfce012280342f3bd688520d68a7f66", 10);
+            AssertPreviewValue("08448232ee094aad8280e9a99ed44f46", 11);


Sorry, what are these values that have changed?

handwritten values were replace with values generated by interacting with the graph instead. It's a bit harder to write values directly into trace by hand since the data is bas64 encoded and gzipped

mjkkirschner

self

First pass

6137115

saintentropy added the DNM Do not merge. For PRs. label Oct 29, 2023

missed file

8132647

mjkkirschner reviewed Oct 30, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Oct 30, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Show resolved Hide resolved

aparajit-pratap reviewed Nov 8, 2023

View reviewed changes

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

aparajit-pratap reviewed Nov 8, 2023

View reviewed changes

test/Engine/FFITarget/MinimalTraceTargets.cs Outdated Show resolved Hide resolved

check data before jsonconvert

c14abbb

saintentropy commented Nov 14, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Show resolved Hide resolved

mjkkirschner added 4 commits November 14, 2023 17:02

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

67a66eb

…eSerializationUpdate

some review comments

5859718

fix compression fix 1 test

fix 2 more tests

7ac11e3

fix tests, remove dep on samples package

37a666e

mjkkirschner removed the DNM Do not merge. For PRs. label Nov 16, 2023

aparajit-pratap reviewed Nov 16, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Nov 16, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Nov 16, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

mjkkirschner changed the title ~~WIP Experiment: Convert Trace Serialization mechanism from SOAP to JSON~~ Convert Trace Serialization mechanism from SOAP to JSON Nov 16, 2023

pinzart90 reviewed Nov 16, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/CallSite.cs Outdated Show resolved Hide resolved

add logger method

87c8860

log shorten legacy check

saintentropy commented Nov 17, 2023

View reviewed changes

src/NodeServices/TraceSupport.cs Show resolved Hide resolved

saintentropy commented Nov 17, 2023

View reviewed changes

src/Engine/ProtoCore/Lang/TraceUtils.cs Show resolved Hide resolved

mjkkirschner added 2 commits November 16, 2023 20:43

remove soap formatter

753eb4a

review comments

46f301e

remove use of ISerializable

add unit test

b9f75cd

saintentropy commented Nov 17, 2023

View reviewed changes

aparajit-pratap reviewed Nov 17, 2023

View reviewed changes

mjkkirschner approved these changes Nov 17, 2023

View reviewed changes

mjkkirschner merged commit d8de392 into DynamoDS:master Nov 17, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Trace Serialization mechanism from SOAP to JSON #14530

Convert Trace Serialization mechanism from SOAP to JSON #14530

saintentropy commented Oct 29, 2023 •

edited by mjkkirschner

Loading

mjkkirschner commented Oct 31, 2023

aparajit-pratap Nov 16, 2023

mjkkirschner Nov 16, 2023

saintentropy Nov 17, 2023

saintentropy Nov 17, 2023

saintentropy Nov 17, 2023

saintentropy Nov 17, 2023

saintentropy Nov 17, 2023

saintentropy commented Nov 17, 2023

saintentropy commented Nov 17, 2023

github-actions bot commented Nov 17, 2023 •

edited

Loading

saintentropy Nov 17, 2023

saintentropy Nov 17, 2023

saintentropy Nov 17, 2023

aparajit-pratap Nov 17, 2023

mjkkirschner Nov 17, 2023

mjkkirschner left a comment

Convert Trace Serialization mechanism from SOAP to JSON #14530

Convert Trace Serialization mechanism from SOAP to JSON #14530

Conversation

saintentropy commented Oct 29, 2023 • edited by mjkkirschner Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

mjkkirschner commented Oct 31, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintentropy commented Nov 17, 2023

saintentropy commented Nov 17, 2023

github-actions bot commented Nov 17, 2023 • edited Loading

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

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

saintentropy commented Oct 29, 2023 •

edited by mjkkirschner

Loading

github-actions bot commented Nov 17, 2023 •

edited

Loading