search

microsoft / perfview

PerfView is a CPU and memory performance-analysis tool
http://channel9.msdn.com/Series/PerfView-Tutorial
MIT License
4.05k stars 695 forks source link

Display timestamp while view ETW event #2008

Closed iaalm closed 3 months ago

iaalm commented 3 months ago

Addressing #1700 As the Time MSec is quite useful for filter, I just adding one extra field. Manually tested worked.

iaalm commented 3 months ago

@microsoft-github-policy-service agree

iaalm commented 3 months ago

@brianrob could you help review?

pharring commented 3 months ago

Time zones are always tricky. My experience with services tells me to convert everything to UTC. That tends to be a safe approach -- avoiding all question about time zone -- but, it's probably not appropriate for PerfView (expect, perhaps as an option). With PerfView, there are two local time zones to consider: The time zone on the machine where the data was gathered (if known) and the time zone on the machine where PerfView is running. When we say "Trace local", I assume that means that the times will be displayed using the time zone on the machine where the trace was gathered (including any adjustment for daylight saving that was in effect when the trace started). I seem to remember that ETW sessions include a time zone offset in the header. I don't know if netperf does.

I wanted to call this out, so we are clear and precise about what "Trace Local" means.

Oh, now that I think of it, we should update the Help documentation to describe the columns in the Event Viewer. There's a section for the Event Viewer, but it doesn't describe the various columns. (There is a "Column Description" section for the Stack Viewer, but not for the Event Viewer) We should add a "Column Description" section to the "The Event Viewer" section. We'll then get a chance to explain what "Time MSec" and "Timestamp (Trace Local)" mean.

iaalm commented 3 months ago

@pharring That's a good point. Seems these two time zones are both accessible, so I'm thinking about make an option for it. Maybe default using the time zone of gathered machine.

brianrob commented 3 months ago

Time zones are always tricky. My experience with services tells me to convert everything to UTC. That tends to be a safe approach -- avoiding all question about time zone -- but, it's probably not appropriate for PerfView (expect, perhaps as an option). With PerfView, there are two local time zones to consider: The time zone on the machine where the data was gathered (if known) and the time zone on the machine where PerfView is running. When we say "Trace local", I assume that means that the times will be displayed using the time zone on the machine where the trace was gathered (including any adjustment for daylight saving that was in effect when the trace started). I seem to remember that ETW sessions include a time zone offset in the header. I don't know if netperf does.

I wanted to call this out, so we are clear and precise about what "Trace Local" means.

Oh, now that I think of it, we should update the Help documentation to describe the columns in the Event Viewer. There's a section for the Event Viewer, but it doesn't describe the various columns. (There is a "Column Description" section for the Stack Viewer, but not for the Event Viewer) We should add a "Column Description" section to the "The Event Viewer" section. We'll then get a chance to explain what "Time MSec" and "Timestamp (Trace Local)" mean.

Agreed with @pharring's comments here. We should make sure to clarify time zone - I like the idea of a "Trace Local" option, but if that is too complicated, I am OK with UTC as the only option. I just want to make sure that we make it clear which one the user is looking at.

Also agreed on updating the help and description.

iaalm commented 3 months ago

@brianrob Please help review again.

iaalm commented 3 months ago

@brianrob for review😀

iaalm commented 3 months ago

@brianrob gentle ping..