Add Process Metadata Events #1276

mjsabby · 2020-09-18T05:55:02Z

mjsabby · 2020-09-18T22:08:16Z

@brianrob Could you take a look at this one?

brianrob

Thanks @mjsabby. Some comments and questions.

brianrob · 2020-09-21T23:53:19Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        }
+
+        [Event(2, Opcode = EventOpcode.Stop, Task = Tasks.Process)]
+        public void ProcessExit(long ProcessId, long ParentProcessId, int ExitCode, string Executable, string CommandLine)


Nit: Can we call this ProcessStop to match the start/stop naming guidance?

brianrob · 2020-09-21T23:54:14Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+    [EventSource(Name = "ProcessMetadataEventSource")]
+    public sealed class ProcessMetadataEventSource : EventSource
+    {
+        public class Tasks


Not sure if we need them yet, but should we have keywords for Process, Thread, and Module?

I intentionally steered away from them, because it would seem like process metadata is a sort of monolith. I'm happy to add them now or later if you see it differently.

brianrob · 2020-09-21T23:55:14Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        }
+
+        [Event(5, Opcode = EventOpcode.Start, Task = Tasks.Module)]
+        public void ModuleLoad(long ProcessId, ulong LoadAddress, long ModuleSize, Guid DebugGuid, int DebugAge, string ModuleFilePath, string DebugModuleFileName)


Is the DebugGuid a PDBGuid, or were you going for a cross-platform representation, and avoiding the term PDB? If so, is Guid used on Linux? I know for shared objects it's the build id.

As luck has it, ELF Debug IDs are 20-bytes. so I contemplated making this a byte[] but then thought about MachO which is also a GUID without any age, so I thought we could encode it as 0. And we could hack into the ELF IDs as guid + age to take the 20 bytes.

I just picked this and not a byte array because it was more convenient. @noahfalk thoughts?

Gotcha - yes, interested in @noahfalk's thoughts here. Do we have any other precedent?

I'd be tempted to encode it as a byte[] or string. Fixed size types would be a little more performant but given these shouldn't be frequent events I'd favor an encoding that feels straightforward.

If the goal is to be able to look up the images and symbols on a symbol server then the data we need is:
format - PE/ELF/MachO

PE - filesize, timestamp, filename, debug signature, age, and the codeview debug directory major/minor version (major=0x100,minor=504D is a sentinel value that indicates portable pdb is supported - PE spec).

ELF - build-id and filename

MachO - uuid and name

SSQP spec

brianrob · 2020-09-21T23:57:09Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        }
+
+        [Event(3, Opcode = EventOpcode.Start, Task = Tasks.Thread)]
+        public void ThreadCreate(long ProcessId, long ThreadId, ulong StackBaseAddress, string ThreadName)


Question: For the process and thread events, I'm interpreting that these events are for when the action occurs (e.g. when a thread is created we trigger a ThreadCreate event). What about cases where the thread already existed when the trace starts. Do we need a way to encapsulate that? I know that you're looking to avoid rundown - do you have a recommendation on how to handle this?

Rundown events or capture state events would have the same format, so I thought we could overload this, but I can also just add a specific thing for "Rundown", or maybe a more neutral term for catch up events.

I do think it would be worth knowing if the thread has existed longer than the trace or not. Perhaps something similar to Thread/DCStart? Also, if you don't mind, let's change to Thread/Start and Thread/Stop top match existing precedent.

Do we need a way to encapsulate that? I know that you're looking to avoid rundown - do you have a recommendation on how to handle this?

I agree that it would be worthwhile to distinguish information that is snapshotting the current state of the system from events that are indicating a state change is occuring at a specific point in time. Differently named events are certainly one way to represent that but I am going to suggest we hold off exploring too deeply before we've come to some decision whether we want the capability to slice regions out of a trace file and process the events in that time range in isolation. That slicing functionality seems like a pretty useful capability generally and I know its something Bing in particular benefits from, but it doesn't appear to play well with rundown style approaches that place a large number of stateful events at only a single point in the file. I think most formats that do support that kind of slicing wind up having something like keyframes, up-to-date snapshots of information that are repeated on a regular cadence.

brianrob · 2020-09-22T00:02:03Z

src/TraceEvent/TraceLog.cs

+
+            processMetadataParser.ThreadStart += delegate (ThreadCreateArgsTraceData data)
+            {
+                TraceProcess process = processes.GetOrCreateProcess((int)data.ProcessId, data.TimeStampQPC);


Can we work these more complicated Threading and module load events into helper functions and then just call them from here and from the ETW equivalents? There's enough here that it would be nice to only have one version of these.

brianrob · 2020-09-22T00:20:05Z

src/TraceEvent/TraceLog.cs

@@ -30,6 +30,7 @@
 using System.Text.RegularExpressions;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Diagnostics.Tracing.Parsers.ProcessMetadataEventSource;


I know that in one of your other PRs, you had done some work to expose process name inside of the EventPipe source. Do you think that would be worth plumbing in here as well?

brianrob · 2020-09-22T00:20:52Z

@noahfalk can you take a look at this as well? Would like to get your thoughts, especially on the moduleload abstractions.

noahfalk · 2020-09-23T07:17:29Z

Its in my todo list now : )

noahfalk · 2020-09-24T06:43:35Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+namespace Microsoft.Diagnostics.Tracing
+{
+    [EventSource(Name = "ProcessMetadataEventSource")]
+    public sealed class ProcessMetadataEventSource : EventSource


Does this EventSource ever get created or invoked when I run TraceEvent? I couldn't find any code elsewhere that used it though I suspect you needed it to create the parser generated code?

It is still useful to use as a reference for what would get emitted that we can talk about, but if its only role was going to a reference then maybe we can put the code somewhere else? @brianrob do we have any precedent around this?

It is only for reference.

noahfalk · 2020-09-24T06:47:50Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+
+namespace Microsoft.Diagnostics.Tracing
+{
+    [EventSource(Name = "ProcessMetadataEventSource")]


This name typically wouldn't have "EventSource" at the end of it. Most of our naming conventions either do Microsoft-bla-bla or we use a dotted namespace name. Neither of these feel appropriate here however so I am trying to think what we should do. I don't think we need to block on it though.

noahfalk · 2020-09-24T06:51:57Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        }
+
+        [Event(1, Opcode = EventOpcode.Start, Task = Tasks.Process)]
+        public void ProcessStart(long ProcessId, long ParentProcessId, string Executable, string CommandLine)


Does CommandLine include the executable? If it does do we need Executable as a separate field?

noahfalk · 2020-09-25T11:50:19Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        }
+
+        [Event(3, Opcode = EventOpcode.Start, Task = Tasks.Thread)]
+        public void ThreadCreate(long ProcessId, long ThreadId, ulong StackBaseAddress, string ThreadName)


Do we need a way to encapsulate that? I know that you're looking to avoid rundown - do you have a recommendation on how to handle this?

I agree that it would be worthwhile to distinguish information that is snapshotting the current state of the system from events that are indicating a state change is occuring at a specific point in time. Differently named events are certainly one way to represent that but I am going to suggest we hold off exploring too deeply before we've come to some decision whether we want the capability to slice regions out of a trace file and process the events in that time range in isolation. That slicing functionality seems like a pretty useful capability generally and I know its something Bing in particular benefits from, but it doesn't appear to play well with rundown style approaches that place a large number of stateful events at only a single point in the file. I think most formats that do support that kind of slicing wind up having something like keyframes, up-to-date snapshots of information that are repeated on a regular cadence.

noahfalk · 2020-09-25T12:01:15Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        [Event(3, Opcode = EventOpcode.Start, Task = Tasks.Thread)]
+        public void ThreadCreate(long ProcessId, long ThreadId, ulong StackBaseAddress, string ThreadName)
+        {
+            this.WriteEvent(3, ProcessId, ThreadId, StackBaseAddress, ThreadName);


Do we have a definition of what string to expect in ThreadName? For example .Net has a Thread.Name property and Linux has pthread_getname_np() but I don't know if the strings returned by those APIs are guaranteed to be equal.

Are you suggesting a name change? I suppose it could be either, but I think adding two thread name events would be overkill.

I am fine with the argument name (and keeping it singular). I am just trying to sort out what data do we expect to be provided? On Windows we have precedent from the kernel events but on all other platforms we have to define what is the equivalent data we are going to populate this with.

noahfalk · 2020-09-25T12:28:21Z

src/TraceEvent/EventSources/ProcessMetadataEventSource.cs

+        }
+
+        [Event(5, Opcode = EventOpcode.Start, Task = Tasks.Module)]
+        public void ModuleLoad(long ProcessId, ulong LoadAddress, long ModuleSize, Guid DebugGuid, int DebugAge, string ModuleFilePath, string DebugModuleFileName)


I'd be tempted to encode it as a byte[] or string. Fixed size types would be a little more performant but given these shouldn't be frequent events I'd favor an encoding that feels straightforward.

If the goal is to be able to look up the images and symbols on a symbol server then the data we need is:
format - PE/ELF/MachO

PE - filesize, timestamp, filename, debug signature, age, and the codeview debug directory major/minor version (major=0x100,minor=504D is a sentinel value that indicates portable pdb is supported - PE spec).

ELF - build-id and filename

MachO - uuid and name

SSQP spec

noahfalk · 2020-09-25T21:55:29Z

src/TraceEvent/Parsers/ProcessMetadataTraceEventParser.cs

+    using Microsoft.Diagnostics.Tracing.Parsers.ProcessMetadataEventSource;
+
+    [System.CodeDom.Compiler.GeneratedCode("traceparsergen", "2.0")]
+    public sealed class ProcessMetadataEventSourceTraceEventParser : TraceEventParser


I realized above I said we didn't need to block on the name, but that probably also means we need to avoid putting the name in new public API surface. Can we switch to internal for now or should we just try to sort out the naming right away?

I'm fine but I don't have an opinion, I think it should be the .net name.

noahfalk · 2020-09-25T21:56:04Z

src/TraceEvent/TraceEvent.cs

+        /// For convenience, we provide a property returns a ProcessMetadataTraceEventParser that knows 
+        /// how to parse all the Process Metadata events into callbacks.
+        /// </summary>
+        public ProcessMetadataEventSourceTraceEventParser ProcessMetadata


Similar to the comment above, another spot where the name is currently showing up in public API surface

Add Process Metadata Events

a52ce02

mjsabby force-pushed the processmetadata branch from 2a29458 to a52ce02 Compare September 18, 2020 05:56

brianrob reviewed Sep 22, 2020

View reviewed changes

noahfalk reviewed Sep 25, 2020

View reviewed changes

Base automatically changed from master to main February 2, 2021 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Process Metadata Events #1276

Add Process Metadata Events #1276

mjsabby commented Sep 18, 2020

mjsabby commented Sep 18, 2020

brianrob left a comment

brianrob Sep 21, 2020

mjsabby Sep 23, 2020

brianrob Sep 21, 2020

mjsabby Sep 23, 2020

brianrob Sep 21, 2020

mjsabby Sep 23, 2020

brianrob Sep 23, 2020

noahfalk Sep 25, 2020

brianrob Sep 21, 2020

mjsabby Sep 23, 2020

brianrob Sep 23, 2020

noahfalk Sep 25, 2020

brianrob Sep 22, 2020

brianrob Sep 22, 2020

mjsabby Sep 23, 2020

brianrob commented Sep 22, 2020

noahfalk commented Sep 23, 2020

noahfalk Sep 24, 2020

mjsabby Sep 25, 2020

noahfalk Sep 24, 2020

noahfalk Sep 24, 2020

noahfalk Sep 25, 2020

noahfalk Sep 25, 2020

mjsabby Sep 25, 2020

noahfalk Sep 25, 2020 •

edited

Loading

noahfalk Sep 25, 2020

noahfalk Sep 25, 2020

mjsabby Sep 25, 2020

noahfalk Sep 25, 2020

Add Process Metadata Events #1276

Are you sure you want to change the base?

Add Process Metadata Events #1276

Conversation

mjsabby commented Sep 18, 2020

mjsabby commented Sep 18, 2020

brianrob left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrob commented Sep 22, 2020

noahfalk commented Sep 23, 2020

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

noahfalk Sep 25, 2020 • 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

noahfalk Sep 25, 2020 •

edited

Loading