Merges pratik/otel-phase7-native-metrics into pratik/otel-phase8-log-correlation.
Conflict was one TESTING.md hunk under "Nodes not reaching proposing state":
this branch renamed the node directories to Node-N in integration-test.sh,
phase-7 kept nodeN and expanded the [peer_private] explanation. Resolution
keeps this branch's Node-1 path (its own script uses that naming) and
phase-7's fuller prose citing peerfinder/Config.cpp.
Non-conflicting phase-7 changes come through: OTelCollector's gauges_ list
becomes weak_ptr, matching the earlier hooks_ change; the phase-6 revert of
the StatsD-test onCollectionReady() calls resolved against phase-7's version
that keeps them.
Merges pratik/otel-phase6-statsd into pratik/otel-phase7-native-metrics.
Phase-6 dropped the three onCollectionReady() calls that had been added to
its StatsD test, because that method is only declared here on phase-7.
This branch's own copy of the file was unchanged from the merge base, so
the default merge would have silently deleted the calls from here too —
where they are needed, because this branch gates polling behind
onCollectionReady() in OTelCollectorImp::onTimer.
Resolution keeps both sides: phase-6's two new include lines
(Counter.h, Gauge.h) and phase-7's three onCollectionReady() calls plus
their doxygen and inline explanations. The merged file is exactly
phase-7's tip plus those two includes.
TESTING.md auto-merged cleanly; both sides added text under Test 1 in
different regions.
gauges_ was still a vector of raw pointers after the hook list moved to
weak references. onCollectionReady() and onCollectionStopping() copy that
list under mutex_ and then call arm()/disarm() with the lock released,
because both enter the SDK's observable registry lock. ~OTelGaugeImpl only
re-acquires mutex_ to erase its own entry, so nothing stopped a gauge from
being destroyed between the snapshot and the dereference.
Store weak references, for the reason the hook list already does: locking
an entry keeps that gauge alive for exactly its own arm or disarm call, and
one destroyed since the snapshot locks to null and is skipped. Registration
moves from the OTelGaugeImpl constructor to makeGauge(), because no
weak_ptr to the object exists until the owning shared_ptr does, and the
destructor now prunes by expiry rather than by address.
The startup log line counts the gauges that were still live rather than the
snapshot length, so a skipped entry is not reported as a failed
registration.
The SDK callback path is unchanged and does not need this: gaugeCallback
takes a void* and is guarded by RemoveCallback being synchronous, as
~OTelGaugeImpl already documents.
resolveNodePublicKey() returned std::nullopt in three real cases: a first boot
with no wallet database, a standalone run (its wallet is a private temporary
database), and --newnodeid. Telemetry's resources are built during
ApplicationImp's member-init list and are immutable, so on those runs the node
reported an empty service.instance.id and no xrpl.node.id for the whole run,
while setup() minted a key moments later and patched only the tracer.
Replace it with resolveNodeIdentity(), which always returns a keypair: derived
from a configured seed, else read from an existing wallet database, else
minted. Main.cpp passes that pair to makeApplication(), ApplicationImp stores
it in nodeIdentity_ -- now declared before telemetry_ and no longer an optional,
because it is always set -- and builds the telemetry resource from it.
setup() calls getNodeIdentity(), which now persists rather than mints: it
stores the resolved pair when the wallet holds no identity, adopts the stored
one when it does, and clears first for --newnodeid. The write stays in setup()
because that is where the database exists; a standalone run has no persistent
wallet to write to, which is why the pair has to be decided before
construction rather than read back afterwards. Wallet gains storeNodeIdentity()
for that write, and getNodeIdentity(session) now uses it instead of repeating
the insert.
The three-argument makeApplication() mints a keypair, so jtx::Env and any other
test Application behave as a standalone run always did.
Also fold the three hand-rolled "meter from a NoopMeterProvider" copies into
telemetry::noopMeter(): the base-pointer call and the kMeterVersion argument are
both easy to get wrong alone, and the meter identity has to match the one the
histogram views select on.
The new gtest covers the wallet half: store-then-read, store not replacing an
existing identity, clear-then-store, and that the mint path persists. It adds
the tests.libxrpl > xrpl.rdb levelization edge, regenerated here.
0eb4291688 added collector->onCollectionReady() to both StatsD tests. That
method does not exist on this branch: it is introduced later, alongside the
polling gate it belongs to, so all four build legs and clang-tidy failed.
Nothing gates polling here. The StatsDCollectorImp constructor starts its
thread, run() calls setTimer() unconditionally, and onTimer polls metrics_
and drains the buffers every second. The gauge test's expectation already
holds without any lifecycle call, because StatsDGaugeImpl starts dirty so a
untouched gauge emits its zero on the first flush.
Restores the two includes that commit also dropped.
The config template wrote each node's log to a lowercase node{N} directory
while setting service_instance_id=Node-{N}. The collector takes the node name
from the log file's parent directory and stamps it as the Loki
service_instance_id label, so the logs carried a name no trace or metric
shared and nothing joined. Use Node-{N} and state the rule.
The template also set log_level to warning. Nothing in the pipeline filters on
severity; the constraint is that a log line carries trace context only when it
is emitted inside an active span. At warning the only such statements in the
consensus accept span are a catch path a healthy round never takes and a
periodic censorship warning. At info the CNF Val / CNF buildLCL pair writes one
line per accepted ledger, which is what makes this test's Step 1 findable.
Grafana 13 offers the link per span, labelled "Logs for this span", in the
span's Links row — not per trace. Fix the step and the expected-results row.
The example log line quoted a message that does not exist. The real in-span
RPC statement logs at debug, so the severity code is DBG; say which line to
look for under each test, since Test 2 now logs at info.
Drop the reference to workload/validate_telemetry.py: that file is not part of
this branch, and its instant-endpoint call uses seconds, so the nanoseconds
claim applied only to query_range.
On the OTel path only [insight] server is load-bearing. CollectorManager reads
endpoint and hands it to OTelCollector, which logs it at startup and routes
nothing with it; the real export endpoint is [telemetry] metrics_endpoint,
which the template already sets. service_instance_id and service_name in that
section are read and discarded.
Leaving the line invited an operator to reconcile a mismatch that has no
effect. integration-test.sh already emits only server=otel with the same
explanation, so the two now agree.
The Payment destination was not a valid XRPL address — its base58 checksum
does not match — so Test 1 Step 4 and Test 2 Step 7 could never have returned
the tesSUCCESS they claim. Use a valid one and note that the destination does
not need to exist.
The Tempo search loop had no -G, so curl posted the parameters as a body,
Tempo answered 200 while ignoring the query, and every span name came back
non-zero. It also had no time bound, and Tempo keeps blocks for an hour, so a
re-run was answered by the previous run's traces. Add -G, RUN_START, and
start/end, matching what integration-test.sh already does.
Split the query list in two: 35 names that should be present, and 8 that need
a trigger neither test performs, where zero is the expected answer. Previously
two of the latter sat in the pass/fail list and read as failures.
Correct the standalone span table. consensus.mode_change fires once per round
start whether or not the mode changes, ledger.validate cannot fire because
checkAccept is unreachable in standalone, and the apply-stage, TxQ and ledger
families were missing rows. Give each "No" row the reason that actually
applies: the establish phase, a missing validator key, or no peers.
Also: ledger_accept is not required before submit, the teardown pgrep matched
more than this node, [peer_private] also disables the inbound listener, and
the 15-second wait covers Tempo but not Prometheus.
callHooks() copied the hook list into a vector of raw pointers, released
mutex_, then dereferenced them. It has to release the lock: a handler may
drop the last reference to a hook, and ~OTelHookImpl re-acquires mutex_,
so invoking handlers under the non-recursive lock would deadlock. That
left a window in which an entry could be freed before it was used.
The window is not reachable today. Every hook belongs to a long-lived
ApplicationImp member, and onCollectionStopping() runs before those
members are destroyed, both from stop() and from the destructor body.
That call disarms each gauge via RemoveCallback, which blocks until an
in-flight callback finishes, because the SDK holds its registry mutex
across the callback. Safety therefore rests on four separate facts, none
of them enforced by a test, one of them internal to a vendored library.
Store weak references instead, so the code is correct by construction:
locking an entry keeps that hook alive for exactly its own handler call,
and a hook destroyed since the snapshot locks to null and is skipped.
Registration moves from the OTelHookImpl constructor to makeHook(),
because no weak_ptr to the object exists until the owning shared_ptr
does, and the destructor now prunes by expiry rather than by address.
Add three GTests over the real collector. A test-local MetricReader
drives one synchronous collection pass, since the SDK ships only a
threaded periodic reader. They assert a live hook runs, a destroyed hook
is skipped, and destroying one hook leaves its siblings registered --
the last pairing both directions so neither can pass vacuously.
Not yet run: verifying them needs a telemetry-enabled build of
xrpl_tests. Compile, clang-tidy and the pre-commit gates are clean.
Two conflicts, both where this branch's log-pipeline additions sat next to the
upstream spanmetrics -> span_metrics rename: the config header comment, which
this branch extended with a logs line, and the integration test, where the
log-correlation step precedes the span-metrics step. This branch's content is
kept in both and the rename carried into it.
Four conflicts, all where this branch's replacement of the StatsD path with
native OTLP met the upstream spanmetrics -> span_metrics rename. This branch's
design wins in every case; the rename is carried into its text rather than
reverting it, so the connector, its pipeline references, the header comment,
the TESTING.md summary and the runbook all use span_metrics while keeping the
native-OTLP wording.
One addition beyond a straight take-a-side: publish the collector's health
check port. This branch restored the health_check extension and its own
TESTING.md polls http://localhost:13133/ to decide the collector is ready, but
the port was never published on this side of docker-compose.yml, so that check
could not pass from the host. Verified the merged config loads with no
deprecation warnings and that 13133 is published exactly once.
No metric name changed: traces_span_metrics_* already read that way before the
rename, which only ever touched component names and prose.
Conflict in docker/telemetry/otel-collector-config.yaml, in the service
pipelines: this branch renamed the deprecated spanmetrics connector to
span_metrics and adds the statsd metrics pipeline, while upstream renamed the
deprecated otlp exporter to otlp_grpc. Both kept.
With both renames present the collector now starts with no deprecation
warnings at all, which was the point of the pair.
Conflict in docker/telemetry/otel-collector-config.yaml, in the traces
pipeline: this branch added the attributes/hash processor while upstream
renamed the deprecated otlp exporter to otlp_grpc. Both sides kept — the
processor list keeps attributes/hash and the exporter list takes the new name.
Checked that the exporter definition key was renamed to match the reference,
and that the collector still loads the merged config.
`docker compose down` keeps the `tempo-data` named volume, so a previous
run's traces stay in Tempo and can satisfy this run's span searches. Pass
-v in cleanup(), and tear the stack down before starting it: a run that
reaches the summary deliberately leaves the stack up, so nothing else
clears it.
Three CI failures, one cause each.
macOS could not compile the tracker: Apple's libc++ has no std::atomic for a
shared_ptr, so the primary template's trivially-copyable assert fired. Publish
through boost::atomic_shared_ptr instead, which every standard library the
matrix covers can build. boost/smart_ptr is already used in this tree. The
header's note no longer claims libstdc++ as the assumption.
Four tests that predate the metrics_endpoint scheme guard set tls_client_cert
and only put traces_endpoint on https, so the new guard threw before the check
each one asserts. They now set both endpoints. Nine tests in the two files set a
client cert; the other five stay correct because the pairing, use_tls and traces
checks all run ahead of the metrics one.
Eleven clang-tidy findings: redundant member initialisers, two aggregate
initialisations that wanted designated form, two unbraced bodies, three
unparenthesised multiplications, and a reserve before a loop that emplaces.
Dropping std::make_shared also left <memory> unused in both files.
The pinned collector warns on every start that "spanmetrics" is a deprecated
alias for "span_metrics". Rename the connector, its pipeline references and
the prose that names it.
The derived metric names are untouched. They come from the connector's
`namespace` setting rather than its component name, so occurrences inside a
metric name such as traces_spanmetrics_calls_total are deliberately left as
they are; renaming those would break every span panel. The rename is applied
only to the bare word, never where it is joined to a metric name by
underscores.
This branch introduces the connector, so the change belongs here.
The pinned collector warns on every start that "otlp" is a deprecated alias
for "otlp_grpc". Rename the trace exporter to otlp_grpc/tempo.
Only the exporter is affected. The otlp RECEIVER keeps its name: it serves
both gRPC and HTTP under one component and is not deprecated, verified by
renaming the exporter alone and seeing the warning stop.
This belongs on this branch because it introduces the exporter, and it is the
last of four deprecated aliases in the collector config; the other three are
owned by later branches in the chain.
Addresses the open review findings on this branch.
The log root was never delivered at all. Docker creates a missing bind-mount
source as root, Config::getDebugLogFile() only warns when it cannot create the
network subdirectory inside it, and Application carries on. The node therefore
looked healthy while writing no debug.log, and Loki stayed empty with no error
at any layer. docker/telemetry/data/logs has in fact been root-owned in a
working checkout since it was first created. A one-shot xrpld-logdir-init
service now creates the directory and hands it to XRPLD_UID/XRPLD_GID,
following the pattern the storage-init service already uses.
Ingested logs carried no node identity, so a multi-node stack collapsed into
one indistinguishable stream while every dashboard filters on
service_instance_id. The receiver now sets include_file_path and lifts the
per-node directory onto the resource attribute service.instance.id, which is
on the allow-list Loki promotes to an indexed stream label. A record attribute
would only become structured metadata and could not be used in a selector.
For that to join anything the directory name has to equal the emitter's
service_instance_id, so the node directories are renamed to match: node$i
becomes Node-$i, and the standalone config writes to logs/xrpld-standalone.
The integration test aborted before reporting. Under set -o pipefail the
grep | head -1 pipeline is killed by SIGPIPE once the log exceeds the pipe
buffer, so the run exited 141 somewhere past a few hundred matching lines and
read as a flaky test. grep -m1 stops on its own. The test also verified the
local file and Tempo but never that a line reached Loki, which is the one hop
this branch adds, so a bounded Loki assertion is added alongside a readiness
wait.
Documentation fixes: the Tempo cross-check counted .data, but Tempo returns
OTLP shape so the array is batches and one trace can span several; the Loki
step used the instant /query endpoint, which rejects a bare log selector with
HTTP 400 and a text/plain body, so jq could never parse it and the step never
printed a number even when ingestion worked. The filelog comment claimed six
fractional digits where the node always emits nine. The two flowcharts used
<br/>, carried no legend, and advertised GetSpan(), which Log.cpp deliberately
avoids in favour of reading the thread-local context directly.
Finally, rename the deprecated collector component names: the pinned
collector warns on every start that otlphttp and filelog are aliases for
otlp_http and file_log. Alloy's otelcol.exporter.otlphttp and
otelcol.receiver.filelog are that product's own component names and are not
deprecated, so they are left alone.
The health_check extension was present on the previous branch and dropped
here with no replacement, while this branch's own TESTING.md still polls
http://localhost:13133/ to decide the collector is ready. That check has had
no listener since, so the documented readiness step cannot pass.
Also add batch to the metrics pipeline. Without it the OTLP metric path
exports one request per instrument; the added delay is bounded by the batch
timeout, well under the Prometheus scrape interval.
Both belong here rather than downstream: this branch owns the metrics
pipeline and is the one that regressed the extension.
Two files conflicted and both were composed rather than taken from one side.
integration-test.sh: kept this branch's spanmetrics names, since the collector
sets namespace: "span" here and traces_span_metrics_* matches nothing, and took
phase-6's --max-time on every probe. The Tempo time bound needed restoring by
hand: RUN_START, the check_span guard and the start/end parameters are on
phase-6 and absent here, so a plain resolution kept phase-6's comment about
bounding the search while shipping no bound. All four pieces are back.
TESTING.md: kept server=otel with the metrics endpoint. Phase-6's template sets
server=statsd and documents prefix, which this branch's OTel path ignores.
The two writer paths run on the consensus and ledger-publish threads, and both
took a single mutex that the nine window getters also held while scanning their
deques. At a week of 4-second ledgers those deques hold about 151,000 records,
the same record stored three times, so a read walked half a million entries and
the writers waited behind it.
Each writer now owns a 128-slot ring and shares nothing with the other, so a
record is one timestamp read and one store. A full ring drops the event and
bumps droppedEvents() rather than blocking a consensus thread; at 4-second
ledgers that needs about eight and a half minutes with no drain, against a
worst normal drain gap of one export interval plus one export timeout.
The windows become a grid of one-minute buckets with a running agreed and total
per span, so a getter reads a published snapshot instead of counting. Missed is
derived from the two, which leaves a late repair touching only the agreed side.
Steady-state footprint drops from about 8 MB to about 91 KB, and window edges
quantise to a minute.
Whichever thread is inside reconcile() owns the decision state; a second caller
returns instead of waiting. Readers take a shared_ptr to the snapshot, so the
values they read cannot be overwritten underneath them.
The clock is now injected, which is what makes bucket expiry reachable in a
test. The suite covers pairing, single-sided misses, the grace boundary, late
repair across a bucket edge and past the window, each window edge exactly, two
grid lengths of steady traffic, ring overflow, and three real-thread cases.
It no longer sleeps: nine sleep_for calls totalling 81 seconds are gone.
Every public method keeps its name and signature.
This branch switches integration-test.sh to [insight] server=otel and adds an
assertion that no StatsD listener is needed, but TESTING.md still described the
metrics it verifies as StatsD-derived and its manual node-config template had no
[insight] stanza at all.
CollectorManagerImp falls through to NullCollector when server is neither statsd
nor otel, so a reader building configs from that template got zero
beast::insight metrics. The template also omitted service_instance_id and
metrics_endpoint, which the script writes; without the former every node is
indistinguishable in the $node dashboard filter.
curl applies no overall timeout of its own, so a server that accepts the
connection and then stops answering parks a poll loop for the rest of the run
and the loop's attempt count stops bounding anything. Add a CURL_MAX_TIME
ceiling and apply it to all 18 executable probes in integration-test.sh.
TESTING.md's manual node-config template also disagreed with what the script
writes, so a reader following it could not reproduce the automated path:
- no [insight] stanza, so no beast::insight metric leaves the node at all and
Step 10b's ten rippled_* assertions cannot pass
- [ips_fixed] listed all six peer ports including the node's own
The log level is deliberately untouched: the template and the script agree on
warning here.
With tls_client_cert set, only traces_endpoint was checked for an https
scheme. Telemetry::makeMetricExporter() attaches the client certificate and
key to the metric exporter whenever use_tls=1, and metrics_endpoint defaults
to a plain http URL, so an operator who set up mTLS and overrode only
traces_endpoint exported every metric in the clear with the configured client
identity unused.
Check both endpoints, and state the requirement under metrics_endpoint and
tls_client_cert in the example config. Four config tests cover an explicit
http metrics endpoint, the omitted-key default, both endpoints on https, and
a one-way-TLS control that must stay accepted.
A StatsDCollector polls its metrics only after onCollectionReady(), so
neither test flushed anything: the gauge test timed out and the counter test
passed for the wrong reason. Call it in both, and drop the two includes
whose symbols the file never names.
Three conflicts, all resolved by keeping this branch's rewrite and
re-applying the incoming change onto it:
- 09-data-collection-reference.md: phase-7 rewrote both attribute tables,
so the incoming table would have reverted them. Kept phase-7's and
re-applied the two "XRPL epoch" spellings.
- integration-test.sh: phase-7 moved these checks from StatsD to OTel and
no longer defines check_statsd_metric, so only this side compiles.
- TelemetryConfig.cpp: the incoming side carried networkTypeFromId(), which
this branch already has. Kept one definition and took the incoming
doc wording, which the auto-merged body below it already matches.
readability-identifier-naming wants lower_case for a namespace, so
clang-tidy failed on this file under warnings-as-errors. All seven use
sites move with the declaration.
The rename script rewrites "Ripple epoch" to "XRPL epoch", so the old
spelling in a tracked .md makes the check-rename job fail on a dirty tree.
The attribute keys are left alone: the script's pattern needs a space, and
those keys are a cross-layer contract.
