Add metric on lock failure for IsManaged

Author: gleocadie

profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp

@@ -1567,7 +1567,7 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::Initialize(IUnknown* corProfilerI
     // Use managed code cache
     if (_pConfiguration->UseManagedCodeCache())
     {
-        _managedCodeCache = std::make_unique<ManagedCodeCache>(_pCorProfilerInfo);
+        _managedCodeCache = std::make_unique<ManagedCodeCache>(_pCorProfilerInfo, _metricsRegistry);
         if (!_managedCodeCache->Initialize())
         {
             Log::Error("Failed to initialize managed code cache. The profiler will not run.");

profiler/src/ProfilerEngine/Datadog.Profiler.Native/ManagedCodeCache.cpp

@@ -4,6 +4,8 @@
 #include "ManagedCodeCache.h"
 
 #include "Configuration.h"
+#include "CounterMetric.h"
+#include "MetricsRegistry.h"
 
 #include <algorithm>
 #include <chrono>
@@ -88,8 +90,9 @@ struct IMAGE_NT_HEADERS_GENERIC
     WORD    Magic;
 };
 
-ManagedCodeCache::ManagedCodeCache(ICorProfilerInfo4* pProfilerInfo)
-    : _profilerInfo(pProfilerInfo)
+ManagedCodeCache::ManagedCodeCache(ICorProfilerInfo4* pProfilerInfo, MetricsRegistry& metricsRegistry)
+    : _profilerInfo(pProfilerInfo),
+      _lockFailureMetric(metricsRegistry.GetOrRegister<CounterMetric>("dotnet_managed_code_cache_lock_failures"))
 {
 }
 
@@ -239,7 +242,14 @@ std::optional<bool> ManagedCodeCache::IsManaged(std::uintptr_t ip) const noexcep
     // Best effort to identify an instruction pointer. When called from a signal
     // handler, IsManagedImpl uses a time-based lock acquire (bounded wait) and
     // returns std::nullopt if a lock cannot be acquired within the timeout.
-    return IsManagedImpl(ip);
+    auto result = IsManagedImpl(ip);
+    if (!result.has_value())
+    {
+        // Lock could not be acquired within the timeout: record it. Incr() is an
+        // atomic increment, so this is safe to call from a signal handler.
+        _lockFailureMetric->Incr();
+    }
+    return result;
 }
 
 std::optional<bool> ManagedCodeCache::IsManagedImpl(std::uintptr_t ip) const noexcept

profiler/src/ProfilerEngine/Datadog.Profiler.Native/ManagedCodeCache.h

@@ -4,6 +4,7 @@
 #pragma once
 
 #include <atomic>
+#include <memory>
 #include <vector>
 #include <unordered_map>
 #include <unordered_set>
@@ -17,6 +18,9 @@
 #include "cor.h"
 #include "corprof.h"
 
+class CounterMetric;
+class MetricsRegistry;
+
 // Represents a single contiguous code range
 struct CodeRange {
     UINT_PTR startAddress;
@@ -73,7 +77,7 @@ class ManagedCodeCache {
 public:
     static constexpr FunctionID InvalidFunctionId = -1;
 
-    explicit ManagedCodeCache(ICorProfilerInfo4* pProfilerInfo);
+    ManagedCodeCache(ICorProfilerInfo4* pProfilerInfo, MetricsRegistry& metricsRegistry);
     ~ManagedCodeCache();
 
     // Signal-safe lookup methods (no allocation)
@@ -168,6 +172,10 @@ class ManagedCodeCache {
     // Profiler interface (ICorProfilerInfo4 is available in .NET Framework 4.5+)
     ICorProfilerInfo4* _profilerInfo;
 
+    // Counts how many times IsManaged failed to acquire a lock within the timeout
+    // (signal-handler read path backing off instead of blocking).
+    std::shared_ptr<CounterMetric> _lockFailureMetric;
+
     std::optional<FunctionID> GetFunctionIdImpl(std::uintptr_t ip) const noexcept;
     std::optional<bool> IsCodeInR2RModule(std::uintptr_t ip, bool signalSafe) const noexcept;
     std::optional<FunctionID> GetFunctionFromIP_Original(std::uintptr_t ip) noexcept;

profiler/test/Datadog.Profiler.Native.Tests/FrameStoreTest.cpp

@@ -6,6 +6,7 @@
 
 #include "FrameStore.h"
 #include "ManagedCodeCache.h"
+#include "MetricsRegistry.h"
 #include "MockProfilerInfo.h"
 
 #include <memory>
@@ -86,7 +87,8 @@ TEST(FrameStoreTest, GetFrame_WithCache_NativeIp_ReturnsNotResolvedAndDropped)
 
     // Empty cache => any IP resolves to InvalidFunctionId (there are no registered
     // JIT ranges and no R2R modules), which is exactly the "native IP" case.
-    auto cache = std::make_unique<ManagedCodeCache>(&mockProfiler);
+    MetricsRegistry metricsRegistry;
+    auto cache = std::make_unique<ManagedCodeCache>(&mockProfiler, metricsRegistry);
     cache->Initialize();
 
     FrameStore frameStore(
@@ -161,7 +163,8 @@ TEST(FrameStoreTest, GetFrame_WithCache_CachedPathNullopt_ReturnsResolvedPlaceho
 {
     auto mockProfiler = MockProfilerInfo{};
 
-    auto cache = std::make_unique<ManagedCodeCache>(&mockProfiler);
+    MetricsRegistry metricsRegistry;
+    auto cache = std::make_unique<ManagedCodeCache>(&mockProfiler, metricsRegistry);
     cache->Initialize();
 
     // Register an R2R module range so GetFunctionId falls through to

profiler/test/Datadog.Profiler.Native.Tests/LinuxStackFramesCollectorTest.cpp

@@ -166,7 +166,7 @@ class LinuxStackFramesCollectorFixture : public ::testing::Test
 
 #ifdef ARM64
         // TODO maybe a mock of ICorProfilerInfo to avoid crashing
-        _pManagedCodeCache = std::make_unique<ManagedCodeCache>(nullptr);
+        _pManagedCodeCache = std::make_unique<ManagedCodeCache>(nullptr, _metricsRegistry);
         _pUnwinder = std::make_unique<HybridUnwinder>(_pManagedCodeCache.get());
 #else
         _pUnwinder = std::make_unique<Backtrace2Unwinder>();

profiler/test/Datadog.Profiler.Native.Tests/ManagedCodeCacheTest.cpp

@@ -3,7 +3,9 @@
 
 #include "gtest/gtest.h"
 #include "gmock/gmock.h"
+#include "CounterMetric.h"
 #include "ManagedCodeCache.h"
+#include "MetricsRegistry.h"
 #include "MockProfilerInfo.h"
 
 #include <thread>
@@ -15,11 +17,12 @@ using namespace testing;
 class ManagedCodeCacheTest : public Test {
 protected:
     MockProfilerInfo* mockProfiler;
+    MetricsRegistry metricsRegistry;
     std::unique_ptr<ManagedCodeCache> cache;
 
     void SetUp() override {
         mockProfiler = new MockProfilerInfo();
-        cache = std::make_unique<ManagedCodeCache>(mockProfiler);
+        cache = std::make_unique<ManagedCodeCache>(mockProfiler, metricsRegistry);
         cache->Initialize();
     }
 
@@ -44,6 +47,15 @@ class ManagedCodeCacheTest : public Test {
                 return S_OK;
             });
     }
+
+    // Helper: read (and reset) the IsManaged lock-failure counter.
+    // GetMetrics() exchanges the underlying atomic to 0, so each call returns the
+    // number of failures recorded since the previous read.
+    uint64_t GetLockFailureCount() {
+        auto metric = metricsRegistry.GetOrRegister<CounterMetric>("dotnet_managed_code_cache_lock_failures");
+        auto metrics = metric->GetMetrics();
+        return metrics.empty() ? 0 : static_cast<uint64_t>(metrics.front().second);
+    }
 };
 
 // Test: Single code range
@@ -404,6 +416,47 @@ TEST_F(ManagedCodeCacheTest, IsManaged_WriterHoldsPagesMutex_ReturnsNullopt) {
     EXPECT_TRUE(afterRelease.value());
 }
 
+// Test: IsManaged increments the lock-failure metric when it cannot acquire the
+// pages mutex within the timeout, and leaves it untouched on the success path.
+TEST_F(ManagedCodeCacheTest, IsManaged_LockAcquisitionFailure_IncrementsMetric) {
+    FunctionID testFuncId = 0x4321;
+    uintptr_t codeStart = 0xD000;
+    ULONG32 codeSize = 0x100;
+
+    SetupMockCodeInfo(testFuncId, codeStart, codeSize);
+    cache->AddFunction(testFuncId);
+
+    // Success path must not increment the failure metric.
+    ASSERT_TRUE(cache->IsManaged(codeStart + 0x50).value_or(false));
+    EXPECT_EQ(0u, GetLockFailureCount());
+
+    // Hold the pages mutex exclusively so IsManaged times out and returns nullopt.
+    std::atomic<bool> writerHoldsLock{false};
+    std::atomic<bool> readerDone{false};
+    std::thread writer([&]() {
+        auto lock = cache->LockPagesMutexExclusiveForTest();
+        writerHoldsLock.store(true);
+        while (!readerDone.load())
+        {
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+        }
+    });
+
+    while (!writerHoldsLock.load())
+    {
+        std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
+
+    auto contended = cache->IsManaged(codeStart + 0x50);
+    EXPECT_FALSE(contended.has_value());
+
+    readerDone.store(true);
+    writer.join();
+
+    // The failed acquisition must have been recorded exactly once.
+    EXPECT_EQ(1u, GetLockFailureCount());
+}
+
 // Test: IsManaged must also return nullopt when the writer holds the modules
 // mutex exclusively and the probed IP is not in any JIT page (i.e. the code path
 // falls through to IsCodeInR2RModule(ip, /*signalSafe*/true)).