Refactor log directory resolution into smaller methods, add tests#7255
Open
lucaspimentel wants to merge 3 commits intomasterfrom
Open
Refactor log directory resolution into smaller methods, add tests#7255lucaspimentel wants to merge 3 commits intomasterfrom
lucaspimentel wants to merge 3 commits intomasterfrom
Conversation
github-actions
bot
added
the
area:tracer
lucaspimentel
force-pushed
the
lpimentel/refactor-log-directory
branch
from
8d547a5 to
dccf49d
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7255) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (69ms) : 67, 72
master - mean (69ms) : 68, 71
section Bailout
This PR (7255) - mean (73ms) : 72, 75
master - mean (73ms) : 72, 75
section CallTarget+Inlining+NGEN
This PR (7255) - mean (1,041ms) : 1000, 1082
master - mean (1,044ms) : 990, 1098
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (108ms) : 102, 115
master - mean (108ms) : 105, 110
section Bailout
This PR (7255) - mean (108ms) : 106, 111
master - mean (108ms) : 106, 110
section CallTarget+Inlining+NGEN
This PR (7255) - mean (743ms) : 682, 805
master - mean (744ms) : 699, 789
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (96ms) : 94, 99
master - mean (95ms) : 93, 98
section Bailout
This PR (7255) - mean (97ms) : 95, 99
master - mean (97ms) : 95, 99
section CallTarget+Inlining+NGEN
This PR (7255) - mean (733ms) : 709, 756
master - mean (736ms) : 710, 762
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (95ms) : 92, 98
master - mean (94ms) : 91, 97
section Bailout
This PR (7255) - mean (95ms) : 93, 98
master - mean (95ms) : 93, 97
section CallTarget+Inlining+NGEN
This PR (7255) - mean (637ms) : 621, 653
master - mean (638ms) : 626, 651
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (195ms) : 190, 199
master - mean (192ms) : 189, 196
section Bailout
This PR (7255) - mean (198ms) : 194, 201
master - mean (197ms) : 193, 200
section CallTarget+Inlining+NGEN
This PR (7255) - mean (1,157ms) : 1100, 1214
master - mean (1,142ms) : 1096, 1188
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (280ms) : 274, 286
master - mean (276ms) : 270, 282
section Bailout
This PR (7255) - mean (280ms) : 276, 284
master - mean (276ms) : 272, 280
section CallTarget+Inlining+NGEN
This PR (7255) - mean (944ms) : 902, 986
master - mean (942ms) : 906, 978
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (271ms) : 267, 276
master - mean (270ms) : 264, 276
section Bailout
This PR (7255) - mean (273ms) : 267, 278
master - mean (269ms) : 265, 273
section CallTarget+Inlining+NGEN
This PR (7255) - mean (936ms) : 888, 984
master - mean (926ms) : 901, 951
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7255) - mean (272ms) : 265, 280
master - mean (267ms) : 262, 272
section Bailout
This PR (7255) - mean (272ms) : 267, 276
master - mean (267ms) : 262, 273
section CallTarget+Inlining+NGEN
This PR (7255) - mean (833ms) : 809, 858
master - mean (828ms) : 813, 844
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-02-27 01:07:35 Comparing candidate commit c2121d7 in PR branch Found 11 performance improvements and 7 performance regressions! Performance is the same for 165 metrics, 9 unstable metrics. scenario:Benchmarks.Trace.ActivityBenchmark.StartStopWithChild netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs net6.0
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice net472
scenario:Benchmarks.Trace.CharSliceBenchmark.OriginalCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery netcoreapp3.1
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.RedisBenchmark.SendReceive net472
scenario:Benchmarks.Trace.SerilogBenchmark.EnrichedLog net472
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net6.0
|
lucaspimentel
force-pushed
the
lpimentel/refactor-log-directory
branch
from
dccf49d to
3d77f7a
Compare
lucaspimentel
force-pushed
the
lpimentel/refactor-log-directory
branch
from
a69e50b to
f0bf2b3
Compare
This comment has been minimized.
This comment has been minimized.
lucaspimentel
force-pushed
the
lpimentel/refactor-log-directory
branch
2 times, most recently
from
65a9c16 to
956d0c9
Compare
lucaspimentel
force-pushed
the
lpimentel/refactor-log-directory
branch
from
956d0c9 to
a5f7c9b
Compare
lucaspimentel
changed the title
GetDefaultLogDirectory()
lucaspimentel
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f7c9bc77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+324
to
+326
| var logDirectory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | ||
| ? @"Z:\nonexistent\invalid\path\that\cannot\be\created" | ||
| : "/root/nonexistent/invalid/path/that/cannot/be/created"; |
There was a problem hiding this comment.
The Linux branch in this test uses /root/... as an "invalid" directory, but that path is writable when tests run as root (for example in containerized CI), so TryCreateLogDirectory returns true and the assertion fails even though production code is correct. I reproduced this by running DatadogLoggingFactoryTests in this environment, where WithInvalidPath_ReturnsFalse fails for exactly this reason; the test should use a path that is invalid regardless of user privileges.
Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment.
AI is right I'm afraid, as shown by the failing tests 😅 Try this:
Suggested change
| var logDirectory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | |
| ? @"Z:\nonexistent\invalid\path\that\cannot\be\created" | |
| : "/root/nonexistent/invalid/path/that/cannot/be/created"; | |
| var logDirectory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | |
| ? @"Z:\nonexistent\invalid\path\that\cannot\be\created" | |
| : "/dev/null/nonexistent/invalid/path/that/cannot/be/created"; |
lucaspimentel
force-pushed
the
lpimentel/refactor-log-directory
branch
from
a5f7c9b to
c2121d7
Compare
andrewlock
approved these changes
Feb 27, 2026
| if (string.IsNullOrEmpty(logDirectory)) | ||
| var isWindows = FrameworkDescription.Instance.IsWindows(); | ||
|
|
||
| if (ImmutableAzureAppServiceSettings.IsRunningInAzureAppServices(source, telemetry)) |
Member
There was a problem hiding this comment.
Looks like you've removed the || ImmutableAzureAppServiceSettings.IsRunningInAzureFunctions() - is that correct?
Comment on lines
+253
to
+265
| if (StringUtil.IsNullOrEmpty(programData)) | ||
| { | ||
| try | ||
| { | ||
| Directory.CreateDirectory(logDirectory); | ||
| } | ||
| catch | ||
| // fallback #1: try reading from the env var | ||
| programData = EnvironmentHelpersNoLogging.ProgramData(); | ||
|
|
||
| if (StringUtil.IsNullOrEmpty(programData)) | ||
| { | ||
| // Unable to create the directory meaning that the user | ||
| // will have to create it on their own. | ||
| // Last effort at writing logs | ||
| logDirectory = Path.GetTempPath(); | ||
| // fallback #2: hard-coded | ||
| programData = @"C:\ProgramData"; | ||
| } | ||
| } | ||
|
|
||
| return logDirectory!; | ||
| return programData; |
Member
There was a problem hiding this comment.
nit: I think early returns are neater in this case and reduces nesting
Suggested change
| if (StringUtil.IsNullOrEmpty(programData)) | |
| { | |
| try | |
| { | |
| Directory.CreateDirectory(logDirectory); | |
| } | |
| catch | |
| // fallback #1: try reading from the env var | |
| programData = EnvironmentHelpersNoLogging.ProgramData(); | |
| if (StringUtil.IsNullOrEmpty(programData)) | |
| { | |
| // Unable to create the directory meaning that the user | |
| // will have to create it on their own. | |
| // Last effort at writing logs | |
| logDirectory = Path.GetTempPath(); | |
| // fallback #2: hard-coded | |
| programData = @"C:\ProgramData"; | |
| } | |
| } | |
| return logDirectory!; | |
| return programData; | |
| if (!StringUtil.IsNullOrEmpty(programData)) | |
| { | |
| return programData; | |
| } | |
| // fallback #1: try reading from the env var | |
| // fallback #2: hard-coded | |
| var envProgramData = EnvironmentHelpersNoLogging.ProgramData(); | |
| return StringUtil.IsNullOrEmpty(envProgramData) | |
| ? @"C:\ProgramData" | |
| : envProgramData; |
| } | ||
| } | ||
|
|
||
| public class GetDefaultLogDirectoryTests |
Member
There was a problem hiding this comment.
nit: I don't think these should be separate classes based on our typical code style
Comment on lines
+247
to
+260
| [Fact] | ||
| public void ReturnsNonEmptyDirectory() | ||
| { | ||
| // Skip on non-Windows platforms since this method is only called on Windows | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var result = DatadogLoggingFactory.GetProgramDataDirectory(); | ||
|
|
||
| result.Should().NotBeNullOrEmpty(); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
meh, covered by the next test
Suggested change
| [Fact] | |
| public void ReturnsNonEmptyDirectory() | |
| { | |
| // Skip on non-Windows platforms since this method is only called on Windows | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | |
| { | |
| return; | |
| } | |
| var result = DatadogLoggingFactory.GetProgramDataDirectory(); | |
| result.Should().NotBeNullOrEmpty(); | |
| } |
Comment on lines
+324
to
+326
| var logDirectory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | ||
| ? @"Z:\nonexistent\invalid\path\that\cannot\be\created" | ||
| : "/root/nonexistent/invalid/path/that/cannot/be/created"; |
Member
There was a problem hiding this comment.
AI is right I'm afraid, as shown by the failing tests 😅 Try this:
Suggested change
| var logDirectory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | |
| ? @"Z:\nonexistent\invalid\path\that\cannot\be\created" | |
| : "/root/nonexistent/invalid/path/that/cannot/be/created"; | |
| var logDirectory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | |
| ? @"Z:\nonexistent\invalid\path\that\cannot\be\created" | |
| : "/dev/null/nonexistent/invalid/path/that/cannot/be/created"; |
Comment on lines
+287
to
+306
| public void WithValidPath_CreatesDirectoryAndReturnsTrue() | ||
| { | ||
| var logDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); | ||
| Directory.Exists(logDirectory).Should().BeFalse(); | ||
|
|
||
| var result = DatadogLoggingFactory.TryCreateLogDirectory(logDirectory); | ||
|
|
||
| result.Should().BeTrue(); | ||
| Directory.Exists(logDirectory).Should().BeTrue(); | ||
|
|
||
| // Cleanup | ||
| try | ||
| { | ||
| Directory.Delete(logDirectory); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
nit: I'd just combine this and the final tests. Plus the cleanup theoretically needs "fixing" to cleanup in case of failure:
Suggested change
| public void WithValidPath_CreatesDirectoryAndReturnsTrue() | |
| { | |
| var logDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); | |
| Directory.Exists(logDirectory).Should().BeFalse(); | |
| var result = DatadogLoggingFactory.TryCreateLogDirectory(logDirectory); | |
| result.Should().BeTrue(); | |
| Directory.Exists(logDirectory).Should().BeTrue(); | |
| // Cleanup | |
| try | |
| { | |
| Directory.Delete(logDirectory); | |
| } | |
| catch | |
| { | |
| // Ignore cleanup errors | |
| } | |
| } | |
| public void WithValidPath_CreatesDirectoryAndReturnsTrue() | |
| { | |
| var parentDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); | |
| var logDirectory = Path.Combine(parentDir, "nested", "log", "directory"); | |
| Directory.Exists(logDirectory).Should().BeFalse(); | |
| Directory.Exists(parentDir).Should().BeFalse(); | |
| try | |
| { | |
| var result = DatadogLoggingFactory.TryCreateLogDirectory(logDirectory); | |
| result.Should().BeTrue(); | |
| Directory.Exists(logDirectory).Should().BeTrue(); | |
| } | |
| finally | |
| { | |
| // Cleanup | |
| try | |
| { | |
| Directory.Delete(logDirectory, recursive: true); | |
| } | |
| catch | |
| { | |
| // Ignore cleanup errors | |
| } | |
| } | |
| } |
Comment on lines
+331
to
+355
|
|
||
| [Fact] | ||
| public void WithNestedPath_CreatesAllDirectories() | ||
| { | ||
| var parentDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); | ||
| var logDirectory = Path.Combine(parentDir, "nested", "log", "directory"); | ||
| Directory.Exists(logDirectory).Should().BeFalse(); | ||
| Directory.Exists(parentDir).Should().BeFalse(); | ||
|
|
||
| var result = DatadogLoggingFactory.TryCreateLogDirectory(logDirectory); | ||
|
|
||
| result.Should().BeTrue(); | ||
| Directory.Exists(logDirectory).Should().BeTrue(); | ||
| Directory.Exists(parentDir).Should().BeTrue(); | ||
|
|
||
| // Cleanup | ||
| try | ||
| { | ||
| Directory.Delete(parentDir, recursive: true); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
nit: covered by first test
Suggested change
| [Fact] | |
| public void WithNestedPath_CreatesAllDirectories() | |
| { | |
| var parentDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); | |
| var logDirectory = Path.Combine(parentDir, "nested", "log", "directory"); | |
| Directory.Exists(logDirectory).Should().BeFalse(); | |
| Directory.Exists(parentDir).Should().BeFalse(); | |
| var result = DatadogLoggingFactory.TryCreateLogDirectory(logDirectory); | |
| result.Should().BeTrue(); | |
| Directory.Exists(logDirectory).Should().BeTrue(); | |
| Directory.Exists(parentDir).Should().BeTrue(); | |
| // Cleanup | |
| try | |
| { | |
| Directory.Delete(parentDir, recursive: true); | |
| } | |
| catch | |
| { | |
| // Ignore cleanup errors | |
| } | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Refactor
DatadogLoggingFactorylog directory resolution logic into smaller, testable methods and add unit tests.Reason for change
The log directory resolution logic was a single large method mixing concerns:
Breaking it apart makes each piece independently testable and easier to understand.
Implementation details
GetDefaultLogDirectory()— determines the default log directory based on platform and Azure environmentGetProgramDataDirectory()— resolves theProgramDatapath on Windows (with Nano Server fallbacks)TryCreateLogDirectory()— attempts to create a log directory, returning success/failureinternalwith[TestingAndPrivateOnly]for testabilitySimplify
GetLogDirectory()to orchestrate the above methods with clear fallback chain:DD_TRACE_LOG_DIRECTORYenv varDD_TRACE_LOG_PATH(deprecated, directory extracted from file path)GetDefaultLogDirectory()(platform/Azure-aware default)Path.GetTempPath()(last resort)Test coverage
GetDefaultLogDirectoryTests— verifies default paths on Windows/Linux, and Azure App Services detectionGetProgramDataDirectoryTests— verifiesProgramDataresolution on WindowsTryCreateLogDirectoryTests— verifies directory creation success, existing directory, invalid path, and nested path scenariosOther details