ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-24 14:10:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(telemetry): Warn on server=otel with telemetry off, and widen the TLS guard

Browse Source 
Three fixes in the telemetry wiring this branch owns.

An OTel collector records through the global meter provider, which only the
telemetry module installs. With [telemetry] enabled=0 the collector attached to
the SDK's noop provider, dropped every metric, and still logged that it had
started. makeCollectorManager now takes whether telemetry is on and warns; what
gets built is unchanged. telemetry_ is constructed before collectorManager_, so
the value is available.

The default metrics endpoint was spelled three times. Name it once as
kDefaultMetricsEndpoint, beside the export-cadence defaults that already live in
Telemetry.h for the same reason, and read it from the Setup member, the config
parser's default and the collector.

The scheme guard ran only when tls_client_cert was set, so use_tls=1 on an http
endpoint validated the certificate files and then exported in the clear. It now
covers every use_tls=1 node, for both signals. Two cases that pinned the old
behaviour are flipped to _throws, each asserting which endpoint key the message
names, and three more get https endpoints so the scheme guard is not what fires.
This commit is contained in:
Pratik Mankawde
2026-09-23 14:29:24 +01:00
parent 360cde04ef
commit 5ce7e4e855
6 changed files with 93 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
include/xrpl/telemetry/Telemetry.h
View File

@@ -167,6 +167,16 @@ inline constexpr auto kDefaultMetricExportInterval = std::chrono::milliseconds{1
*/
inline constexpr auto kDefaultMetricExportTimeout = std::chrono::milliseconds{500};
/**
* Default OTLP/HTTP URL for metrics, signal path included.
*
* The collector's standard port on the same host. Declared here so the Setup
* member, the config parser's default and the collector's startup log all name
* one string. Outside the telemetry #ifdef, because the config parser reads it
* in every build.
*/
inline constexpr char const* kDefaultMetricsEndpoint = "http://localhost:4318/v1/metrics";
/**
* How a consensus round span picks its trace id.
*
@@ -307,7 +317,7 @@ public:
* point the two signals at different collectors, or at one whose OTLP
* paths are not the defaults.
*/
std::string metricsEndpoint = "http://localhost:4318/v1/metrics";
std::string metricsEndpoint = kDefaultMetricsEndpoint;
/**
* Whether to use TLS for the exporter connection.

31
src/libxrpl/telemetry/TelemetryConfig.cpp
View File

@@ -72,7 +72,7 @@ constexpr char const* consensusTraceStrategy = "consensus_trace_strategy";
namespace dflt {
constexpr char const* serviceName = "xrpld";
constexpr char const* tracesEndpoint = "http://localhost:4318/v1/traces";
constexpr char const* metricsEndpoint = "http://localhost:4318/v1/metrics";
constexpr char const* metricsEndpoint = kDefaultMetricsEndpoint;
constexpr std::uint32_t batchSize = 512u;
constexpr std::uint32_t batchDelayMs = 5000u;
constexpr std::uint32_t maxQueueSize = 2048u;
@@ -250,14 +250,14 @@ requirePositive(std::chrono::milliseconds value, char const* configKey)
}
/**
* Throw unless an endpoint URL is one the client certificate can be used on.
* Throw unless an endpoint URL is one TLS can actually be used on.
*
* The OTLP/HTTP exporter turns TLS on from the URL scheme alone, and matches
* "https:" exactly and case-sensitively. So a client certificate only reaches
* the collector on an https endpoint, and this check is what holds that
* invariant: with a client certificate configured, the endpoint is an https URL.
* "https://" is required in full, which is stricter than the exporter's own
* test, so anything this accepts the exporter also treats as TLS.
* "https:" exactly and case-sensitively. So the certificate settings only mean
* anything on an https endpoint, and this check is what holds that invariant:
* with TLS asked for, the endpoint is an https URL. "https://" is required in
* full, which is stricter than the exporter's own test, so anything this
* accepts the exporter also treats as TLS.
*
* @param endpoint Endpoint URL from the config, or the built-in default.
* @param configKey Config key the URL came from, named in the message.
@@ -273,8 +273,8 @@ requireHttpsEndpoint(std::string const& endpoint, char const* configKey)
Throw<std::runtime_error>(
std::string("Invalid value '") + configKey + "' in " + kSectionLabel +
": must start with '" + std::string{kHttpsPrefix} + "' when " + key::tlsClientCert +
" is set, but is '" + endpoint + "'.");
": must start with '" + std::string{kHttpsPrefix} + "' when " + key::useTls +
"=1, but is '" + endpoint + "'.");
}
/**
@@ -388,13 +388,12 @@ makeTelemetrySetup(
// Still inside the enabled branch, and checked before the files are
// opened so a scheme problem is not hidden behind a path problem. Each
// exporter reads TLS off its own endpoint scheme, and both are handed
// the client certificate, so both endpoints have to be https. Checking
// only one leaves the other signal exporting in the clear without this
// node's identity. tls_ca_cert is left out of this check: it only names
// a trust store, while a client certificate is this node's own identity
// and has to reach the collector to mean anything.
if (!setup.tlsClientCertPath.empty())
// exporter reads TLS off its own endpoint scheme alone, so use_tls=1 on
// an http endpoint would validate the certificate files and then still
// export in the clear. Both endpoints are checked, because checking only
// one leaves the other signal in plaintext. An operator who asks for TLS
// gets TLS on both signals, or a startup error.
if (setup.useTls)
{
requireHttpsEndpoint(setup.tracesEndpoint, key::tracesEndpoint);
requireHttpsEndpoint(setup.metricsEndpoint, key::metricsEndpoint);

60
src/tests/libxrpl/telemetry/TelemetryConfig.cpp
View File

@@ -331,8 +331,8 @@ TEST(TelemetryConfig, parse_full_section)
section.set("enabled", "1");
section.set("service_name", "my-rippled");
section.set("service_instance_id", "custom-id");
section.set("traces_endpoint", "http://collector:4318/v1/traces");
section.set("metrics_endpoint", "http://collector:4318/v1/metrics");
section.set("traces_endpoint", "https://collector:4318/v1/traces");
section.set("metrics_endpoint", "https://collector:4318/v1/metrics");
section.set("use_tls", "1");
section.set("tls_ca_cert", caCert);
section.set("batch_size", "256");
@@ -351,8 +351,8 @@ TEST(TelemetryConfig, parse_full_section)
EXPECT_TRUE(setup.enabled);
EXPECT_EQ(setup.serviceName, "my-rippled");
EXPECT_EQ(setup.serviceInstanceId, "custom-id");
EXPECT_EQ(setup.tracesEndpoint, "http://collector:4318/v1/traces");
EXPECT_EQ(setup.metricsEndpoint, "http://collector:4318/v1/metrics");
EXPECT_EQ(setup.tracesEndpoint, "https://collector:4318/v1/traces");
EXPECT_EQ(setup.metricsEndpoint, "https://collector:4318/v1/metrics");
EXPECT_TRUE(setup.useTls);
EXPECT_EQ(setup.tlsCertPath, caCert);
EXPECT_EQ(setup.batchSize, 256u);
@@ -474,11 +474,14 @@ TEST(TelemetryConfig, mtls_neither_set_is_one_way_tls)
{
// Telemetry is on so the checks run, and this config must pass all of
// them: one-way TLS with a CA bundle and no client certificate. The CA
// path has to name a real file, because the parser opens it here.
// path has to name a real file, because the parser opens it here. Both
// endpoints are https because use_tls=1 now requires it.
TempDir const dir;
auto const caCert = mtls::writeCertFile(dir.file("ca.pem"));
Section section = mtls::makeSection(true);
section.set("use_tls", "1");
section.set(mtls::keyEndpoint, mtls::httpsEndpoint);
section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpsEndpoint);
section.set("tls_ca_cert", caCert);
auto const setup = mtls::parseSection(section);
@@ -535,11 +538,14 @@ TEST(TelemetryConfig, tls_missing_client_key_file_throws)
TEST(TelemetryConfig, tls_missing_ca_cert_file_throws)
{
// One-way TLS with no client certificate, so the CA bundle is the only
// path checked.
// path checked. Both endpoints are https so the scheme guard, which runs
// first, cannot be what throws.
TempDir const dir;
auto const absentCa = dir.file("absent-ca.pem");
Section section = mtls::makeSection(true);
section.set("use_tls", "1");
section.set(mtls::keyEndpoint, mtls::httpsEndpoint);
section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpsEndpoint);
section.set("tls_ca_cert", absentCa);
EXPECT_THAT(
@@ -745,22 +751,26 @@ TEST(TelemetryConfig, mtls_client_cert_with_the_default_metrics_endpoint_throws)
HasSubstr(mtls::defaultMetricsEndpoint))));
}
TEST(TelemetryConfig, one_way_tls_on_a_plain_http_metrics_endpoint_is_accepted)
TEST(TelemetryConfig, one_way_tls_on_a_plain_http_metrics_endpoint_throws)
{
// The control for the metric guard's scope, matching the trace one below:
// same plain http metrics endpoint and use_tls=1, but no client identity to
// lose. Widen the guard to every use_tls=1 node and this case starts failing.
// use_tls=1 with no client certificate, and only the metrics endpoint left
// on plain http. traces_endpoint is https so the trace guard, which runs
// first, cannot be what throws -- the message must name metrics_endpoint.
// Narrow the guard back to tls_client_cert and this case passes.
TempDir const dir;
auto const ca = mtls::writeCertFile(dir.file("ca.pem"));
Section section = mtls::makeSection(true);
section.set("use_tls", "1");
section.set(mtls::keyEndpoint, mtls::httpsEndpoint);
section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpEndpoint);
section.set("tls_ca_cert", ca);
telemetry::Telemetry::Setup setup;
ASSERT_NO_THROW(setup = mtls::parseSection(section));
EXPECT_EQ(setup.metricsEndpoint, mtls::metricsHttpEndpoint);
EXPECT_TRUE(setup.tlsClientCertPath.empty());
EXPECT_THAT(
[&section] { mtls::parseSection(section); },
ThrowsMessage<std::runtime_error>(AllOf(
HasSubstr(mtls::schemeError),
HasSubstr(mtls::keyMetricsEndpoint),
HasSubstr(mtls::metricsHttpEndpoint))));
}
TEST(TelemetryConfig, mtls_scheme_check_is_case_sensitive_like_the_exporter)
@@ -780,12 +790,13 @@ TEST(TelemetryConfig, mtls_scheme_check_is_case_sensitive_like_the_exporter)
ThrowsMessage<std::runtime_error>(HasSubstr(mtls::schemeError)));
}
TEST(TelemetryConfig, one_way_tls_on_a_plain_http_endpoint_is_accepted)
TEST(TelemetryConfig, one_way_tls_on_a_plain_http_endpoint_throws)
{
// The control for the guard's scope: same http:// endpoint and use_tls=1,
// but no client certificate. Only a client identity can be silently
// dropped, so this configuration is left alone. Widen the guard to every
// use_tls=1 node and this case starts failing.
// use_tls=1 with no client certificate, on an http:// endpoint. The
// exporter reads TLS off the scheme, so this config would check the CA
// file and then export in the clear. The guard covers every use_tls=1
// node, not just the ones presenting a client identity, so it fires here
// too. Narrow the guard back to tls_client_cert and this case passes.
TempDir const dir;
auto const ca = mtls::writeCertFile(dir.file("ca.pem"));
Section section = mtls::makeSection(true);
@@ -793,11 +804,12 @@ TEST(TelemetryConfig, one_way_tls_on_a_plain_http_endpoint_is_accepted)
section.set(mtls::keyEndpoint, mtls::httpEndpoint);
section.set("tls_ca_cert", ca);
telemetry::Telemetry::Setup setup;
ASSERT_NO_THROW(setup = mtls::parseSection(section));
EXPECT_EQ(setup.tracesEndpoint, mtls::httpEndpoint);
EXPECT_EQ(setup.tlsCertPath, ca);
EXPECT_TRUE(setup.tlsClientCertPath.empty());
EXPECT_THAT(
[&section] { mtls::parseSection(section); },
ThrowsMessage<std::runtime_error>(AllOf(
HasSubstr(mtls::schemeError),
HasSubstr(mtls::keyEndpoint),
HasSubstr(mtls::httpEndpoint))));
}
TEST(TelemetryConfig, mtls_scheme_not_checked_when_telemetry_disabled)

5
src/xrpld/app/main/Application.cpp
View File

@@ -363,6 +363,11 @@ public:
// helper, keeping metrics and traces on one network label.
config_->section("telemetry").valueOr<std::string>("service_name", ""),
telemetry::networkTypeFromId(config_->networkId),
// telemetry_ is declared before this member, so it is already
// built. An OTel collector needs the meter provider that only
// the telemetry module installs, and this lets the collector
// warn instead of silently dropping every metric.
telemetry_->isEnabled(),
logs_->journal("Collector")))
, jobQueue_(
std::make_unique<JobQueue>(

25
src/xrpld/app/main/CollectorManager.cpp
View File

@@ -10,6 +10,7 @@
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/config/BasicConfig.h>
#include <xrpl/config/Constants.h>
#include <xrpl/telemetry/Telemetry.h>
#include <memory>
#include <string>
@@ -29,6 +30,7 @@ public:
Section const& params,
std::string const& serviceName,
std::string const& networkType,
bool telemetryEnabled,
beast::Journal journal)
: journal_(journal)
{
@@ -45,11 +47,24 @@ public:
// LCOV_EXCL_START -- OTel collector path is not exercised in unit tests
else if (server == "otel")
{
// Read OTLP metrics endpoint from [insight] section.
// Default to the standard OTLP/HTTP metrics path on localhost.
// The collector records through the global meter provider, and only
// the telemetry module installs one. With telemetry off it attaches
// to the SDK's noop provider and every metric is dropped, which
// otherwise looks like a clean start with empty dashboards. Say so
// rather than change what is built.
if (!telemetryEnabled && journal_.warn())
{
journal_.warn() << "[insight] server=otel needs [telemetry] enabled=1. "
"Telemetry is off, so no metric will be exported.";
}
// Read OTLP metrics endpoint from [insight] section, falling back
// to the same default the [telemetry] parser uses.
std::string endpoint = get(params, "endpoint");
if (endpoint.empty())
endpoint = "http://localhost:4318/v1/metrics";
{
endpoint = telemetry::kDefaultMetricsEndpoint;
}
std::string const& prefix(get(params, "prefix"));
// Read for signature uniformity only. OTelCollector ignores it:
@@ -100,9 +115,11 @@ makeCollectorManager(
Section const& params,
std::string const& serviceName,
std::string const& networkType,
bool telemetryEnabled,
beast::Journal journal)
{
return std::make_unique<CollectorManagerImp>(params, serviceName, networkType, journal);
return std::make_unique<CollectorManagerImp>(
params, serviceName, networkType, telemetryEnabled, journal);
}
} // namespace xrpl

5
src/xrpld/app/main/CollectorManager.h
View File

@@ -33,6 +33,10 @@ public:
* (empty -> the collector defaults it to "xrpld").
* @param networkType xrpl.network.type resource attribute for OTel
* metrics (e.g. "mainnet"), derived from [network_id].
* @param telemetryEnabled Whether the telemetry module is on. The OTel
* collector records through the global meter provider, which only the
* telemetry module installs, so with this false an OTel collector would
* discard every metric. Used to warn, not to change what is built.
* @param journal Journal for logging.
*/
std::unique_ptr<CollectorManager>
@@ -40,6 +44,7 @@ makeCollectorManager(
Section const& params,
std::string const& serviceName,
std::string const& networkType,
bool telemetryEnabled,
beast::Journal journal);
} // namespace xrpl