Logging improvements #456
Logging improvements #456
Conversation
| } | ||
| } | ||
|
|
||
| logDirectory = CreateDirectoryIfMissing(logDirectory) ?? Path.GetTempPath(); |
There was a problem hiding this comment.
We need a respective doc update to reflect this fallback.
src/OpenTelemetry.AutoInstrumentation/Instrumentations/GraphQL/GraphQLCommon.cs
Show resolved
Hide resolved
|
Added a related issue #457 |
|
|
||
| if (logDirectory == null) | ||
| { | ||
| var nativeLogFile = Environment.GetEnvironmentVariable(ConfigurationKeys.LogPath); |
There was a problem hiding this comment.
how about renaming nativeLogFile to something more or less like envVarValue ?
docs/config.md
Outdated
| @@ -58,6 +58,8 @@ Default logs directory paths are: | |||
| - Windows: `%ProgramData%\OpenTelemetry .NET AutoInstrumentation\logs` | |||
| - Linux: `/var/log/opentelemetry/dotnet` | |||
|
|
|||
| If there is a problem with creation of the default directory, the path of the current user's [temporary folder](https://docs.microsoft.com/en-us/dotnet/api/System.IO.Path.GetTempPath?view=net-6.0) is used. | |||
There was a problem hiding this comment.
| If there is a problem with creation of the default directory, the path of the current user's [temporary folder](https://docs.microsoft.com/en-us/dotnet/api/System.IO.Path.GetTempPath?view=net-6.0) is used. | |
| If there is a problem with the creation of the default directory, | |
| the path of the current user's [temporary folder](https://docs.microsoft.com/en-us/dotnet/api/System.IO.Path.GetTempPath?view=net-6.0) | |
| is used. |
There was a problem hiding this comment.
Looks good, however since #458 was merged, the following files may need to be updated for tests to pass:
src/OpenTelemetry.AutoInstrumentation/Diagnostics/SdkSelfDiagnosticsEventListener.cstest/OpenTelemetry.AutoInstrumentation.Tests/Diagnostics/SdkSelfDiagnosticsEventListenerTests.cs
.../OpenTelemetry.AutoInstrumentation.Tests/Diagnostics/SdkSelfDiagnosticsEventListenerTests.cs
Show resolved
Hide resolved
| /// <summary> | ||
| /// Configuration key for setting the directory for the profiler's log files. | ||
| /// If set, this setting takes precedence over environment variable OTEL_DOTNET_AUTO_LOG_PATH. | ||
| /// If not set, default is | ||
| /// "%ProgramData%"\OpenTelemetry .NET AutoInstrumentation\logs\" on Windows or | ||
| /// "/var/log/opentelemetry/dotnet/" on Linux. | ||
| /// </summary> | ||
| public const string LogDirectory = "OTEL_DOTNET_AUTO_LOG_DIRECTORY"; | ||
|
|
||
| /// <summary> | ||
| /// Configuration key for setting the path for the profiler's native log file. | ||
| /// Environment variable OTEL_DOTNET_AUTO_LOG_DIRECTORY takes precedence over this setting, if set. | ||
| /// </summary> | ||
| public const string LogPath = "OTEL_DOTNET_AUTO_LOG_PATH"; | ||
|
|
There was a problem hiding this comment.
Do we need to have both settings?
There was a problem hiding this comment.
I added them both here, because both need to be checked to be consistent with StartupLogger, which takes them into account when deciding on log directory.
There was a problem hiding this comment.
I guess it is also used in the native logger. I created an issue to remove one of them: #476
There was a problem hiding this comment.
Yeah, better to have a single env var and use the same for native and managed.
Another thing that we may want to consider post-beta is: the native log is one file for multiple applications so it is not symmetrical to the managed one. Perhaps it will make the user's life simpler if the logs of a single run were easier to "group".
Why
Remove console logging.
Fixes #334
What
Simple logging to a file, based on logic from loader:
FileSinkfrom loaderPossible improvements:
Tests
Smoke tests.