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.

Sign up for GitHub

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

Jump to bottom

Normalize the environment variable names used by crashtracking #5898

Merged
merged 5 commits into from
Aug 16, 2024
Merged

Normalize the environment variable names used by crashtracking #5898

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b3f8d8d
Normalize the crashtracking environment variables
kevingosse Aug 14, 2024
868c5db
Remove DD_TRACE_CRASH_HANDLER tests
kevingosse Aug 14, 2024
f74319d
Update functions_to_wrap.c
kevingosse Aug 14, 2024
904b7eb
Address PR feedback
kevingosse Aug 16, 2024
aa6d53e
Fix
kevingosse Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 57 additions & 68 deletions profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ void initLibrary(void)
{
check_init();

const char* crashHandlerEnabled = getenv("DD_TRACE_CRASH_HANDLER_ENABLED");
const char* crashHandlerEnabled = getenv("DD_CRASHTRACKING_ENABLED");

if (crashHandlerEnabled != NULL)
{
Expand All @@ -204,91 +204,80 @@ void initLibrary(void)
// If set, set DD_TRACE_CRASH_HANDLER_PASSTHROUGH to indicate dd-dotnet that it should call createdump
// If not set, set it to 1 so that .NET calls createdump in case of crash
// (and we will redirect the call to dd-dotnet)
const char* crashHandlerEnv = getenv("DD_TRACE_CRASH_HANDLER");
// The path to the crash handler is not set, try to deduce it
const char* libraryPath = getLibraryPath();

if (crashHandlerEnv == NULL || crashHandlerEnv[0] == '\0')
{
// The path to the crash handler is not set, try to deduce it
const char* libraryPath = getLibraryPath();
if (libraryPath != NULL)
{
// If the library is in linux-x64 or linux-musl-x64, we use that folder
// Otherwise, if the library is in continuousprofiler, we have to call isAlpine()
// and use either ../linux-x64/ or ../linux-musl-x64/, or their ARM64 equivalent
char* subFolder = getSubfolder(libraryPath);

if (libraryPath != NULL)
{
// If the library is in linux-x64 or linux-musl-x64, we use that folder
// Otherwise, if the library is in continuousprofiler, we have to call isAlpine()
// and use either ../linux-x64/ or ../linux-musl-x64/, or their ARM64 equivalent
char* subFolder = getSubfolder(libraryPath);
if (subFolder != NULL)
{
char* newCrashHandler = NULL;

if (subFolder != NULL)
if (strcmp(subFolder, DdDotnetFolder) == 0
|| strcmp(subFolder, DdDotnetMuslFolder) == 0)
{
char* newCrashHandler = NULL;
// We use the dd-dotnet in that same folder
char* folder = getFolder(libraryPath);

if (strcmp(subFolder, DdDotnetFolder) == 0
|| strcmp(subFolder, DdDotnetMuslFolder) == 0)
if (folder != NULL)
{
// We use the dd-dotnet in that same folder
char* folder = getFolder(libraryPath);

if (folder != NULL)
{
asprintf(&newCrashHandler, "%s/dd-dotnet", folder);
free(folder);
}
asprintf(&newCrashHandler, "%s/dd-dotnet", folder);
free(folder);
}
else
}
else
{
char* folder = getFolder(libraryPath);

if (folder != NULL)
{
char* folder = getFolder(libraryPath);
const char* currentDdDotnetFolder;

if (folder != NULL)
if (isAlpine() == 0)
{
const char* currentDdDotnetFolder;

if (isAlpine() == 0)
{
currentDdDotnetFolder = DdDotnetFolder;
}
else
{
currentDdDotnetFolder = DdDotnetMuslFolder;
}

if (strcmp(subFolder, "continuousprofiler") == 0)
{
// If we're in continuousprofiler, we need to go up one folder
asprintf(&newCrashHandler, "%s/../%s/dd-dotnet", folder, currentDdDotnetFolder);
}
else
{
// Assume we're at the root
asprintf(&newCrashHandler, "%s/%s/dd-dotnet", folder, currentDdDotnetFolder);
}

free(folder);
currentDdDotnetFolder = DdDotnetFolder;
}
else
{
currentDdDotnetFolder = DdDotnetMuslFolder;
}
}

free(subFolder);

if (newCrashHandler != NULL)
{
// Make sure the file exists and has execute permissions
if (access(newCrashHandler, X_OK) == 0)
if (strcmp(subFolder, "continuousprofiler") == 0)
{
crashHandler = newCrashHandler;
// If we're in continuousprofiler, we need to go up one folder
asprintf(&newCrashHandler, "%s/../%s/dd-dotnet", folder, currentDdDotnetFolder);
}
else
{
free(newCrashHandler);
// Assume we're at the root
asprintf(&newCrashHandler, "%s/%s/dd-dotnet", folder, currentDdDotnetFolder);
}

free(folder);
}
}

free(subFolder);

if (newCrashHandler != NULL)
{
// Make sure the file exists and has execute permissions
if (access(newCrashHandler, X_OK) == 0)
{
crashHandler = newCrashHandler;
}
else
{
free(newCrashHandler);
}
}
}
}
else
{
// The environment variables can change during the lifetime of the process,
// so make a copy of the string
crashHandler = strdup(crashHandlerEnv);
}

if (crashHandler != NULL && crashHandler[0] != '\0')
{
Expand All @@ -302,7 +291,7 @@ void initLibrary(void)
if (enableMiniDump != NULL && enableMiniDump[0] == '1')
{
// If DOTNET_DbgEnableMiniDump is set, the crash handler should call createdump when done
char* passthrough = getenv("DD_TRACE_CRASH_HANDLER_PASSTHROUGH");
char* passthrough = getenv("DD_INTERNAL_CRASHTRACKING_PASSTHROUGH");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use constants for env var names to avoid repeating them and maybe introduce typos

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 904b7eb


if (passthrough == NULL || passthrough[0] == '\0')
{
Expand All @@ -311,7 +300,7 @@ void initLibrary(void)
// - dotnet run sets DOTNET_DbgEnableMiniDump=1
// - dotnet then launches the target app
// - the target app thinks DOTNET_DbgEnableMiniDump has been set by the user and enables passthrough
setenv("DD_TRACE_CRASH_HANDLER_PASSTHROUGH", "1", 1);
setenv("DD_INTERNAL_CRASHTRACKING_PASSTHROUGH", "1", 1);
}
}
else
Expand All @@ -320,7 +309,7 @@ void initLibrary(void)
// but we instruct it to not call createdump afterwards
setenv("COMPlus_DbgEnableMiniDump", "1", 1);
setenv("DOTNET_DbgEnableMiniDump", "1", 1);
setenv("DD_TRACE_CRASH_HANDLER_PASSTHROUGH", "0", 1);
setenv("DD_INTERNAL_CRASHTRACKING_PASSTHROUGH", "0", 1);
}

originalMiniDumpName = getenv("DOTNET_DbgMiniDumpName");
Expand Down
50 changes: 0 additions & 50 deletions profiler/test/Datadog.Profiler.IntegrationTests/LinuxOnly/WrapperLibraryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,55 +80,5 @@ public void EnsureAppDoesNotCrashIfProfilerDeactivateAndTracerActivated(string a
var lines = File.ReadAllLines(logFile);
lines.Should().ContainMatch(expectedErrorMessage);
}

[TestAppFact("Samples.ExceptionGenerator")]
public void RedirectCrashHandler(string appName, string framework, string appAssembly)
{
var runner = new TestApplicationRunner(appName, framework, appAssembly, _output, enableTracer: true, commandLine: "--scenario 7");

runner.Environment.SetVariable("COMPlus_DbgMiniDumpType", string.Empty);

RegisterCrashHandler(runner);

using var processHelper = runner.LaunchProcess();

runner.WaitForExitOrCaptureDump(processHelper.Process, milliseconds: 30_000).Should().BeTrue();
processHelper.Drain();
processHelper.ErrorOutput.Should().Contain("Unhandled exception. System.InvalidOperationException: Task failed successfully");
processHelper.StandardOutput.Should().MatchRegex(@"createdump [\w\.\/]+createdump \d+")
.And.NotContain("Writing minidump");
}

[TestAppFact("Samples.ExceptionGenerator")]
public void DontRedirectCrashHandlerIfPathNotSet(string appName, string framework, string appAssembly)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keep this test and rename it into GenerateDumpIfDbgRequested to validate that existing CLR behavior is still working with CrashTracking

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 904b7eb

{
var runner = new TestApplicationRunner(appName, framework, appAssembly, _output, enableTracer: true, commandLine: "--scenario 7");

// Don't set DD_TRACE_CRASH_HANDLER. In that case, the call to createdump shouldn't be redirected
runner.Environment.SetVariable("COMPlus_DbgEnableMiniDump", "1");
runner.Environment.SetVariable("COMPlus_DbgMiniDumpName", "/dev/null");
runner.Environment.SetVariable("COMPlus_DbgMiniDumpType", string.Empty);

using var processHelper = runner.LaunchProcess();

runner.WaitForExitOrCaptureDump(processHelper.Process, milliseconds: 30_000).Should().BeTrue();
processHelper.Drain();
processHelper.ErrorOutput.Should().Contain("Unhandled exception. System.InvalidOperationException: Task failed successfully");
processHelper.StandardOutput.Should().NotMatchRegex(@"createdump [\w\.\/]+createdump \d+")
.And.Contain("Writing minidump");
}

private void RegisterCrashHandler(TestApplicationRunner runner)
{
var crashHandler = "/bin/echo";

if (!File.Exists(crashHandler))
{
_output.WriteLine($"Crash handler {crashHandler} does not exist.");
throw new FileNotFoundException($"Crash handler {crashHandler} does not exist.");
}

runner.Environment.SetVariable("DD_TRACE_CRASH_HANDLER", crashHandler);
}
}
}
4 changes: 2 additions & 2 deletions tracer/src/Datadog.Trace.Tools.dd_dotnet/CreatedumpCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ private void Execute(InvocationContext context)

// Note: if refactoring, make sure to dispose the ClrMD DataTarget before calling createdump,
// otherwise the calls to ptrace from createdump will fail
if (Environment.GetEnvironmentVariable("DD_TRACE_CRASH_HANDLER_PASSTHROUGH") == "1")
if (Environment.GetEnvironmentVariable("DD_INTERNAL_CRASHTRACKING_PASSTHROUGH") == "1")
{
if (allArguments.Length > 0)
{
Expand Down Expand Up @@ -547,7 +547,7 @@ private unsafe void GenerateCrashReport(int pid, int? signal, int? crashThread)

try
{
var outputFile = Environment.GetEnvironmentVariable("DD_TRACE_CRASH_OUTPUT");
var outputFile = Environment.GetEnvironmentVariable("DD_INTERNAL_CRASHTRACKING_OUTPUT");

if (!string.IsNullOrEmpty(outputFile))
{
Expand Down
8 changes: 4 additions & 4 deletions tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/CreatedumpTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,14 @@ public async Task Passthrough(string passthrough, bool shouldCallCreatedump)
// If it wasn't set, then we set it so that dd-dotnet will be invoked in case of crash.
// If a child dotnet process is spawned, we may then mistakenly think that COMPlus_DbgEnableMiniDump
// was set from the environment, even though it was set by us. To prevent that, we set the
// DD_TRACE_CRASH_HANDLER_PASSTHROUGH environment variable, which codifies the result of the
// DD_INTERNAL_CRASHTRACKING_PASSTHROUGH environment variable, which codifies the result of the
// "was COMPlus_DbgEnableMiniDump set?" check.

SkipOn.Platform(SkipOn.PlatformValue.MacOs);

using var reportFile = new TemporaryFile();

(string, string)[] args = [LdPreloadConfig, .. CreatedumpConfig, ("DD_TRACE_CRASH_HANDLER_PASSTHROUGH", passthrough), CrashReportConfig(reportFile)];
(string, string)[] args = [LdPreloadConfig, .. CreatedumpConfig, ("DD_INTERNAL_CRASHTRACKING_PASSTHROUGH", passthrough), CrashReportConfig(reportFile)];

using var helper = await StartConsoleWithArgs("crash-datadog", false, args);

Expand Down Expand Up @@ -103,7 +103,7 @@ public async Task DoNothingIfNotEnabled(bool enableCrashDumps)

using var reportFile = new TemporaryFile();

(string, string)[] args = [LdPreloadConfig, ("DD_TRACE_CRASH_HANDLER_ENABLED", "0")];
(string, string)[] args = [LdPreloadConfig, ("DD_CRASHTRACKING_ENABLED", "0")];

if (enableCrashDumps)
{
Expand Down Expand Up @@ -444,7 +444,7 @@ void ValidateStacktrace(JToken callstack)

private static (string Key, string Value) CrashReportConfig(TemporaryFile reportFile)
{
return ("DD_TRACE_CRASH_OUTPUT", reportFile.Url);
return ("DD_INTERNAL_CRASHTRACKING_OUTPUT", reportFile.Url);
}

private static void CopyDirectory(string source, string destination)
Expand Down
Loading