diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 469ffc3fc9..8c985fbb17 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -343,7 +343,13 @@ makeTelemetrySetup( setup.enabled = section.valueOr(key::enabled, 0) != 0; setup.serviceName = section.valueOr(key::serviceName, dflt::serviceName); setup.serviceVersion = version; + // Match makeMetricsRegistryOptions() in Application.cpp: an empty + // configured value is treated as absent and falls back to the node key, + // so traces and metrics stamp the same identity. Otherwise one node + // reports two identities and every $node filter shows half the series. setup.serviceInstanceId = section.valueOr(key::serviceInstanceId, nodePublicKey); + if (setup.serviceInstanceId.empty()) + setup.serviceInstanceId = nodePublicKey; setup.tracesEndpoint = section.valueOr(key::tracesEndpoint, dflt::tracesEndpoint); setup.metricsEndpoint = diff --git a/src/tests/libxrpl/telemetry/MetricMacros.cpp b/src/tests/libxrpl/telemetry/MetricMacros.cpp index d29aeb2930..2cbb202cdb 100644 --- a/src/tests/libxrpl/telemetry/MetricMacros.cpp +++ b/src/tests/libxrpl/telemetry/MetricMacros.cpp @@ -60,9 +60,21 @@ public: configure(bool enabled, opentelemetry::nostd::shared_ptr meter) { enabled_ = enabled; + stopped_ = false; meter_ = std::move(meter); } + /** + * Simulate MetricsRegistry::stop(): the recording gate flips closed even + * while enabled_ stays true, matching the real class where a call after + * stop() must not touch the SDK instrument cache. + */ + void + stop() noexcept + { + stopped_ = true; + } + /** * Number of times meter() has been consulted, so a test can assert the * create-once (function-local static) and disabled-gating behavior exactly. @@ -79,6 +91,16 @@ public: return enabled_; } + /** + * Mirrors MetricsRegistry::recording(): the macros consult this instead of + * isEnabled() so a stopped registry records nothing. + */ + [[nodiscard]] bool + recording() const noexcept + { + return enabled_ && !stopped_; + } + [[nodiscard]] opentelemetry::nostd::shared_ptr meter() const noexcept { @@ -92,6 +114,12 @@ private: */ bool enabled_ = true; + /** + * Set by stop() to model the real registry's post-shutdown state: + * enabled_ stays true but recording() flips to false. + */ + bool stopped_ = false; + /** * Meter handed to the macro; sourced from a bare SDK provider. */ @@ -320,6 +348,28 @@ TEST(MetricMacros, observable_counter_and_updown_register_do_not_crash) EXPECT_EQ(app.registry().meterCalls(), 2); } +TEST(MetricMacros, stopped_registry_records_nothing) +{ + ScopedBareProvider const bareProvider; + FakeApp app; + wire(app, /*enabled=*/true); + + // Simulate MetricsRegistry::stop(): recording() flips closed even while + // isEnabled() stays true, because the OTel provider is torn down in + // stop() and a Record on a stale SDK instrument would deref a dangling + // AggregationConfig for a first-seen attribute set. + app.registry().stop(); + ASSERT_TRUE(app.registry().isEnabled()); + ASSERT_FALSE(app.registry().recording()); + + XRPL_METRIC_COUNTER_INC( + app, "test_macro_stopped_counter_total", "Counter after stop() must be inert"); + + // The recording() gate short-circuits before the create-once static path + // runs, so meter() is never consulted. + EXPECT_EQ(app.registry().meterCalls(), 0); +} + TEST(MetricMacros, disabled_registry_is_noop) { ScopedBareProvider const bareProvider; diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index c92e92a7ef..64cf521e1c 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -322,6 +322,18 @@ TEST(TelemetryConfig, parse_empty_section) EXPECT_TRUE(setup.traceLedger); } +TEST(TelemetryConfig, empty_service_instance_id_falls_back_to_node_key) +{ + // An empty value is indistinguishable from an unset key on the metrics + // side (makeMetricsRegistryOptions falls back to the node key), so the + // trace side must do the same. Without this fallback traces stamp "" while + // metrics stamp the node key and every $node filter shows half the series. + Section section; + section.set("service_instance_id", ""); + auto const setup = telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); + EXPECT_EQ(setup.serviceInstanceId, "nHUtest123"); +} + TEST(TelemetryConfig, parse_full_section) { // The CA path has to name a real file: with enabled=1 and use_tls=1 the diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index bda8978034..a11a88622a 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1453,8 +1453,11 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) // The metrics resource was fixed at construction, but the tracer resource is // built by start() below, so the stored key still reaches spans if it - // differs from the resolved one. - if (!config_->section("telemetry").exists("service_instance_id")) + // differs from the resolved one. Treat an empty configured value as absent, + // matching makeMetricsRegistryOptions() and makeTelemetrySetup(), so the + // trace side does not keep an empty instance id while the metrics side + // holds the node key. + if (config_->section("telemetry").valueOr("service_instance_id", "").empty()) telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_.first)); // xrpl.node.id always carries the node public key. Unlike diff --git a/src/xrpld/telemetry/MetricMacros.h b/src/xrpld/telemetry/MetricMacros.h index 4b0f3754e0..0dbf642222 100644 --- a/src/xrpld/telemetry/MetricMacros.h +++ b/src/xrpld/telemetry/MetricMacros.h @@ -96,9 +96,16 @@ * MetricsRegistry::meter(). The registry builds that meter in its * constructor, before any subsystem exists, and guarantees it is never * empty while the registry is enabled (a no-op meter stands in if the - * pipeline failed to build). So a call site holds a valid instrument from - * its first call and needs no check of its own; the only branch on the - * hot path is the isEnabled() gate. + * pipeline failed to build, and again after stop()). So a call site holds + * a valid instrument from its first call and needs no check of its own. + * The only branch on the hot path is the recording() gate, which is false + * once stop() has torn the pipeline down; without that gate a Record on a + * stale SDK instrument would deref a dangling AggregationConfig. + * + * @note Static-init safety: Meter::CreateXxx is declared noexcept in the + * OTel API (opentelemetry/metrics/meter.h), so the function-local static + * that caches the instrument cannot throw during first-call construction. + * A throw there would call std::terminate. * * @note The OBSERVABLE registration macros are the opposite: call them * EAGERLY, exactly once, from constructor/init code -- never from a hot @@ -136,7 +143,7 @@ #define XRPL_METRIC_COUNTER_INC(app, name, description) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_counter_ = \ xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ @@ -151,7 +158,7 @@ #define XRPL_METRIC_COUNTER_INC_LABELED(app, name, description, ...) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_counter_ = \ xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ @@ -164,7 +171,7 @@ #define XRPL_METRIC_COUNTER_ADD(app, name, description, amount) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_counter_ = \ xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ @@ -177,7 +184,7 @@ #define XRPL_METRIC_COUNTER_ADD_LABELED(app, name, description, amount, ...) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_counter_ = \ xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ @@ -194,7 +201,7 @@ #define XRPL_METRIC_UPDOWN_ADD(app, name, description, amount) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_updown_ = \ xrpl_mr_->meter()->CreateInt64UpDownCounter((name), (description)); \ @@ -207,7 +214,7 @@ #define XRPL_METRIC_UPDOWN_ADD_LABELED(app, name, description, amount, ...) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_updown_ = \ xrpl_mr_->meter()->CreateInt64UpDownCounter((name), (description)); \ @@ -218,7 +225,7 @@ #define XRPL_METRIC_HISTOGRAM_RECORD(app, name, description, value) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_hist_ = \ xrpl_mr_->meter()->CreateDoubleHistogram((name), (description)); \ @@ -231,7 +238,7 @@ #define XRPL_METRIC_HISTOGRAM_RECORD_LABELED(app, name, description, value, ...) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_hist_ = \ xrpl_mr_->meter()->CreateDoubleHistogram((name), (description)); \ @@ -257,7 +264,7 @@ #define XRPL_METRIC_GAUGE_RECORD(app, name, description, value) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_gauge_ = \ xrpl_mr_->meter()->CreateDoubleGauge((name), (description)); \ @@ -269,7 +276,7 @@ #define XRPL_METRIC_GAUGE_RECORD_LABELED(app, name, description, value, ...) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ static auto const xrpl_gauge_ = \ xrpl_mr_->meter()->CreateDoubleGauge((name), (description)); \ @@ -317,7 +324,7 @@ #define XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER(app, name, description, valueFn) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ auto xrpl_m_ = xrpl_mr_->meter(); \ auto* xrpl_fn_ = new std::function(valueFn); \ @@ -342,7 +349,7 @@ #define XRPL_METRIC_OBSERVABLE_COUNTER_REGISTER(app, name, description, valueFn) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ auto xrpl_m_ = xrpl_mr_->meter(); \ auto* xrpl_fn_ = new std::function(valueFn); \ @@ -367,7 +374,7 @@ #define XRPL_METRIC_OBSERVABLE_UPDOWN_REGISTER(app, name, description, valueFn) \ do \ { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->recording()) \ { \ auto xrpl_m_ = xrpl_mr_->meter(); \ auto* xrpl_fn_ = new std::function(valueFn); \ diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index 08c1f596c5..0c04d16e34 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -78,7 +78,6 @@ #include #include #include -#include #include #include #include @@ -99,6 +98,7 @@ #include #include #include +#include #include #include #include @@ -248,14 +248,10 @@ void MetricsRegistry::disablePipeline(std::string_view reason) { provider_.reset(); - // meter_ becomes a no-op meter, which keeps the invariant the - // XRPL_METRIC_* macros rely on: an enabled registry always has a meter, - // so every call site gets an instrument (a no-op one here) with no check - // of its own. Through the base pointer, as Telemetry::getMeter() does: - // the no-op provider's override hides the base class's defaulted overload. - opentelemetry::nostd::shared_ptr const noop( - new opentelemetry::metrics::NoopMeterProvider()); - meter_ = noop->GetMeter(std::string(kMeterName), std::string(kMeterVersion)); + // A no-op meter keeps the invariant the XRPL_METRIC_* macros rely on: an + // enabled registry always has a meter, so every call site gets an inert + // instrument here with no check of its own. + meter_ = noopMeter(kMeterName); JLOG(journal_.error()) << "MetricsRegistry: metrics pipeline failed to initialise, " "continuing without native metrics: " << reason; @@ -278,23 +274,26 @@ MetricsRegistry::startAsyncGauges() // same-named instruments, and a call after stop() would register on a // provider that is gone. Checked before the pipeline, so a call after // stop() is reported as what it is and not as a build failure. - if (phase_ != Phase::Ready) + auto const currentPhase = phase_.load(std::memory_order_relaxed); + if (currentPhase != Phase::Ready) { JLOG(journal_.warn()) << "MetricsRegistry: startAsyncGauges() called " - << (phase_ == Phase::Stopped ? "after stop()" : "twice") + << (currentPhase == Phase::Stopped ? "after stop()" : "twice") << "; ignored"; return; } // The pipeline failed to build: the meter is a no-op, so registering - // gauges on it would only log a success that is not one. + // gauges on it would only log a success that is not one. phase_ stays + // at Ready, so a second call lands here again and logs the same message. + // Idempotent. if (!provider_) { JLOG(journal_.warn()) << "MetricsRegistry: startAsyncGauges() without a pipeline; " "no gauges registered"; return; } - phase_ = Phase::GaugesArmed; + phase_.store(Phase::GaugesArmed, std::memory_order_relaxed); registerAsyncGauges(); @@ -462,9 +461,12 @@ void MetricsRegistry::stop() { #ifdef XRPL_ENABLE_TELEMETRY - // Idempotent: the destructor calls this after run() or the Application - // destructor already did. - phase_ = Phase::Stopped; + // Store Stopped with release ordering BEFORE the pipeline goes away. + // Every recording thread reads phase_ through recording() with acquire + // ordering, so any record that has not yet passed the gate will see + // Stopped and skip. Idempotent: destructor calls this after run() or + // ~ApplicationImp already did. + phase_.store(Phase::Stopped, std::memory_order_release); if (!provider_) return; @@ -477,11 +479,23 @@ MetricsRegistry::stop() // to detach first. callbacksDetached_.store(true, std::memory_order_release); + // meter_ is left alone on purpose. Job threads are still running here and + // may be inside a macro, so writing meter_ would race with their read. + // The recording() gate is what keeps them off the dying pipeline: only the + // macros read meter_, and none of them does so once phase_ is Stopped. + // // SDK teardown order: Shutdown() stops the PeriodicExportingMetricReader // thread (so no further gauge callbacks fire) and performs the final // collect-and-export drain itself. The trailing ForceFlush() is a // redundant safety net (a no-op once the reader is shut down), then // reset() destroys the provider. + // + // provider_.reset() destroys MeterProvider -> MeterContext -> ViewRegistry + // -> each View -> its shared_ptr. Live SDK + // SyncMetricStorage instances cached in call-site statics still hold a + // raw AggregationConfig pointer; a Record with a NEW attribute set after + // this point would fire the factory lambda and deref that dangling + // pointer, and a late meter()->CreateXxx would return null. provider_->Shutdown(); provider_->ForceFlush(); provider_.reset(); @@ -498,7 +512,7 @@ void MetricsRegistry::recordRpcStarted([[maybe_unused]] std::string_view method) { #ifdef XRPL_ENABLE_TELEMETRY - if (!enabled_ || !rpcStartedCounter_) + if (!recording() || !rpcStartedCounter_) return; rpcStartedCounter_->Add(1, {{"method", std::string(method)}}); #endif @@ -510,7 +524,7 @@ MetricsRegistry::recordRpcFinished( [[maybe_unused]] std::int64_t durationUs) { #ifdef XRPL_ENABLE_TELEMETRY - if (!enabled_ || !rpcFinishedCounter_) + if (!recording() || !rpcFinishedCounter_) return; rpcFinishedCounter_->Add(1, {{"method", std::string(method)}}); if (rpcDurationHistogram_) @@ -529,7 +543,7 @@ MetricsRegistry::recordRpcErrored( [[maybe_unused]] std::int64_t durationUs) { #ifdef XRPL_ENABLE_TELEMETRY - if (!enabled_ || !rpcErroredCounter_) + if (!recording() || !rpcErroredCounter_) return; rpcErroredCounter_->Add(1, {{"method", std::string(method)}}); if (rpcDurationHistogram_) @@ -552,7 +566,7 @@ MetricsRegistry::recordJobQueued( [[maybe_unused]] std::string_view jobName) { #ifdef XRPL_ENABLE_TELEMETRY - if (!enabled_ || !jobQueuedCounter_) + if (!recording() || !jobQueuedCounter_) return; jobQueuedCounter_->Add( 1, @@ -568,7 +582,7 @@ MetricsRegistry::recordJobStarted( [[maybe_unused]] std::int64_t queuedDurUs) { #ifdef XRPL_ENABLE_TELEMETRY - if (!enabled_ || !jobStartedCounter_) + if (!recording() || !jobStartedCounter_) return; // Build the attribute pair once: both the counter and the histogram // must carry the identical label set or they cannot be joined. @@ -595,7 +609,7 @@ MetricsRegistry::recordJobFinished( [[maybe_unused]] std::int64_t runningDurUs) { #ifdef XRPL_ENABLE_TELEMETRY - if (!enabled_ || !jobFinishedCounter_) + if (!recording() || !jobFinishedCounter_) return; std::string const handler(sanitiseHandler(jobName)); jobFinishedCounter_->Add(1, {{kJobTypeLabel, std::string(jobType)}, {kHandlerLabel, handler}}); @@ -1732,7 +1746,7 @@ void MetricsRegistry::incrementLedgersClosed() { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && ledgersClosedCounter_) + if (recording() && ledgersClosedCounter_) ledgersClosedCounter_->Add(1); #endif } @@ -1741,7 +1755,7 @@ void MetricsRegistry::incrementValidationsSent() { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && validationsSentCounter_) + if (recording() && validationsSentCounter_) validationsSentCounter_->Add(1); #endif } @@ -1750,7 +1764,7 @@ void MetricsRegistry::incrementValidationsChecked() { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && validationsCheckedCounter_) + if (recording() && validationsCheckedCounter_) validationsCheckedCounter_->Add(1); #endif } @@ -1759,7 +1773,7 @@ void MetricsRegistry::incrementStateChanges() { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && stateChangesCounter_) + if (recording() && stateChangesCounter_) stateChangesCounter_->Add(1); #endif } @@ -1768,7 +1782,7 @@ void MetricsRegistry::incrementLedgerHistoryMismatch(std::string_view reason) { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && ledgerHistoryMismatchCounter_) + if (recording() && ledgerHistoryMismatchCounter_) ledgerHistoryMismatchCounter_->Add(1, {{"reason", std::string(reason)}}); #endif } @@ -1777,7 +1791,7 @@ void MetricsRegistry::incrementTxqExpired() { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && txqExpiredCounter_) + if (recording() && txqExpiredCounter_) txqExpiredCounter_->Add(1); #endif } @@ -1786,7 +1800,7 @@ void MetricsRegistry::incrementTxqDropped(std::string_view reason) { #ifdef XRPL_ENABLE_TELEMETRY - if (enabled_ && txqDroppedCounter_) + if (recording() && txqDroppedCounter_) txqDroppedCounter_->Add(1, {{"reason", std::string(reason)}}); #endif } diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index c65c8bb646..c9c4f5b757 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -461,6 +461,11 @@ public: /** * Flush pending metrics and shut down the pipeline. * + * Stores `Phase::Stopped` first so `recording()` reads false on every + * later record call, then destroys the SDK provider. meter_ is not + * touched: record threads may still be running, and the gate is what + * keeps them off the dying pipeline. Idempotent. + * * @pre `detachCallbacks()` should have been called earlier in the * shutdown sequence; otherwise there is a narrow race between * the final reader-thread tick and the destruction of @@ -478,6 +483,28 @@ public: return enabled_; } + /** + * @return true when a record call is safe to run. + * + * False when the registry is disabled, or after stop() has torn down the + * export pipeline. After stop() the SDK's SyncMetricStorage still holds a + * raw pointer to an AggregationConfig owned by a destroyed View, so a + * record with a first-seen attribute set would fire the factory lambda + * and deref that dangling pointer. Every XRPL_METRIC_* macro reads this + * once before touching an instrument. + * + * One acquire atomic load in the hot path. + */ + [[nodiscard]] bool + recording() const noexcept + { +#ifdef XRPL_ENABLE_TELEMETRY + return enabled_ && phase_.load(std::memory_order_acquire) != Phase::Stopped; +#else + return enabled_; +#endif + } + // ----------------------------------------------------------------- // Synchronous instrument recording (called from PerfLog hot paths) // ----------------------------------------------------------------- @@ -849,11 +876,11 @@ public: * counters/histograms can be declared at their call site instead of as * MetricsRegistry members. * - * Invariant: never empty while isEnabled() is true. The constructor sets + * Invariant: never empty while recording() is true. The constructor sets * it to the real meter, or to a no-op meter when the pipeline failed to - * build, so a call site creates its instrument with no check of its own. - * Empty only when the registry is disabled, which the macros gate on - * first. + * build, and never writes it again, so reads need no lock. After stop() + * the meter's SDK context is gone; the macros gate on recording() first, + * so no caller reaches it then. * * @return The shared Meter. */ @@ -950,13 +977,18 @@ private: * Where the registry is in its life. Construction ends in `Ready`; * startAsyncGauges() moves to `GaugesArmed`; stop() to `Stopped`. A call * that does not fit the current phase logs a warning and does nothing. + * + * After `Stopped` the SDK pipeline is gone. recording() reads false, so + * no macro touches meter_ or a cached instrument. */ enum class Phase { Ready, GaugesArmed, Stopped }; /** - * Current phase; written only from the Application lifecycle thread. + * Current phase. Written from the Application lifecycle thread with + * release ordering; read from record threads via `recording()` with + * acquire ordering, so no record starts once stop() has stored `Stopped`. */ - Phase phase_{Phase::Ready}; + std::atomic phase_{Phase::Ready}; /** * Set by detachCallbacks() during shutdown so every ObservableGauge