-
Notifications
You must be signed in to change notification settings - Fork 757
NetTrace LabelList metadata overrides and metadata flushing #2281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
noahfalk
merged 4 commits into
microsoft:main
from
noahfalk:onecollect_nettrace_features
Aug 25, 2025
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
0b72f66
NetTrace LabelList metadata overrides and metadata flushing
noahfalk 724be70
Add support for explicit zero opcode label overriding templates, but …
noahfalk 2f78391
Fix ValidateStackFields to work for older NetTrace format versions too
noahfalk f80e04c
Remove unneeded opcode re-assignment
noahfalk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the
_metadataTemplatesare cleared, would them_state.m_templatesdictionary need to be updated or completely flushed as well? I'm still getting acquainted with this codebase, but I was thinking that known metadata templates would get added to them_state.m_templatesdictionary through theRegisterUnhandledEventcallbackperfview/src/TraceEvent/RegisteredTraceEventParser.cs
Lines 1185 to 1224 in a8605fe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. If the metadata gets flushed and can be re-defined, then yes,
m_state.m_templateswill likely contain templates that correspond to the old metadata and would need to be flushed. This is likely to be difficult though because the template has also likely been added to the main lookup hash table. Can you remind me what the goal is of flushing the metadata part way through a trace?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today if you want to read/write a NetTrace file you need to maintain a mapping for MetadataID -> Metadata for every event type in the file. Although typical files don't have large numbers of event types there is no guarantee that will be true for all traces. A pathological trace could have 10M events each with a different metadata definition. If the reader/writer are required to maintain these caches there is a potential for unbounded memory usage. To avoid that we need a mechanism that allows the reader/writer to keep the cache bounded. When the writer is writing out events any time the cache gets too big it can emit a sequence point, delete its cache, and begin accumulating a new cache. This is the same pattern we already do for Stacks, LabelLists, and optionally Threads. On the reader side the reader doesn't have the freedom to enforce its own cache size limit but it can at least benefit from whatever limit the writer put in place.
A 2nd benefit is that flushable metadata is a precursor to being able to use nettrace format as part of a circular event buffer. Today if we wrote a bunch of nettrace data and then deleted the first half of the file we might have deleted the metadata necessary to understand events in the 2nd half of the file. However if deletion is done at sequence point boundaries where metadata is flushed then we can be assured that all metadata necessary to parse the remaining events is still available. It makes regions of the file between sequence points self-contained.
I expect this will be fine, with a few caveats:
Metadata=1, EventId=1, ProviderGuid=X, EventName=Foo
Metadata=2, EventId=1, ProviderGuid=X, EventName=Bar
Parsing such a Trace with TraceEvent would treat all events with either of those MetadataIDs as having whichever name was observed first. With flushing the same rule still applies as before. If writers want the trace to be parsed properly we expect Metadata that refers to the same ProviderId+EventId should agree on the same definition. This means it will be fine if TraceEvent's cache retains an old template because we expect it to store the same data as any new template on the same ProviderId+EventId.
NOTE: Looking at TraceEvent's code, it seems to assume that ProviderId+EventId is a sufficient identifier without considering PID. If two processes disagreed on the definition of an event TraceEvent appears to treat all events from all processes using whatever definition was observed first. Nothing specific to NetTrace, this behavior appears to apply for everything sharing the default TraceDispatcher logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @noahfalk. That makes sense. I agree that it's already possible to end up in the situation you describe where the EventID and Provider GUIDs for two metadata entries are the same but the event name is different, and only the first one will be used during lookup. Given that we expect writers not to do this, I think it we're fine for now.
We could certainly build a feature in the future that would allow you to remove a template from the cache. However, this would only be impactful if nothing else rooted the template. Some applications would benefit from this, while others might benefit much less if they hooked all/many of the templates to a delegate in such a way that the template remained rooted during execution. Either way, this is something we can look at going forward.