Skip to content

Commit

Permalink
Normalize the environment variable names used by crashtracking (#5898)
Browse files Browse the repository at this point in the history 
Normalize the environment variable names used by crashtracking.

`DD_CRASHTRACKING_*`: environment variables expected to be set by the
customers. Currently:
- `DD_CRASHTRACKING_ENABLED`: enables or disables crashtracking
(default: enabled)

`DD_INTERNAL_CRASHTRACKING_*`: environment variables used by the tracer
infrastructure and/or tests. Currently:
- `DD_INTERNAL_CRASHTRACKING_PASSTHROUGH`: automatically set to decide
whether the real createdump should be called or not
- `DD_INTERNAL_CRASHTRACKING_OUTPUT`: save the crash report to a file
instead of using telemetry

Now that other languages are implementing the feature, it's important to
have consistent names.

`DD_TRACE_CRASH_HANDLER` is not needed anymore, removed it.
Removed the profiler tests that relied on `DD_TRACE_CRASH_HANDLER`
(they're not needed now that we have crashtracking tests).

The existing tests were updated.
  • Loading branch information
@kevingosse @andrewlock
kevingosse authored and andrewlock committed Aug 23, 2024
1 parent 9f69b4b commit 124ee45
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 115 deletions.
149 changes: 73 additions & 76 deletions profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/functions_to_wrap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,19 @@ char* getSubfolder(const char* path)

static char* originalMiniDumpName = NULL;
static const char* datadogCrashMarker = "datadog_crashtracking";
#define DD_CRASHTRACKING_ENABLED "DD_CRASHTRACKING_ENABLED"
#define DD_INTERNAL_CRASHTRACKING_PASSTHROUGH "DD_INTERNAL_CRASHTRACKING_PASSTHROUGH"
#define DOTNET_DbgEnableMiniDump "DOTNET_DbgEnableMiniDump"
#define COMPlus_DbgEnableMiniDump "COMPlus_DbgEnableMiniDump"
#define DOTNET_DbgMiniDumpName "DOTNET_DbgMiniDumpName"
#define COMPlus_DbgMiniDumpName "COMPlus_DbgMiniDumpName"

__attribute__((constructor))
void initLibrary(void)
{
const char* crashHandlerEnabled = getenv("DD_TRACE_CRASH_HANDLER_ENABLED");
check_init();

const char* crashHandlerEnabled = getenv(DD_CRASHTRACKING_ENABLED);

if (crashHandlerEnabled != NULL)
{
Expand All @@ -196,105 +204,94 @@ 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')
{
char* enableMiniDump = getenv("DOTNET_DbgEnableMiniDump");
char* enableMiniDump = getenv(DOTNET_DbgEnableMiniDump);

if (enableMiniDump == NULL)
{
enableMiniDump = getenv("COMPlus_DbgEnableMiniDump");
enableMiniDump = getenv(COMPlus_DbgEnableMiniDump);
}

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);

if (passthrough == NULL || passthrough[0] == '\0')
{
Expand All @@ -303,25 +300,25 @@ 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
{
// If DOTNET_DbgEnableMiniDump is not set, we set it so that the crash handler is called,
// 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(COMPlus_DbgEnableMiniDump, "1", 1);
setenv(DOTNET_DbgEnableMiniDump, "1", 1);
setenv(DD_INTERNAL_CRASHTRACKING_PASSTHROUGH, "0", 1);
}

originalMiniDumpName = getenv("DOTNET_DbgMiniDumpName");
originalMiniDumpName = getenv(DOTNET_DbgMiniDumpName);
if (originalMiniDumpName == NULL)
{
originalMiniDumpName = getenv("COMPlus_DbgMiniDumpName");
originalMiniDumpName = getenv(COMPlus_DbgMiniDumpName);
}
setenv("COMPlus_DbgMiniDumpName", datadogCrashMarker, 1);
setenv("DOTNET_DbgMiniDumpName", datadogCrashMarker, 1);
setenv(COMPlus_DbgMiniDumpName, datadogCrashMarker, 1);
setenv(DOTNET_DbgMiniDumpName, datadogCrashMarker, 1);
}
}

Expand Down
34 changes: 1 addition & 33 deletions profiler/test/Datadog.Profiler.IntegrationTests/LinuxOnly/WrapperLibraryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,29 +82,10 @@ public void EnsureAppDoesNotCrashIfProfilerDeactivateAndTracerActivated(string a
}

[TestAppFact("Samples.ExceptionGenerator")]
public void RedirectCrashHandler(string appName, string framework, string appAssembly)
public void GenerateDumpIfDbgRequested(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)
{
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);
Expand All @@ -117,18 +98,5 @@ public void DontRedirectCrashHandlerIfPathNotSet(string appName, string framewor
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
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
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

0 comments on commit 124ee45

Please sign in to comment.