diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 5fda2b6f9f..fe7a65fc29 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -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. diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 469ffc3fc9..409d4d9772 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -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::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); diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index 52e8ea37c6..3d29db603f 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -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( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(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(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( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::schemeError), + HasSubstr(mtls::keyEndpoint), + HasSubstr(mtls::httpEndpoint)))); } TEST(TelemetryConfig, mtls_scheme_not_checked_when_telemetry_disabled) diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index ae29fc57ce..5e8cc9139f 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -363,6 +363,11 @@ public: // helper, keeping metrics and traces on one network label. config_->section("telemetry").valueOr("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( diff --git a/src/xrpld/app/main/CollectorManager.cpp b/src/xrpld/app/main/CollectorManager.cpp index df40c4e1df..4182085a90 100644 --- a/src/xrpld/app/main/CollectorManager.cpp +++ b/src/xrpld/app/main/CollectorManager.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -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(params, serviceName, networkType, journal); + return std::make_unique( + params, serviceName, networkType, telemetryEnabled, journal); } } // namespace xrpl diff --git a/src/xrpld/app/main/CollectorManager.h b/src/xrpld/app/main/CollectorManager.h index 2539b7ecc8..19157e3a78 100644 --- a/src/xrpld/app/main/CollectorManager.h +++ b/src/xrpld/app/main/CollectorManager.h @@ -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 @@ -40,6 +44,7 @@ makeCollectorManager( Section const& params, std::string const& serviceName, std::string const& networkType, + bool telemetryEnabled, beast::Journal journal); } // namespace xrpl