fix(telemetry): make this branch's files compile clean with telemetry off

The metric macros discard their arguments when telemetry is compiled out, so a
file's used-symbol set differs from the one its includes and declarations were
written for. That produced eleven clang-tidy errors here, all from that cause.

- guard <atomic>, <functional> and <memory> in MetricsRegistry.h, and
  <algorithm> and <optional> in DatabaseConfig_test.cpp, to match their uses
- include Journal.h directly in MetricsRegistry.cpp, whose constructor takes a
  beast::Journal by value in both configurations
- compile out recordGetObjectMetrics, its declaration, its call site and the
  two clock reads that feed it, since every statement in it records a metric
- drop two duplicate includes and rename a function-local static constant to
  kEmpty
This commit is contained in:
Pratik Mankawde
2026-08-26 18:07:39 +01:00
parent 879ad9fbe0
commit 7253aa787b
6 changed files with 47 additions and 17 deletions

View File

@@ -33,18 +33,23 @@
#include <xrpl/nodestore/detail/DatabaseRotatingImp.h>
#include <xrpl/rdb/DatabaseCon.h>
#include <algorithm>
#include <atomic>
#include <chrono>
#include <cstddef>
#include <cstdint>
#include <memory>
#include <optional>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
#ifdef XRPL_ENABLE_TELEMETRY
// std::ranges::sort / std::ranges::find_if and std::optional / std::nullopt are
// used only by the gauge-helper suite below, so they are guarded like its uses.
#include <algorithm>
#include <optional>
#endif
namespace xrpl::node_store {
class DatabaseConfig_test : public beast::unit_test::Suite

View File

@@ -428,10 +428,8 @@ TEST(MetricsRegistryScaledMean, default_scale_is_one)
#include <boost/asio/io_context.hpp>
#include <optional>
#include <stdexcept>
#include <string>
#include <string_view>
using namespace xrpl;
@@ -699,8 +697,8 @@ public:
[[nodiscard]] std::optional<uint256> const&
getTrapTxID() const override
{
static std::optional<uint256> const empty;
return empty;
static std::optional<uint256> const kEmpty;
return kEmpty;
}
DatabaseCon&
getWalletDB() override

View File

@@ -76,7 +76,6 @@
#include <xrpl/server/NetworkOPs.h>
#include <xrpl/shamap/SHAMap.h>
#include <xrpl/shamap/SHAMapNodeID.h>
#include <xrpl/telemetry/GetObjectMetricNames.h>
#include <xrpl/telemetry/SpanGuard.h>
#include <xrpl/telemetry/SpanNames.h>
#include <xrpl/tx/apply.h>
@@ -117,6 +116,13 @@
#include <utility>
#include <vector>
#ifdef XRPL_ENABLE_TELEMETRY
// The TMGetObjectByHash metric names and label keys. Every use of them sits in
// an XRPL_METRIC_* argument list, and those macros expand to nothing when
// telemetry is off, so the include is guarded like its uses.
#include <xrpl/telemetry/GetObjectMetricNames.h>
#endif
using namespace std::chrono_literals;
namespace xrpl {
@@ -2857,10 +2863,13 @@ PeerImp::processGetObjectByHash(std::shared_ptr<protocol::TMGetObjectByHash> con
int const requested = packet.objects_size();
int const iterLimit = std::min(requested, tuning::kHardMaxReplyNodes);
#ifdef XRPL_ENABLE_TELEMETRY
// Time the whole loop once, not each iteration: the loop can run up to
// kHardMaxReplyNodes times, so per-iteration clock reads would cost more
// than the lookups they measure.
// than the lookups they measure. Both clock reads serve only the metric
// recorded below, so neither happens when telemetry is compiled out.
auto const lookupStart = std::chrono::steady_clock::now();
#endif
for (int i = 0; i < iterLimit; ++i)
{
@@ -2886,8 +2895,12 @@ PeerImp::processGetObjectByHash(std::shared_ptr<protocol::TMGetObjectByHash> con
newObj.set_ledgerseq(obj.ledgerseq());
}
#ifdef XRPL_ENABLE_TELEMETRY
// Measured here rather than at the call below, which would fold the fee
// computation and charge() into the reported lookup latency.
auto const lookupElapsed = std::chrono::duration_cast<std::chrono::microseconds>(
std::chrono::steady_clock::now() - lookupStart);
#endif
// Apply work-proportional charge. `charge()` posts the disconnect
// step (if any) back to strand_, so it is safe to call from this
@@ -2902,12 +2915,15 @@ PeerImp::processGetObjectByHash(std::shared_ptr<protocol::TMGetObjectByHash> con
resource::Charge const fee = computeGetObjectByHashFee(requested, reply.objects_size());
charge(fee, "processed get object by hash request");
#ifdef XRPL_ENABLE_TELEMETRY
recordGetObjectMetrics(requested, reply.objects_size(), lookupElapsed, fee);
#endif
JLOG(pJournal_.trace()) << "GetObj: " << reply.objects_size() << " of " << requested;
send(std::make_shared<Message>(reply, protocol::mtGET_OBJECTS));
}
#ifdef XRPL_ENABLE_TELEMETRY
void
PeerImp::recordGetObjectMetrics(
int const requested,
@@ -2936,9 +2952,9 @@ PeerImp::recordGetObjectMetrics(
// negative -- the counter takes an unsigned amount, where a wrap would
// read as ~1.8e19 rather than as an error.
//
// Written as two calls rather than a loop over a {hit, miss} pair: the
// macros expand to empty statements in a telemetry-off build, which would
// leave a loop's induction variable unused and fail the -Werror build.
// Written as two calls rather than a loop over a {hit, miss} pair: the two
// amounts come from different expressions, so there is no single value to
// iterate over.
XRPL_METRIC_COUNTER_ADD_LABELED(
app_,
kGetObjectLookupsTotal,
@@ -2953,6 +2969,7 @@ PeerImp::recordGetObjectMetrics(
static_cast<std::uint64_t>(std::max(0, requested - found)),
{{kLabelResult, std::string(kResultMiss)}});
}
#endif // XRPL_ENABLE_TELEMETRY
void
PeerImp::onMessage(std::shared_ptr<protocol::TMHaveTransactions> const& m)

View File

@@ -701,6 +701,7 @@ private:
std::shared_ptr<protocol::TMGetLedger> const& m,
std::vector<SHAMapNodeID> nodeIDs);
#ifdef XRPL_ENABLE_TELEMETRY
/**
* Record the OTel metrics for one completed `TMGetObjectByHash` request.
*
@@ -711,8 +712,9 @@ private:
*
* Records `getobject_request_objects`, `getobject_lookup_us`,
* `getobject_charge`, and both label values of
* `getobject_lookups_total`. Compiles to nothing when telemetry is
* disabled, because the `XRPL_METRIC_*` macros do.
* `getobject_lookups_total`. Every statement is an `XRPL_METRIC_*` record,
* so the whole method — and its one call site — is compiled out when
* telemetry is disabled rather than left as an empty function.
*
* @param requested Objects the peer asked for (`objects_size()`).
* @param found Objects returned, i.e. the reply's object count.
@@ -728,6 +730,7 @@ private:
int const found,
std::chrono::microseconds const lookupElapsed,
resource::Charge const& fee);
#endif // XRPL_ENABLE_TELEMETRY
protected:
// Kept `protected` so test subclasses (see

View File

@@ -22,6 +22,10 @@
#include <xrpld/telemetry/MetricsRegistry.h>
// Unguarded because the constructor's `beast::Journal journal` parameter is
// declared in both configurations; only the member it initialises is guarded.
#include <xrpl/beast/utility/Journal.h>
#ifdef XRPL_ENABLE_TELEMETRY
// The app and overlay includes below are why
@@ -53,7 +57,6 @@
#include <xrpl/basics/CountedObject.h>
#include <xrpl/basics/Log.h>
#include <xrpl/basics/UptimeClock.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/core/ServiceRegistry.h>
#include <xrpl/json/json_value.h>
#include <xrpl/nodestore/Database.h>

View File

@@ -143,11 +143,8 @@
#include <xrpl/beast/utility/Journal.h>
#include <algorithm>
#include <atomic>
#include <cstdint>
#include <functional>
#include <limits>
#include <memory>
#include <optional>
#include <string>
#include <string_view>
@@ -158,6 +155,13 @@
#include <opentelemetry/nostd/shared_ptr.h>
#include <opentelemetry/nostd/unique_ptr.h>
#include <opentelemetry/sdk/metrics/meter_provider.h>
// These three serve only the telemetry-only members below, so they are guarded
// like their uses: std::atomic by callbacksDetached_, std::function by the
// ObserveFn sink, std::shared_ptr by provider_.
#include <atomic>
#include <functional>
#include <memory>
#endif
namespace xrpl {