Address comment

DataDog · Jul 5, 2024 · 66f6868 · 66f6868
1 parent d4b30bd
commit 66f6868
Show file tree

Hide file tree

Showing 6 changed files with 37 additions and 29 deletions.
diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.cpp
@@ -179,6 +179,12 @@ bool ProfilerSignalManager::SetupSignalHandler()
 void ProfilerSignalManager::SignalHandler(int signal, siginfo_t* info, void* context)
 {
     auto* signalManager = Get(signal);
+
+    if (signalManager == nullptr) [[unlikely]]
+    {
+        return;
+    }
+
     if (!signalManager->CallCustomHandler(signal, info, context))
     {
         signalManager->CallOrignalHandler(signal, info, context);

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.cpp
@@ -28,10 +28,10 @@ TimerCreateCpuProfiler::TimerCreateCpuProfiler(
     _pManagedThreadsList{pManagedThreadsList},
     _pProvider{pProvider},
     _callstackProvider{std::move(callstackProvider)},
-    _serviceState{ServiceState::Initialized},
     _samplingInterval{pConfiguration->GetCpuProfilingInterval()}
 {
     Log::Info("Cpu profiling interval: ", _samplingInterval.count(), "ms");
+    Log::Info("timer_create Cpu profiler is enabled");
 }
 
 TimerCreateCpuProfiler::~TimerCreateCpuProfiler()

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.h
@@ -49,18 +49,10 @@ class TimerCreateCpuProfiler : public ServiceBase
     bool StartImpl() override;
     bool StopImpl() override;
 
-    enum class ServiceState
-    {
-        Started,
-        Stopped,
-        Initialized
-    };
-
     ProfilerSignalManager* _pSignalManager;
     IManagedThreadList* _pManagedThreadsList;
     CpuTimeProvider* _pProvider;
     CallstackProvider _callstackProvider;
-    std::atomic<ServiceState> _serviceState;
     std::chrono::milliseconds _samplingInterval;
     std::shared_mutex _registerLock;
 };
diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Configuration.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Configuration.cpp
@@ -15,8 +15,6 @@
 #include "shared/src/native-src/string.h"
 #include "shared/src/native-src/util.h"
 
-using namespace std::literals::chrono_literals;
-
 std::string const Configuration::DefaultDevSite = "datad0g.com";
 std::string const Configuration::DefaultProdSite = "datadoghq.com";
 std::string const Configuration::DefaultVersion = "Unspecified-Version";
@@ -78,6 +76,7 @@ Configuration::Configuration()
     // Check CI Visibility mode
     _isCIVisibilityEnabled = GetEnvironmentValue(EnvironmentVariables::CIVisibilityEnabled, false);
     _internalCIVisibilitySpanId = uint64_t{0};
+    _cpuProfilingInterval = ExtractCpuProfilingInterval();
     if (_isCIVisibilityEnabled)
     {
         // We cannot write 0ull instead of std::uint64_t{0} because on Windows, compiling in x64, std::uint64_t == unsigned long long.
@@ -87,14 +86,15 @@ Configuration::Configuration()
 
         // If we detect CI Visibility we allow to reduce the minimum ms in sampling rate down to 1ms.
         _cpuWallTimeSamplingRate = ExtractCpuWallTimeSamplingRate(1);
+        // for timer_create based profiling
+        _cpuProfilingInterval = ExtractCpuProfilingInterval(1ms);
     }
 
     _isEtwEnabled = GetEnvironmentValue(EnvironmentVariables::EtwEnabled, false);
     _deploymentMode = GetEnvironmentValue(EnvironmentVariables::SsiDeployed, DeploymentMode::Manual);
     _isEtwLoggingEnabled = GetEnvironmentValue(EnvironmentVariables::EtwLoggingEnabled, false);
     _enablementStatus = ExtractEnablementStatus();
     _cpuProfilerType = GetEnvironmentValue(EnvironmentVariables::CpuProfilerType, CpuProfilerType::ManualCpuTime);
-    _cpuProfilingInterval = ExtractCpuProfilingInterval();
 }
 
 fs::path Configuration::ExtractLogDirectory()
@@ -459,16 +459,12 @@ std::chrono::seconds Configuration::ExtractUploadInterval()
     return GetDefaultUploadInterval();
 }
 
-std::chrono::milliseconds Configuration::ExtractCpuProfilingInterval()
+std::chrono::milliseconds Configuration::ExtractCpuProfilingInterval(std::chrono::milliseconds minimum)
 {
-    auto r = shared::GetEnvironmentValue(EnvironmentVariables::CpuProfilingInterval);
-    int32_t interval;
-    if (TryParse(r, interval))
-    {
-        return std::max(std::chrono::milliseconds(interval), DefaultCpuProfilingInterval);
-    }
-
-    return DefaultCpuProfilingInterval;
+    // For normal path (no CI-visibility), 9ms is the default and lowest value we can have
+    // for CI-Visibility we allow it to be less
+    auto interval = GetEnvironmentValue(EnvironmentVariables::CpuProfilingInterval, DefaultCpuProfilingInterval);
+    return std::max(interval, minimum);
 }
 
 std::chrono::nanoseconds Configuration::ExtractCpuWallTimeSamplingRate(int minimum)
@@ -631,6 +627,17 @@ static bool convert_to(shared::WSTRING const& s, double& result)
     return (errno != ERANGE);
 }
 
+static bool convert_to(shared::WSTRING const& s, std::chrono::milliseconds& result)
+{
+    std::uint64_t value;
+    auto parsed = TryParse(s, value);
+    if (parsed)
+    {
+        result = std::chrono::milliseconds(value);
+    }
+    return parsed;
+}
+
 static bool convert_to(shared::WSTRING const& s, DeploymentMode& result)
 {
     // if we reach here it means the env var exists

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Configuration.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Configuration.h
@@ -16,6 +16,8 @@
 #include "shared/src/native-src/dd_filesystem.hpp"
 // namespace fs is an alias defined in "dd_filesystem.hpp"
 
+using namespace std::literals::chrono_literals;
+
 class Configuration final : public IConfiguration
 {
 public:
@@ -82,7 +84,7 @@ class Configuration final : public IConfiguration
     static std::string GetDefaultSite();
     static std::string ExtractSite();
     static std::chrono::seconds ExtractUploadInterval();
-    static std::chrono::milliseconds ExtractCpuProfilingInterval();
+    static std::chrono::milliseconds ExtractCpuProfilingInterval(std::chrono::milliseconds minimum = DefaultCpuProfilingInterval);
     static fs::path GetDefaultLogDirectoryPath();
     static fs::path GetApmBaseDirectory();
     static fs::path ExtractLogDirectory();

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp
@@ -451,7 +451,7 @@ bool CorProfilerCallback::InitializeServices()
         CallstackProvider(_memoryResourceManager.GetSynchronizedPool(100, Callstack::MaxSize)));
 
 #ifdef LINUX
-    if (_pConfiguration->GetCpuProfilerType() == CpuProfilerType::TimerCreate)
+    if (_pConfiguration->IsCpuProfilingEnabled() && _pConfiguration->GetCpuProfilerType() == CpuProfilerType::TimerCreate)
     {
         auto const useMmap = true;
         _pCpuProfiler = RegisterService<TimerCreateCpuProfiler>(
@@ -1196,12 +1196,13 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::Initialize(IUnknown* corProfilerI
 
         COR_PRF_EVENTPIPE_PROVIDER_CONFIG providers[] =
             {
-                {WStr("Microsoft-Windows-DotNETRuntime"),
-                 activatedKeywords,
-                 verbosity,
-                 nullptr
-            }
-        };
+                {
+                    WStr("Microsoft-Windows-DotNETRuntime"),
+                    activatedKeywords,
+                    verbosity,
+                    nullptr
+                }
+            };
 
         hr = _pCorProfilerInfoEvents->EventPipeStartSession(
             sizeof(providers) / sizeof(providers[0]),