Discuss new NetTrace V6 format #2170
Conversation
|
Just a heads up, I've been tweaking the design a bit while implementing the parser. I'll have an updated spec out soon as part of the PR that implements the functionality. (Even though an implementation exists its not set in stone yet - feedback welcome) |
|
This PR is updated with a complete implementation and some adjustments to the design as I considered it more and got some experience with implementing it. Unforetunately due to a personal issue I'll be away for likely at least a week but others are welcome to discuss, build the source if you need something to test with, or merge the code if its urgent to do that. I did have one other design piece I was considering putting in V6 that isn't included right now - changing the current encoding of ActivityId/RelatedActivity into an extensible set of correlation identifiers. In particular it might useful to have a place to store a TraceId/SpanId from the W3C TraceParent spec but it could also allow for open-ended storage of other identifiers. I was thinking of something like new ConstantPoolBlock that stores a list of indexed binary blobs: In the event header repurpose the slot/bit current used for activityId and store an optional reference into the ConstantPool there. The data that would go in the blob would be a concatenation of correlation identifiers similar to the optional thread data or optional metadata info. Something like: |
|
I updated the format with a support for label lists and updated the docs. If folks are happy with this then I think we could check it in. Now that we aren't racing to get this in for the next VS update we do have more time to adjust it, but aside from the label list part which is brand new I'm hoping most feedback elsewhere has already been resolved. I'm not aiming to make any significant further changes. |
|
@noahfalk Can you check the conflicts? Thanks. |
|
yeah, getting that merged up is in my todo list. The ball is in my court :) |
This is not intended to be checked in. Instead any changes we agree upon would get included in an updated version of the existing file format spec.
I extracted the FastSerializer deserialization logic out of EventPipeEventSource and put it into a separate FastSerializerObjectParser object. When using file format versions <= 5 this parser uses Deserializer to read all the objects. In V6+ we'll use a different (simpler) parser to iterate through blocks. I also added the V6 header detection logic but if it detects the V6 currently the code throws NotImplException. Added a V6 format parsing test case which triggers the NotImplException
Handles the new V6 trace block properly but uses V5 semantics for everything else.
Fix a bug I introduced that incorrectly treated V1 and V2 format headers as not using FastSerialization
-Convert MetadataHeader -> Metadata -Create the template at the end from the completed metadata rather than intermixing the creation of both -Get rid of unneeded metadata properties that temporarily cached data between different phases of parsing metadata
…om the format specific parsing
…e-effects of caching metadata. Instead be explicit.
- Created byte[] for all the different blocks eagerly in EventPipeBlock rather than passing around PinnedStreamReader for a while and then doing the conversion in many different places later. - Replaced ReadAndDispatchEvent() with ReadAndProcessEvent. Metadata is not dispatched so putting Dispatch in the method name was misleading. - Refactored ReadEvent() into the appropriate places in ReadAndProcessEvent(). ReadEvent() was weirdly defined before - it had part of the reading logic but also had the side-effect of caching metadata. - Consolidated populating header fields in ReadEventHeader(). This includes data that is directly in the file format header + data that is in the EventPipeEventHeader object that needs to be looked up from other parts of the file format. Right now the lookups are just stack information but more are coming in the V6 format.
Ideally I'd like to name these NetTraceXYZ but we can't do it consistently because EventPipeEventSource is already a public type. It seemed better to be consistent with EventPipe vs. inconsistent with NetTrace
…k in a separate file
… break extensibility points
…rocess Id information on events -Fixed a metadata parsing bug on V2 format that slipped by in the earlier changes
- Get rid of the length-prefixed extensibility point - KeyValue seems good enough and allows older readers to actually surface the data somewhere. - Split keywordlevelvalue into three separate items - Its slightly more expensive if you want all three, but it felt weird to couple them together and metadata isn't a super demanding part of the format for space efficiency. - Switch from count to size - Aligns the encoding with many of the other lists which are size/length prefixed rather than element count prefixed. For variable size elements its annoying that you have to parse the whole thing to skip over it.
Fixed a bug that has been in the parser probably a long time where TraceEvent.RelatedActivityId would be empty for events that had no stack trace. Previously only stack was translated to an EVENT_HEADER_EXTENDED_DATA_ITEM. Now RelatedActivityId is as well. Removed the MaxSupportedVersion from the UnsupportedFormatVersionException
…her than relative to V5. There is some relative version change information at the end of NetTraceFormat.md. I also renamed the current doc for the V5 format as NetTraceFormat_v5.md
Previously there was only one extended data entry but now the parser potentially creates a 2nd one for related activity id. We need to check if the entries are stack trace entries explicitly.
Although the alternate compression behavior would have been a slightly better for the typical pattern used by the runtime, it is much worse for writers that use the same CaptureThreadIndex for all events. I don't want to pessimize that scenario so much.
…ed EventPipeWriter so that it doesn't need to replicated in every method call
…ce point - Converted the thread indicies and sequence numbers in the sequence point block to use the more compact varuint encoding since we were already editing the block format to handle the flags. - Fixed a parser bug that misreported the number of dropped events in cases where some events were dropped - Clarified in spec that SequencePoint blocks don't have to enumerate sequence numbers for all (or any) threads. Enumerating threads there is just an optional mechanism for dropped event detection.
|
@cincuranet - I've rebased so it should be merging cleanly now Based off discussions with @beaubelgrave there are a few small adjustments in this most recent update:
Bug fixes and spec clarification:
|
Code updates are based on microsoft/perfview#2121 Would like to incorporate microsoft/perfview#2180 into dotnet-monitor, to potentially improve memory usage. ~~Test failures are most likely due to changes from microsoft/perfview#2170 Fixed with newest version
The format was moved to a different file as part of microsoft/perfview#2170.
* Update link to NetTrace format The format was moved to a different file as part of microsoft/perfview#2170. * Apply suggestions from code review Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com> --------- Co-authored-by: Andy (Steve) De George <67293991+adegeo@users.noreply.github.com> Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
5 participants
[EDIT]: I have updated this PR and now I am aiming to get it checked in as long as everyone is happy with it.
This PR adds support for parsing a new V6 version of the NetTrace file format. The design of the format is described in the included NetTraceFormat.md. At the end of that doc is a summary of the changes between V5 and V6.
NOTE: The parser is capable of reading things that the higher level TraceEvent API doesn't yet expose such as event descriptions, optional trace metadata, 8 byte thread ids, new correlation identifiers, etc. I think Brian and I will need to work together outside the scope of this PR to decide if/how/when we expose parts of that data in TraceEvent's public API surface. As-is the unit tests are calling into private APIs to confirm those extra data elements were parsed correctly.