Address comment

DataDog · Jun 21, 2024 · b7c3066 · b7c3066
1 parent e067b37
commit b7c3066
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 8 deletions.
diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/LinuxStackFramesCollector.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/LinuxStackFramesCollector.cpp
@@ -38,7 +38,10 @@ LinuxStackFramesCollector::LinuxStackFramesCollector(
     _errorStatistics{},
     _useBacktrace2{configuration->UseBacktrace2()}
 {
-    _signalManager->RegisterHandler(LinuxStackFramesCollector::CollectStackSampleSignalHandler);
+    if (_signalManager != nullptr)
+    {
+        _signalManager->RegisterHandler(LinuxStackFramesCollector::CollectStackSampleSignalHandler);
+    }
 }
 
 LinuxStackFramesCollector::~LinuxStackFramesCollector()
@@ -87,6 +90,10 @@ StackSnapshotResultBuffer* LinuxStackFramesCollector::CollectStackSampleImplemen
 {
     long errorCode;
 
+    // If there a timer associated to the managed thread, we have to disarm it.
+    // Otherwise, the CPU consumption to collect the callstack, will be accounted as "user app CPU time"
+    auto timerId = pThreadInfo->GetTimerId();
+
     if (selfCollect)
     {
         // In case we are self-unwinding, we do not want to be interrupted by the signal-based profilers (walltime and cpu)
@@ -103,15 +110,12 @@ StackSnapshotResultBuffer* LinuxStackFramesCollector::CollectStackSampleImplemen
     }
     else
     {
-        if (!_signalManager->IsHandlerInPlace())
+        if (_signalManager == nullptr || !_signalManager->IsHandlerInPlace())
         {
             *pHR = E_FAIL;
             return GetStackSnapshotResult();
         }
 
-        // If there a timer associated to the managed thread, we have to disarm it.
-        // Otherwise, the CPU consumption to collect the callstack, will be accounted as "user app CPU time"
-        auto timerId = pThreadInfo->GetTimerId();
         struct itimerspec old;
 
         if (timerId != -1)

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.cpp
@@ -37,7 +37,8 @@ ProfilerSignalManager* ProfilerSignalManager::Get(int signal)
 
     if (signal < 1 || signal > 31)
     {
-        throw std::invalid_argument(std::string("Signal argument is invalid ") + "(" + std::to_string(signal) + "). Value must be: 1 <= signal <= 31");
+        Log::Info("Signal argument is invalid (", std::to_string(signal), " aka ", strsignal(signal), "). Value must be: 1 <= signal <= 31");
+        return nullptr;
     }
 
     auto* manager = &signalManagers[signal - 1]; // 0-based array
@@ -88,6 +89,7 @@ bool ProfilerSignalManager::UnRegisterHandler()
         return false;
     }
 
+    _isHandlerInPlace = false;
     return true;
 }
 

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.cpp
@@ -71,7 +71,14 @@ const char* TimerCreateCpuProfiler::GetName()
 
 bool TimerCreateCpuProfiler::StartImpl()
 {
-    // If the signal is highjacked, what to do?
+    if (_pSignalManager == nullptr)
+    {
+        Log::Info("Profiler Signal manager was not correctly initialized (see previous messages).",
+                  "timer_create-based CPU profiler is disabled.");
+        return false;
+    }
+
+    // If the signal is hijacked, what to do?
     auto registered = _pSignalManager->RegisterHandler(TimerCreateCpuProfiler::CollectStackSampleSignalHandler);
 
     if (registered)

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/StackSamplerLoop.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/StackSamplerLoop.cpp
@@ -75,7 +75,7 @@ StackSamplerLoop::StackSamplerLoop(
     Log::Info("Wall time sampled threads = ", _walltimeThreadsThreshold);
     Log::Info("Max CodeHotspots sampled threads = ", _codeHotspotsThreadsThreshold);
     Log::Info("Max CPU sampled threads = ", _cpuThreadsThreshold);
-    Log::Info("Cpu profiler is ", (_isCpuEnabled) ? "enabled" : "disabled");
+    Log::Info("Manual Cpu profiler is ", (_isCpuEnabled) ? "enabled" : "disabled");
     Log::Info("Wall-time profiler is ", (_isWalltimeEnabled) ? "enabled" : "disabled");
 
     _pCorProfilerInfo->AddRef();

diff --git a/profiler/test/Datadog.Profiler.Native.Tests/ConfigurationTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/ConfigurationTest.cpp
@@ -973,6 +973,12 @@ TEST(ConfigurationTest, CheckDefaultCpuProfilerType)
     ASSERT_THAT(configuration.GetCpuProfilerType(), CpuProfilerType::ManualCpuTime);
 }
 
+TEST(ConfigurationTest, CheckDefaultCpuProfilerTypeWhenEnvVarNotSet)
+{
+    auto configuration = Configuration{};
+    ASSERT_THAT(configuration.GetCpuProfilerType(), CpuProfilerType::ManualCpuTime);
+}
+
 TEST(ConfigurationTest, CheckUnknownCpuProfilerType)
 {
     EnvironmentHelper::EnvironmentVariable ar(EnvironmentVariables::CpuProfilerType, WStr("UnknownCpuProfilerType"));