Update NetTrace parameter parsing #2200
Conversation
BREAKING CHANGE: NetTrace V5 files that use the Array parameter type with character elements were previously marshalled into a .NET char[], but now TraceEvent converts them to a .NET System.String instead. - Updated support for parameter types in NetTrace V6 - Added new parameter types FixedLengthArray, DataLoc, RelLoc, and UTF8CodeUnit - Parameter type UTF16Char was renamed UTF16CodeUnit for in docs and code for clarity (chars are variable size in UTF16, code units are always 2 bytes). Behaviorly there is no change from before. - LengthPrefixedUTF8/16String were removed because they were duplicative with Array<UTF16CodeUnit> and Array<UTF8CodeUnit>. It didn't seem worthwhile to have a bunch of different string types when we could just interpret any of the 4 array variants with UTF8/16 elements as strings. In order to add the new array types I needed to increase the functionality of the underlying PayloadFetch that TraceEvent uses to parse the parameters. PayloadFetch is serializable in the Etlx format so I wanted to added some tests that could round-trip the payload fetch through that serialization. That in turn led me to refactor TraceLog so that I could do it in-memory. I also found a variety of Debug asserts weren't true for the test data so I removed or relaxed them.
brianrob
left a comment
brianrob
left a comment
There was a problem hiding this comment.
Overall LGTM. A few questions/comments.
| // While scanning over the stream, copy all data to the file. | ||
| rawEvents.AllEvents += delegate (TraceEvent data) | ||
| { | ||
| Debug.Assert(_syncTimeQPC != 0); // We should have set this in the Header event (or on session start if it is read time |
There was a problem hiding this comment.
Do you know what changed _syncTimeQPC to be 0? I see this in a few places in this change, but I don't see what actually changed it.
There was a problem hiding this comment.
It is coming from mock trace data (https://github.com/microsoft/perfview/pull/2200/files#diff-c94b1f053e78f716a557a5876766fc48558133ec18fe0fa752e5cf40c23a6fcfR902).
I changed the sync time to zero because I am also reporting events with timestamp 0. When SyncTime > event time that triggered other asserts to fire. It is possible to modify test cases to pass more realistic timestamps but I tried to avoid making the test data more complicated when it was orthogonal to what a particular test case was aiming to test.
There was a problem hiding this comment.
Oops, that was the assert above. This is the line that changed the test data: https://github.com/microsoft/perfview/pull/2200/files#diff-c94b1f053e78f716a557a5876766fc48558133ec18fe0fa752e5cf40c23a6fcfR2850
Co-authored-by: Brian Robbins <brianrob@microsoft.com>
|
@brianrob - I think I've responded to all your feedback. Take a look when you get the chance and let me know if you think we should make more changes. Thanks! |
2 participants
BREAKING CHANGE: NetTrace V5 files that use the Array parameter type with character elements were previously marshalled into a .NET char[], but now TraceEvent converts them to a .NET System.String instead.
In order to add the new array types I needed to increase the functionality of the underlying PayloadFetch that TraceEvent uses to parse the parameters. PayloadFetch is serializable in the Etlx format so I wanted to added some tests that could round-trip the payload fetch through that serialization. That in turn led me to refactor TraceLog so that I could do it in-memory. I also found a variety of Debug asserts weren't true for the test data so I removed or relaxed them.
PTAL @brianrob
Fyi @beaubelgrave @mdh1418 @lateralusX @wiktork
Although most of the work here only impacts the new V6 NetTrace format heads up about the (hopefully minor) breaking change for V5. I could do more to make the change optional or preserve the existing behavior, but it just feels awkward to have character arrays marshalled as .NET char[] rather than string. In any case, if you are concerned about it do let me know.