Two conflicts, both additive on each side.
TelemetryConfig.cpp: include blocks only. This branch added FileUtilities.h for
the certificate readability checks; upstream added <limits> and <optional> for
the bounds parser. Both kept.
The TelemetryConfig test: this branch's mutual-TLS cases and upstream's
batch-bounds cases were added at the same positions, so the file is rebuilt from
both stages and carries all 32 tests. Two shared cases were each edited by one
side only, so the edited side wins in each: upstream asserts the batch defaults
in parse_empty_section, and this branch's parse_full_section writes a real
certificate file, which is now required since the parser opens it.
One conflict, in 06-implementation-phases.md: this branch had rewritten the
phase-4 task table with a Status column, a descoping note and a Spans Produced
section, while upstream corrected the class name in the old plain table. This
branch's section is kept and the name correction re-applied to its 4.1 row.
One conflict, in SpanGuard.h: this branch added struct TraceBytes and upstream
added enum SpanRole at the same position after TraceCategory. Unrelated
declarations, so both are kept.
Two conflicts, both resolved by composing the sides rather than taking one.
cfg/xrpld-example.cfg: this branch had moved the batch-processor keys under
their own heading while upstream edited them in place, so a merge-both would
have documented them twice. Upstream's range sentences are applied to the
relocated block and the head-sampling note keeps its position.
02-design-decisions.md: the summary table changed on both sides for different
reasons. Upstream renamed ledger_index to current_ledger_seq and ledger_seq;
this branch had corrected the PathFinding row to the keys it actually emits.
Both are kept.
childSpan() takes the span name verbatim, so a bare op:: suffix names the span
"process" rather than "rpc.process". The same defect was corrected in the
SpanGuard and Telemetry examples; this is the last copy.
startRoundTracing runs as an argument to Consensus::startRound, so it creates
consensus.round before startRoundInternal applies the new mode. Reading mode_
there recorded the previous round's value, and a validator switching from
observing to proposing got a round span labelled observing that nothing corrected.
The attribute is now written in onModeChange, from the mode being applied. All
three MonitoredMode::set paths funnel through there, so round start, a wrong-ledger
switch and a bow-out all correct the parent span with one statement. Every path
reaches it under RCLConsensus::mutex_ on the thread that created the span.
The stale write is removed rather than kept alongside: neither Consensus::startRound
nor startRoundInternal has an early return before mode_.set, so every round span is
stamped. If a future path ever skipped it the attribute would be absent, which reads
as a gap, instead of confidently wrong. onClose also sets consensus_mode, from the
engine's own parameter, and is correct as it stands.
Also tests addEvent's attribute overload on a live span, reading the exported event
name and each value back off the in-memory exporter. It was previously only ever
called on a null guard, so a dropped attribute exported nothing and failed nothing.
Span kind was derived from TraceCategory alone, so every Rpc-category span was
kServer. A category cannot tell an inbound handler from the internal work under
it, and trace backends pair kServer with kClient, so internal spans left as
kServer become unpaired edges in a service graph and read as extra inbound
requests.
SpanRole is a new xrpl-owned enum, orthogonal to TraceCategory: the category
names the subsystem and gates the span on config, the role says whether the span
handles a remote call. It is a defaulted fourth parameter on span(), freshRoot()
and the ScopedSpanGuard equivalents, defaulting to SpanRole::FromCategory, so no
existing call site changes. resolveSpanKind() applies an explicit role and falls
back to the category map, which keeps its single responsibility. The
telemetry-disabled stubs mirror all four signatures.
No call site passes a role yet. The two that need it are on a later branch.
Also fixes a ScopedSpanGuard example that passed a bare op:: suffix to
childSpan(), which takes the name verbatim. Naming the child rpc.command made it
a child of rpc.command.<cmd>, inverting the hierarchy, so the example's parent is
now rpc.process and the command attribute moved onto the command span.
Review feedback on the RPC integration PR.
The childSpan examples could not work as written. childSpan() takes its parent
from the ambient context and uses impl_ only as a liveness gate, so an unscoped
SpanGuard parent produced two siblings rather than a parent and child. The parent
is now a ScopedSpanGuard, the child no longer reuses the parent's name, and the
examples pass a full dotted constant because childSpan() takes the name verbatim.
Five of the ten Rule D tests could not fail. Four passed an empty L1 key set,
which makes the rule skip validation altogether; the fifth asserted an empty
result against an escaped-quote selector that extracted no labels at all. Each
now passes a nonempty L1 set and carries a known-bad label in the same
expression, so it asserts both that the intended labels are accepted and that
Rule D ran. Verified by disabling the rule: the old tests stay green, the new
ones all fail.
Span kind is not fixed here. categoryToSpanKind and the span factories belong to
the telemetry library, so the role parameter is routed to that branch, and the
two call sites here follow once it exists.
Addresses review findings on the native-metrics work.
StatsDCollector::onTimer drained the send buffer inside the polling_ gate. That
gate holds back hook handlers until the application's services are built, but
sendBuffers() is socket I/O. StatsDEventImpl derives only from EventImpl, so it
never enters metrics_ and posts straight to the buffer; its |ms timings piled up
before onCollectionReady and were dropped after onCollectionStopping. The drain
now runs every tick, and outside metricsLock_, so onCollectionStopping no longer
waits on a UDP flush.
TelemetryImpl's constructor left meterProvider_ set when initMetrics() threw.
initMetrics publishes globally as its last step, so a throw left getMeter()
callers holding a provider nothing else could reach. Reset it in the catch.
~ApplicationImp caught only std::exception around telemetry shutdown while the
callees reach third-party SDK code, so a foreign exception would have terminated
the process. Added a logging catch-all.
ValidationTracker's hard trim evicted by unordered_map bucket order. It now
evicts oldest-first, so the entry dropped under pressure is the one least likely
to still reconcile.
The GetMeter test restored the global meter provider only on the success path,
and ASSERT_TRUE early-returns past it. Uses xrpl::ScopeExit instead.
The hook debounce window is a named constant rather than a bare 500 in a
comparison, and the metric export cadence becomes operator-configurable through
metric_export_interval_ms and metric_export_timeout_ms. Both are range-checked:
the SDK warns and silently substitutes its own 60s/30s defaults when the timeout
is not below the interval, so an unchecked value would slow export rather than
speed it up. Parsing uses a signed representation because lexical_cast<uint32_t>
accepts a leading minus and wraps it.
Naming corrections: CollectorManager documented exported_instance, which no OTel
dashboard uses; node-health queried job_count where the exported name is
jobq_job_count; network-traffic and overlay-traffic-detail referenced an
undeclared DS_PROMETHEUS variable; the counter table omitted the _total suffix
the Prometheus exporter appends; the plan docs and task list carried an xrpld_
prefix formatName never applies; and OTelCollector::New()'s contract promised its
instanceId, serviceName and networkType arguments were read, contradicting the
definition that marks them unused.
NetworkOPs.cpp include block: develop replaced DeliveredAmount.h,
MPTokenIssuanceID.h and NFTSyntheticSerializer.h with the single
rpc/detail/SyntheticFields.h. Kept that and this branch's two telemetry
includes.
Two different facts were sharing one attribute key. The sync-diagnostics
branch already emits validation_status on consensus.validation.accept,
carrying what the validation store did (ValStatus: current, stale,
bad_seq, multiple, conflicting, unknown) with a mapping function and
tests behind it. This branch then added validation_status on
consensus.validation.receive for which exit the receive path took
(queued, dropped_diverged, dropped_load).
One key with two value domains means any aggregation that does not also
filter on span name mixes them. Merging the two branches also produced a
duplicate constexpr declaration in one header, which is how the compiler
surfaced it.
The established accept-path key keeps its name; this one becomes
validation_receive_status, qualified by the span phase it describes. The
values are unchanged.
One [telemetry] key served both OTLP signals, and the metrics URL was
derived from it by suffix-swap: strip a trailing slash, strip a known
signal path if present, append the wanted one. Anything not ending
/v1/traces therefore posted metrics to the traces path, and the OTLP
version was pinned in code where an operator could not reach it.
Adds metrics_endpoint alongside traces_endpoint. Both are full URLs used
verbatim, so traces and metrics can go to different collectors, or to one
whose OTLP paths are not the defaults. signalEndpoint(), kTracesPath and
kMetricsPath are gone; nothing derives an endpoint from another.
The startup log names both URLs, since with two independent endpoints
there was otherwise no way to see where metrics were going.
Also drops exporter=otlp_http from the shipped config and the test
fixture. No branch in the chain reads an `exporter` key: it was a real
Setup member in the first phase-1b implementation, removed when only
OTLP/HTTP was wired up, and already deleted from TESTING.md once on the
same grounds.
Three conflicts, all composed rather than resolved by taking a side:
- TelemetryConfig.cpp: phase-6 kept networkTypeFromId file-local with
[[nodiscard]]; phase-7 had relocated it to public scope for
Application.cpp. Kept phase-7's relocation, so one definition remains.
The [[nodiscard]] survives on the declaration in Telemetry.h.
- Telemetry.cpp x2: phase-7 added getMeter overrides, phase-6 added
[[nodiscard]] to the startSpan below them. Kept both, and put
[[nodiscard]] on getMeter too.
- TESTING.md: phase-7 had the right metric name (span_calls_total, which
the spanmetrics namespace produces) but the wrong label. Its
xrpl.rpc.command appears nowhere else in the branch; the attribute is
bare `command`, which is what the dashboards query. Took phase-7's
metric with the correct label.
Both signalEndpoint call sites follow the renamed member. signalEndpoint
itself is left in place: removing it and adding metrics_endpoint is a
design change, not part of propagating a rename.
Two conflicts, both composed rather than taking a side.
SpanGuard.h: phase-4 added the followsFrom parameter and its @param block;
phase-3 added the @return line. Kept both.
Telemetry.cpp: phase-4 added getConsensusTraceStrategy() immediately above
getTracer()'s return type, where phase-3 added [[nodiscard]]. Kept both, in
both implementation classes.
[telemetry] endpoint carried one OTLP signal while its name implied it
covered every signal. That asymmetry is what let the metrics URL be
guessed later by rewriting this one's path suffix, so anything not
ending /v1/traces silently posted metrics to the traces path.
Renames the key to traces_endpoint and Setup::exporterEndpoint to
tracesEndpoint. The default value is unchanged and the URL is still used
verbatim, with no path derived from it. The startup log line and the
compose-file example name the new key, the latter being where an
operator copies it from.
No metrics_endpoint is added here: this branch has no metrics pipeline,
so the key would parse into a member nothing reads.
Nine std::count_if calls over window1h_ and window7d_ took an iterator
pair; std::ranges::count_if takes the container. One hand-rolled
erase-while-iterating loop becomes a single std::erase_if: pending_ is a
hash_map, which is a std::unordered_map alias, so the C++20 overload applies.
The hard-trim loop below it is left as a loop on purpose. It re-tests
pending_.size() every step to stop as soon as the cap is met, which erase_if
cannot express.
The accessors return values a caller must use: 13 in ValidationTracker.h, the
two Unit.h mappers, the two HistogramBuckets.h helpers, getMeter() and
networkTypeFromId(). No caller in the chain discards any of them.
The trace-context checks validate bytes received from a peer, so a
discarded result means untrusted input was accepted unchecked. hashSpan(),
txReceiveSpan() and txProcessSpan() return an RAII guard; discarding one ends
the span on the same line it began.
The telemetry-disabled twins of both hashSpan overloads already carried the
attribute, so the two #ifdef arms now agree. Both hashSpan overloads also
gain the @return line their siblings already had, now that the result cannot
be dropped.
No caller in the chain discards any of these results.
getInstance(), getTracer(), both startSpan() overloads and networkTypeFromId()
return values that a caller must use. A discarded startSpan() result destroys
the returned span immediately, so the span opens and closes with no content.
Six methods in Telemetry.h already carried the attribute, on the base and on
every override. The new attributes follow that: the overrides in Telemetry.cpp
and NullTelemetry.cpp get it too, because [[nodiscard]] is not inherited and a
call bound to the derived type would otherwise be unchecked.
No caller anywhere in the chain discards any of these results.
consensus.validation.receive is created before the drop decision, so one
span name covered three exits with unrelated cost profiles: two drop
paths that end in microseconds, and a queued path whose handle is moved
into the job and so covers job wait plus checkValidation.
No existing attribute separated them. validation_trusted=true implies
the queued path, but validation_trusted=false spans both drop paths and
the queued path, so any quantile over the span mixed the populations.
Both drop paths are live in the default config, which sets
relayUntrustedValidations.
Adds validation_status, set once on each exit rather than as a default,
following the tx_status precedent in the same file: dropped_diverged,
queued, dropped_load. On the queued path it is set before the handle is
moved into the job.
Harness and docs:
- integration-test.sh queried traces_span_metrics_* for spanmetrics, but this
branch sets the connector namespace to "span", so those two checks matched
nothing and failed. The dashboards and runbook had moved; the script had not.
- The same script queried eight native metric names with a product prefix and
capitals that formatName() cannot produce: it lowercases, maps '.' and ' ' to
'_', and prepends nothing. Corrected against the runbook tables.
- TESTING.md carried the same stale spanmetrics names and a jq example reading
a Prometheus label that does not exist.
- The runbook now records where each part of a derived metric name comes from,
since only the namespace is ours to choose.
Collector:
- OTelCounterImpl::increment silently dropped a negative amount. An OTel
counter takes unsigned deltas, so assert and let a release build under-count
rather than wrap.
- OTelGaugeImpl::increment computed current + amount in int64, which is
undefined on overflow, and the clamp ran afterwards so it could not help.
Check the headroom first. set() now clamps rather than casting a uint64 above
INT64_MAX to a negative, which is what made underflow reachable.
- The meter scope was two bare literals. They are constants now, and
Telemetry.cpp static_asserts them equal to kMeterName and kMeterVersion:
beast cannot include the telemetry header, so a build failure is the only way
to catch the copies drifting.
- formatName uses views::transform and ranges::to, as Backend.cpp already does.
- Unused constructor parameters take [[maybe_unused]] instead of (void) casts.
- The destructor logged "shutting down" and "stopped" with nothing between.
initMetrics was 79 lines doing four jobs. The exporter and the histogram views
are separate functions now, addUnitView is a member rather than a lambda
capturing this, and the export interval and timeout are named. It also derived
the metrics URL from the traces URL by suffix swap, which sent metrics to the
traces path whenever the configured URL had any other shape; both URLs now come
from one rule that handles a bare host, a trailing slash and either signal path.
The default setServiceInstanceId() body ignores its argument. Use the
attribute rather than a (void) cast: the codebase already uses it 104 times
and the build is C++23.
Both comments pointed at a state the squash merge does not publish.
- Consensus.h: 'yields a null guard, same as before' had no antecedent in the
round or the function. Say instead that a null guard makes the setAttribute
calls below no-ops, which is what SpanGuard's impl_ guard does.
- ConsensusSpanLabels.h: 'Split from ConsensusSpanNames.h' describes a split
performed entirely within this change; both headers first appear here. The
dependency rationale and the diagram are unchanged.
Comments only, no behaviour change.
The class docs for the OTel insight bridge described how the code got to its
current shape rather than what it does. Those comparisons resolve against a
revision that the squash merge does not publish, so they read as confidently
wrong once merged.
- OTelCollector: state that it is selected by [insight] server=otel as an
alternative to StatsDCollector. It replaced nothing; CollectorManager still
selects StatsDCollector for server=statsd.
- Unit: give the reason an Event needs an explicit unit, and the consequence
of omitting one, without narrating what the two backends previously assumed.
- OTelEventImpl: say that HistogramBuckets.h is the single owner of the bucket
edges and why a copy goes stale, instead of quoting a superseded edge list.
- HistogramBuckets: drop 'just as 5 s censors them today'. The millisecond
ladder tops at 120000, so nothing is censored at 5 s.
Comments only, no behaviour change.
A comment that describes an earlier state of its own branch documents something
no reader can look up, because the branch is squash-merged and the state it
contrasts against never reaches the merged history. Four passages in
HistogramBuckets.h and one in Telemetry.cpp did exactly that, and the
OTelCollector and Unit.h wording implied a transition rather than a fact.
- HistogramBuckets.h: the ladders now explain the invariant they enforce,
instead of recounting where the edges used to live and how they drifted.
- Telemetry.cpp: one view per unit means a byte count is bucketed on the byte
ladder, stated directly rather than as something it stopped inheriting.
- OTelCollector.cpp: the collector is described by what it is, a thin adapter
over the shared pipeline, rather than as a shim that gave up an exporter.
- Unit.h: the StatsD path is out of service as a present fact, and the contract
its wire format would break is an external protocol one.
Comment text only. No declaration, signature or emitted value changes.
