From a8ca3d50c373b269bd803968de41399721758dad Mon Sep 17 00:00:00 2001
From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Date: Tue, 7 Jul 2026 19:09:02 +0100
Subject: [PATCH] fix(telemetry): node-health detail panels + OTelCollector
 hook deadlock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- node-health.json: repoint the two "All Jobs" detail panels from the
  __name__ regex over old xrpld_<job>_milliseconds_bucket names to the
  native xrpld_job_running/queued_duration_us_bucket histograms, grouping
  by the job_type label and legending on {{job_type}}.
- OTelCollector::callHooks: copy the hook list under mutex_ and invoke
  handlers outside the lock. A handler can drop the last reference to an
  OTelHookImpl, whose destructor calls removeHook() and re-acquires the
  non-recursive mutex_ — invoking handlers under the lock could deadlock.
- ~OTelGaugeImpl: document that the SDK ObservableRegistry serializes
  RemoveCallback against the Observe() callback pass with one mutex, so
  callback removal is synchronous and gaugeCallback cannot run on a
  dangling pointer after the destructor.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 .../grafana/dashboards/node-health.json         |  8 ++++----
 src/libxrpl/beast/insight/OTelCollector.cpp     | 17 +++++++++++++++--
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/docker/telemetry/grafana/dashboards/node-health.json b/docker/telemetry/grafana/dashboards/node-health.json
index c249dcae85..25e34e39d6 100644
--- a/docker/telemetry/grafana/dashboards/node-health.json
+++ b/docker/telemetry/grafana/dashboards/node-health.json
@@ -834,8 +834,8 @@
           "datasource": {
             "type": "prometheus"
           },
-          "expr": "histogram_quantile($quantile, sum by (le, __name__) (rate({__name__=~\"xrpld_(makeFetchPack|publishAcqLedger|untrustedValidation|manifest|localTransaction|ledgerReplayRequest|ledgerRequest|untrustedProposal|ledgerReplayTask|ledgerData|clientCommand|clientSubscribe|clientFeeChange|clientConsensus|clientAccountHistory|clientRPC|clientWebsocket|RPC|updatePaths|transaction|batch|advanceLedger|publishNewLedger|fetchTxnData|writeAhead|trustedValidation|writeObjects|acceptLedger|trustedProposal|sweep|clusterReport|heartbeat|administration|handleHaveTransactions|doTransactions)_milliseconds_bucket\", exported_instance=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))",
-          "legendFormat": "{{__name__}}"
+          "expr": "histogram_quantile($quantile, sum by (le, job_type) (rate(xrpld_job_running_duration_us_bucket{exported_instance=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))",
+          "legendFormat": "{{job_type}}"
         }
       ],
       "fieldConfig": {
@@ -873,8 +873,8 @@
           "datasource": {
             "type": "prometheus"
           },
-          "expr": "histogram_quantile($quantile, sum by (le, __name__) (rate({__name__=~\"xrpld_(makeFetchPack_q|publishAcqLedger_q|untrustedValidation_q|manifest_q|localTransaction_q|ledgerReplayRequest_q|ledgerRequest_q|untrustedProposal_q|ledgerReplayTask_q|ledgerData_q|clientCommand_q|clientSubscribe_q|clientFeeChange_q|clientConsensus_q|clientAccountHistory_q|clientRPC_q|clientWebsocket_q|RPC_q|updatePaths_q|transaction_q|batch_q|advanceLedger_q|publishNewLedger_q|fetchTxnData_q|writeAhead_q|trustedValidation_q|writeObjects_q|acceptLedger_q|trustedProposal_q|sweep_q|clusterReport_q|heartbeat_q|administration_q|handleHaveTransactions_q|doTransactions_q)_milliseconds_bucket\", exported_instance=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))",
-          "legendFormat": "{{__name__}}"
+          "expr": "histogram_quantile($quantile, sum by (le, job_type) (rate(xrpld_job_queued_duration_us_bucket{exported_instance=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))",
+          "legendFormat": "{{job_type}}"
         }
       ],
       "fieldConfig": {
diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp
index 8ae1ced399..77120dbd36 100644
--- a/src/libxrpl/beast/insight/OTelCollector.cpp
+++ b/src/libxrpl/beast/insight/OTelCollector.cpp
@@ -614,6 +614,11 @@ OTelGaugeImpl::gaugeCallback(opentelemetry::metrics::ObserverResult result, void
 
 OTelGaugeImpl::~OTelGaugeImpl()
 {
+    // RemoveCallback must run before this object is destroyed so the SDK
+    // collection thread cannot invoke gaugeCallback on a dangling `this`.
+    // The SDK's ObservableRegistry guards its callback list and the Observe()
+    // pass with the same mutex, so RemoveCallback cannot return while a
+    // callback for this instrument is in flight — removal is synchronous.
     gauge_->RemoveCallback(gaugeCallback, this);
     collector_->removeGauge(this);
 }
@@ -782,8 +787,16 @@ OTelCollectorImp::callHooks()
     if (!lastHookCallMs_.compare_exchange_strong(last, now, std::memory_order_acq_rel))
         return;  // Another thread won the race.
 
-    std::scoped_lock const lock(mutex_);
-    for (auto* hook : hooks_)
+    // Copy the hook list under the lock, then invoke handlers outside it.
+    // A handler may drop the last reference to an OTelHookImpl, whose
+    // destructor calls removeHook() and re-acquires mutex_; invoking
+    // handlers while holding the (non-recursive) lock would deadlock.
+    std::vector<OTelHookImpl*> hooks;
+    {
+        std::scoped_lock const lock(mutex_);
+        hooks = hooks_;
+    }
+    for (auto* hook : hooks)
         hook->callHandler();
 }