Skip to content
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

Allow viewing the log file for SelfDiagnostics for .NET Framework #1693

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Jan 13, 2021

Fixes Bug#2 in #1689. When running with .NET Framework, the log file cannot be opened as read-only, caused by default FileShare behavior in MemoryMappedFile in .NET Framework.

.NET Framework opens the file in FileShare.None mode but .NET Core opens the file in FileShare.Read mode.

Changes

Use the FileStream version of MemoryMappedFile.CreateFromFile instead of the string version, because the the string version opens the file in different FileShare mode for .NET Framework and .NET Core.

Tested manually:

  1. Modify code. Wrap using (var activity = MyActivitySource.StartActivity("SayHello")) block in docs/trace/getting-started/Program.cs with while (true) loop.
  2. Create a configurations file and run the project according to issue SelfDiagnosticsModule doesn't filter out logs of lower severity level as configured #1689.
  3. When the program is running, open the output file (with name like "getting-started.exe..log"). The file should open successfully.

@xiang17 xiang17 requested a review from a team as a code owner January 13, 2021 00:39
@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #1693 (fe51233) into main (4f39a58) will increase coverage by 0.69%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1693      +/-   ##
==========================================
+ Coverage   82.46%   83.15%   +0.69%     
==========================================
  Files         250      250              
  Lines        6803     6823      +20     
==========================================
+ Hits         5610     5674      +64     
+ Misses       1193     1149      -44     
Impacted Files Coverage Δ
...nTelemetry/Internal/SelfDiagnosticsConfigParser.cs 81.39% <ø> (+32.55%) ⬆️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 74.52% <100.00%> (+37.31%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+8.82%) ⬆️

@xiang17 xiang17 changed the title Fix different FileShare behavior in MemoryMappedFile between .NET Framework and .NET Core for SelfDiagnostics Allow viewing the log file for SelfDiagnostics for .NET Framework Jan 13, 2021
@xiang17
Copy link
Contributor Author

xiang17 commented Jan 14, 2021

I also ran the unit tests for the code before the fix as a reproduce of the bug. The test cases all passed for .NET Core but failed for .NET Framework as expected.

OpenTelemetry - Microsoft Visual Studio 1_13_2021 4_37_15 PM - Copy (2)

@cijothomas
Copy link
Member

please add this to changelog as well.

@xiang17
Copy link
Contributor Author

xiang17 commented Jan 20, 2021

please add this to changelog as well.

Added.

@cijothomas
Copy link
Member

@xiang17 please resolve confloicts. change look good for me.

@xiang17
Copy link
Contributor Author

xiang17 commented Jan 22, 2021

@xiang17 please resolve confloicts. change look good for me.

Done.

@eddynaka eddynaka changed the base branch from master to main January 27, 2021 22:14
@cijothomas cijothomas merged commit 4b5388b into open-telemetry:main Jan 28, 2021
@xiang17 xiang17 deleted the xiang17/FixBugInSelfDiagnostics_FileShareStatus branch January 29, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants