gtest's EXPECT_EQ expands to an if/else, so an unbraced if around it is a
dangling-else error with warnings treated as errors. Found by the local
build of xrpl_tests.
Conflicts, all resolved by keeping phase-9's rewritten text and applying
the upstream change to it:
- PathRequest.cpp: drop the Redaction.h include, keep MetricMacros.h.
- XrplCore.cmake: keep the PUBLIC-link comment, note the SField table use.
- OpenTelemetryPlan.md, 02-design-decisions.md, 05-configuration-reference.md,
telemetry-runbook.md: account addresses are emitted raw; no hashing at
any layer; tx.process lists the per-role account attributes.
Conflicts in src/libxrpl/telemetry/TelemetryConfig.cpp and its test:
phase-5's "require an https endpoint whenever use_tls=1" met phase-7's
version of the same guard, which already covers both the traces and the
metrics endpoint. Kept phase-7's side.
A transaction names one or more accounts: the sender in Account, and by
type a Destination, Owner, Issuer, Holder and so on. tx.process now
carries each top-level account-typed field as its own attribute, keyed
tx_<field> in lower snake case (tx_account, tx_destination, ...), so an
account can be searched for in traces whatever role it played.
Addresses are public ledger identifiers and are emitted raw.
The keys live in TxAccountSpanNames.h in libxrpl, with a field-to-key
table in TxAccountSpanNames.cpp. A library test walks TxFormats and the
SField registry: every account field a transaction can carry has a key,
and no field that only ledger entries carry has one. An empty account
field is skipped rather than rendered as the zero address.
Rewrite the privacy policy (design decisions section 2.4.4) and the two
plan summaries that still described account hashing and a configurable
redaction. Add the two account attributes to the path-finding attribute
table and describe pathfind_dest_currency as the rendered asset.
An XRP account address is a public ledger identifier drawn from an
enumerable set. An unsalted hash of it is reversible by lookup, so it
protected nothing and only broke the join against explorers, RPC
responses and logs that show the same address.
- pathfind_source_account and pathfind_dest_account carry the request's
r-address. A value that does not parse as an r-address is not emitted,
so a malformed or mistaken request value never reaches a span. Both
handlers share setAccountAttribute() in PathFindSpanAttributes.h.
- pathfind_dest_currency is to_string(Asset): "XRP", "<issuer>/<CUR>",
or the MPT issuance id.
- The collector's attributes/hash processor is removed. No layer hashes.
- redactAccount() stays available; its header no longer claims to sit in
the emit path.
The alert notification ended with `Values: A=0 B=0 C=1`, the internal ids of the
three query steps, which mean nothing to a reader. The previous attempt removed
that line by replacing the whole email body with a custom template. That worked,
but it threw away everything else Grafana's default email gives: bold section
headings, a styled label table, annotations rendered as clickable links, and the
View and Silence buttons. The result read as a flat block of text and raw URLs.
Fix the line instead of the body. Rename all 13 rules' steps from A/B/C to names
that say what they hold -- close_rate_5m, close_rate_now, is_stalled -- so the
default body's value line reads close_rate_5m=0 close_rate_now=0 is_stalled=1.
The threshold step is named is_* so a 0/1 verdict is obviously a verdict and not
a rate.
Drop the email subject/message override so the default layout returns, and drop
the now-unused xrpld.email.body template. Slack keeps its own body: Grafana
escapes HTML in an email message, so a custom email body can only ever be plain
text, while Slack needs mrkdwn and <url|label> links. One body cannot serve both,
and only Slack benefits from having one at all.
Verified by provisioning the real files into Grafana: all 13 rules load with the
new step names and evaluate health=ok, and the Slack template renders with no
parse error.
The Collector interface said instruments are created in the owning class's
constructor, but not why, or what the boundary is. onCollectionReady() runs
once, after every service the hooks read exists; a collector starts polling and
arms its observable instruments at that point. A gauge made after it is never
armed, so it is never exported, and nothing reports that.
Name the boundary on the interface, and the consequence on makeGauge(), the one
instrument where being late fails silently.
Sharing one body between the Slack and email receivers made the email
unreadable. Email cannot render Slack markup, so the bold asterisks, the
backticks and the `🚨` shortcodes all arrived as literal
characters, and a `<url|label>` link could not become a link at all -- it dumped
the whole dashboard URL inline. Four panel links then buried the prose.
Split the body in two. The Slack body keeps mrkdwn. The email body is plain
text, one fact per line, with each dashboard link on its own labelled line.
Both still share the title and the node-identity fallback.
Measured while fixing this, and now recorded in templates.yaml: email escapes
any HTML in the message, so `<br>` arrives as `<br>` and no tag or anchor
is possible; but a newline in the template does become a real `<br>`, so line
breaks are the only layout tool email has.
Also correct two comments that were wrong. A missing template define does not
ship raw template text: it logs one warning and silently delivers Grafana's
default body, the value dump this file exists to remove, while the rule still
reports health=ok. And the Cloud contact point is not email-only; it holds a
Slack receiver and an email receiver, on an instance shared with other teams.
tx.apply carries its own ledger_seq. On the consensus path ledger.build,
ledger.store, ledger.validate and the queue spans nest under
consensus.accept.apply, which doAccept opens as a scoped guard, so the passage
that said no ambient span exists there is replaced by the edge it now has. The
per-stage failure-rate comment no longer claims the stages leave status unset
on a failing result; all three set an error status.
Grafana's default notification body appends every expression node's value and
every label, so an alert arrived as `Value: A=0, B=0, C=1` over a five-line
`key = value` dump. The refIds mean nothing to a reader and the labels repeat
the title.
Add templates.yaml and point both Slack receivers and the email receiver at it,
so the body is the rule's own description plus its remediation line, and any
panel_* annotation renders as a link.
Rewrite all 13 descriptions to be status-neutral, since the same annotation is
rendered when the alert resolves: a firing-only wording made a resolved
notification claim the node had stopped closing ledgers while reporting a
healthy rate. The reason to care moves to a new `action` annotation, which the
template prints only while firing. Each description now formats its value with
printf and states its threshold, rather than emitting a bare float.
Also fix the Slack title, which referenced a `rulename` label that no rule sets
and so rendered blank; `alertname` is the label Grafana always provides.
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.
The scheme guard only ran when tls_client_cert was set, so use_tls=1 against an
http endpoint booted: the CA file was checked for readability, then the exporter
read TLS off the URL scheme alone and sent spans in the clear. An operator who
asks for TLS should get TLS or a startup error.
Widen the guard to every use_tls=1 node. tls_client_cert already requires
use_tls=1, so this strictly widens the old condition. The error message and the
function's description now name use_tls rather than the certificate key.
one_way_tls_on_a_plain_http_endpoint_is_accepted pinned the old behaviour and is
flipped to _throws, asserting the scheme message, the endpoint key and the URL.
Three cases that sent use_tls=1 at an http endpoint while asserting a file error
now set an https endpoint, so the scheme guard cannot be what throws.
All three stop() overrides cleared the global Telemetry instance
unconditionally, which assumes one live Application per process. The unit-test
binary breaks that whenever two environments run, and makeTelemetry() hands it
a NullTelemetryOtel, so the null implementations matter as much as the real one.
Compare against this before clearing, in all three.
An MPT issuance id is a sequence concatenated with the issuer's account id, so
emitting it whole put an account on a span in the clear, while the Issue arm
beside it redacts its issuer. The collector hashes only the two account
attributes, so nothing downstream caught it.
Hash the whole id: one stable token per asset, no issuer. The comment claiming
the id carries no address was wrong and is corrected.
The tx.process span read the fee with STAmount::xrp(), which throws on a
non-native amount. preflight1 rejects such a transaction with temBAD_FEE, but
the attribute runs first, so with tracing on the submit RPC returned
internalSubmit instead. Turning tracing on must not change a response.
Guard on native() and leave the attribute out otherwise. The peer path was
already contained by its own catch.
