search

microsoft / perfview

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

[TraceEvent] Restrict extension truncating to known exts #2111

Closed mdh1418 closed 2 months ago

mdh1418 commented 2 months ago

GetFileNameWithoutExtensionNoIllegalChars would always trim the "extension" regardless of whether or not the extension was truly an extension. This led to instances such as https://github.com/dotnet/runtime/issues/107315 where a TraceModuleFile with a FilePath of "System.Private.CoreLib" (due to reasons where no physical file on disk backed the image, so a fallback value of the module name was used), would end up with a TraceModuleFile.Name of "System.Private", when it should still be "System.Private.CoreLib".

mdh1418 commented 2 months ago

cc: @brianrob Are there particular tests that validate TraceProcess.Name or TraceModuleFile.Name or other extensions that we know should be truncated?

brianrob commented 2 months ago

I don't think we have any explicit unit tests, but this path is definitely being exercised. There are several traces that will be run through TraceEvent and then converted to text. This will run the code you changed. The failing test is showing that your code does the right thing in the face of a DLL name without the .dll extension:

[xUnit.net 00:00:57.84]            Expected: EVENT 196.607: KernelTraceControl/ImageID/None PID=3420; TID=-1; PName=com.docker; ProceNum=1; DataLen=12; 
[xUnit.net 00:00:57.84]            Actual  : EVENT 196.607: KernelTraceControl/ImageID/None PID=3420; TID=-1; PName=com.docker.service; ProceNum=1; DataLen=12;

If you run the tests locally through VS, the failing test will generate a new baseline that you can copy over the existing baseline. That should fix the test - of course, diff the result to make sure that things look right. I will look as well.

mdh1418 commented 2 months ago

Hi @brianrob and @cincuranet, could I get another review?

When might the TraceEvent package containing this change be available for consumption?

cincuranet commented 2 months ago

Except for small nit, LGTM.

We're releasing roughly once a month. Given the previous release was Aug 27, we're getting close to next one.

cincuranet commented 2 months ago

LGTM.

@brianrob does this change make sense to you (it does to me)?

brianrob commented 2 months ago

LGTM.

@brianrob does this change make sense to you (it does to me)?

Yes, makes sense to me. Feel free to merge.