Add ouroboros-consensus:tracing sublibrary - #2244
Conversation
b003a0e to
7b0838e
Compare
e9b2bd8 to
8f27e3f
Compare
| ../ouroboros-consensus-diffusion | ||
| ../ouroboros-consensus-protocol | ||
| ../ouroboros-consensus.cabal | ||
| ../tracing |
There was a problem hiding this comment.
Should tracing-test join the golden list further down (nix/haskell.nix:69-74)? As it stands it gets neither extraSrcFiles nor --no-create, and tracing/golden sits outside its hs-source-dirs: tracing/test, so haskell.nix prunes the golden file out of the component source. tasty-golden then creates the missing file and reports a pass, which I guess makes the nix check green without comparing anything. The byron/shelley/cardano suites need extraSrcFiles for the same reason, even though the package is already in the fileset above.
Something like:
packages.ouroboros-consensus.components.tests.tracing-test = {
testFlags = lib.mkForce [ "--no-create --hide-successes" ];
extraSrcFiles = [ "tracing/golden/**/*" ];
};A nix build .#checks.x86_64-linux.<ghc>:ouroboros-consensus:test:tracing-test and a grep for Golden file did not exist; created would settle it either way.
| -- and the namespace-indexed methods. That makes them cheap enough to run over | ||
| -- every traced type, and they catch the mistakes that are easy to make when | ||
| -- writing an instance by hand -- a namespace left out of 'allNamespaces', a | ||
| -- typo that makes a namespace unreachable, a missing severity (which silently |
There was a problem hiding this comment.
I wonder if the docstring promises a bit more than the checks deliver. "A namespace left out of allNamespaces" is the case where some constructor's namespaceFor returns a namespace that is not in the list, but every check below iterates allNamespaces itself and namespaceFor is never called. So that particular mistake still passes, and so does a typo, as long as it appears in both places.
I do not think that is fixable cheaply. Catching it needs values, and these types have no Arbitrary or Enum. Should we narrow the wording instead, so the next reader knows what is and is not covered? Something like "every namespace in allNamespaces has a severity, a privacy, a detail level and documentation, and the tree is addressable", plus a note that the namespaceFor direction is not checked.
| , ("proposing", ConwayProposing (AsIx 5)) | ||
| ] | ||
|
|
||
| purposesByItem :: [(String, ConwayPlutusPurpose AsItem ConwayEra)] |
There was a problem hiding this comment.
Should purposesByItem cover all six the way purposesByIndex does? Certifying, voting and proposing are missing, and going by the comment just above they are the {"item": ...}-wrapped ones, so they are the cases most likely to drift. The unknownPurpose fallback and renderTxIn have no entry either.
Adding them looks like three lines, and the golden file grows by three.
| (LogFormatting peer, Show peer) => | ||
| LogFormatting [TraceLabelPeer peer (FetchDecision [Point header])] | ||
| where | ||
| forMachine DMinimal _ = mempty |
There was a problem hiding this comment.
Are these two instances still needed? cardano-node master removed the [TraceLabelPeer peer (FetchDecision [Point header])] instances under #6667, together with the connectedPeers metric documentation, because nothing emits them and their Accept/Decline/EmptyPeersFetch namespaces were the stale ones the configuration consistency check rejected.
Carrying them into the sublibrary brings back both the dead instances and a metricsDocFor advertising a metric that asMetrics at line 750 never produces. Our own MetaTrace test only covers TraceDecisionEvent, which I guess confirms nothing else needs them.
Should we drop them and keep just LogFormatting (FetchDecision [Point header]), the way master does?
| -- projections below matched. | ||
| -- | ||
| -- Those projections cover every purpose the ledger currently defines, but they | ||
| -- are pattern synonyms without a @COMPLETE@ pragma, so GHC cannot check that |
There was a problem hiding this comment.
I think there is a COMPLETE pragma: cardano-ledger-api's Cardano.Ledger.Api.Scripts has one covering the seven AnyEra*Purpose synonyms, AnyEraGuardingPurpose included. So GHC does check exhaustiveness here, and both renderers match six of the seven.
Which makes the fallthrough reachable today rather than in some future era: a Dijkstra guarding redeemer renders as {"kind":"UnknownPlutusPurpose"}. Since we are matching the sixth case explicitly anyway, should we add the seventh and drop the wildcard? Then GHC tells us about the next era, instead of a marker turning up in someone's logs.
Either way the comment wants a small correction.
| directory | ||
| latex-svg-image | ||
|
|
||
| library tracing |
There was a problem hiding this comment.
Should tracing join the libraries array in scripts/ci/check-changelogs.sh:12-20 while we are adding it? As it stands a later change to this sublibrary's .hs files never asks for a fragment, and it is public API: HasIssuer, ConvertTxId, and every log shape an operator parses.
The path would be tracing, since the sublibrary's hs-source-dirs is the directory itself rather than a src/ subdirectory.
Relatedly, I do not see a fragment for this PR, and no no changelog label. I guess CI will point that out anyway.
| ) where | ||
|
|
||
| import Cardano.Logging | ||
| import Cardano.Prelude (maximumDef) |
There was a problem hiding this comment.
maximumDef is the only thing we take from cardano-prelude, at lines 909 and 914. Is it worth a dependency on a new public library for that? foldr max Notice (map sevLedgerEvent events) gives the same answer, and then the sublibrary's dependency list is one entry shorter.
The main ouroboros-consensus library does not depend on cardano-prelude today, so this adds it to the tree rather than reusing something already there.
Introduce a new public sublibrary 'tracing' with instances for Consensus types, moved from cardano-node.
Moving it from cardano-node/cardano-api.
a7e5664 to
332726c
Compare
Reimplement, off cardano-api, the Shelley-era JSON rendering helpers that the era tracing instances need: bech32 stake/reward addresses (CIP-19, via the bech32 package), hex script hashes, and an era-generic script-purpose renderer built on cardano-ledger-api's AnyEraScript projections (replacing cardano-api's per-era Alonzo/Conway plutus-purpose rendering and its ShelleyBasedEra/ AlonzoEraOnwards era witnesses). Output matches cardano-api's.
Move Cardano.Node.Tracing.Era.Byron to Ouroboros.Consensus.Tracing.Era.Byron; inline the trivial textShow helper (its only cardano-api use) and drop the imports the old -Wno-unused-imports hack was masking.
Move Cardano.Node.Tracing.Era.Shelley to Ouroboros.Consensus.Tracing.Era.Shelley,
off cardano-api:
- use the new Era.Shelley.Render helpers (bech32 addresses, script hashes,
era-generic plutus purposes) instead of the cardano-api ones
- drop the cardano-api era witnesses (ShelleyLedgerEra/IsShelleyBasedEra/
shelleyBasedEra/forEraInEon/toScriptIndex): the affected instances are now
phrased directly over the ledger era with an AnyEraScript constraint, and
ExtraRedeemers renders redeemer pointers via renderScriptIndex
- render VRF key hashes via the ledger's own hash (fromVRFVerKeyHash was
always a ledger function, only re-exported by cardano-api)
- inline textShow
- re-add the ToJSON orphans for cardano-data's NonEmptySet/NonEmptyMap that
cardano-node previously got transitively from cardano-api
Builds with no warnings.
HotKey.KESInfo and HotKey.KESEvolutionError are Consensus types, but their
LogFormatting/MetaTrace instances (and the ToJSON orphan for KESPeriod they
need) stayed behind in cardano-node's Cardano.Node.Tracing.Tracers.KESInfo,
even though the HasKESInfo/GetKESInfo classes that feed them had already moved
here. A second consumer of :tracing would have had to reimplement those
instances, and would then have clashed with cardano-node's copies.
Move the instances and traceAsKESInfo here, and split the leftover
Ouroboros.Consensus.Tracing.Queries -- a name inherited from
Cardano.Node.Queries that no longer describes its contents -- along the lines
of what the classes are actually for:
- Ouroboros.Consensus.Tracing.KESInfo: HasKESInfo, GetKESInfo,
traceAsKESInfo, and the instances above
- Ouroboros.Consensus.Tracing.ConvertTxId: ConvertTxId
The instance bodies are unchanged, so the rendered messages and the EKG metric
names stay the same.
The LogFormatting/MetaTrace instances are orphans spread over the Ouroboros.Consensus.Tracing.* modules, four of which (Era.Byron, Era.Shelley, Era.HardFork, Formatting) export nothing else and so have to be imported for their instances alone. Which module carries which instance is not something a consumer should have to track: getting it wrong is silently losing an instance, and it already forced cardano-node to sprinkle `import ... ()` lines over five wiring modules. Add an umbrella that re-exports the name-bearing modules and imports the instance-only ones, so a single import brings the whole set into scope and the internal module structure can change without breaking consumers.
renderScriptPurpose and renderScriptIndex both project a PlutusPurpose through cardano-ledger-api's AnyEraScript pattern synonyms. Those synonyms carry no COMPLETE pragma, so the wildcard is needed to keep the match total -- but falling through to Aeson.Null makes a purpose the ledger has added and we do not know about indistinguishable from one that legitimately rendered as null. Fall through to an explicit marker instead, so an unhandled purpose is greppable in the logs and a distinct shape in the trace schemas. Also record that renderScriptIndex does not reproduce the JSON shape cardano-api's toScriptIndex produced for the ExtraRedeemers field.
The instances were phrased over TraceObjectDiffusion{In,Out}bound with both
type parameters free, gated on an ObjectDiffusionMetricsPrefix class that
supplied the EKG metric prefix. That class was not exported, so the fully
general orphan both blocked anyone from writing an instance for their own
instantiation and gave them no way to extend ours.
The object type is a type family (PerasCert / PerasVote) and so cannot appear
in an instance head, which is what forced the generalisation -- but the
object-id type can, and it is what actually distinguishes the two diffusion
pipelines. Match on it directly and drop the class: a further diffusion kind
adds its own pair of instances rather than an instance of a closed class.
The metric prefixes are unchanged.
Consensus.Tracers has carried perasCertInclusionTracer and perasVoteForgingTracer for a while, but TracePerasCertInclusionEvent and TracePerasVoteForgingEvent had no LogFormatting/MetaTrace instances, so cardano-node could only wire both to a no-op: the events were unobservable. Add the instances, with a namespace, a severity and documentation per constructor. The routine "nothing to do this slot/round" cases are Debug, the decisions and their outcomes Info, and the environment read failure Error. The payloads of the decisions and DB outcomes are rendered via Show, as the object diffusion instances alongside them already do. Also export TracePerasVoteForgingEvent (..) from Ouroboros.Consensus.Node.Tracers, which already exported its cert inclusion counterpart, so both trace types reach a consumer from the module that defines the record holding them.
jsonNonEmptySet/jsonNonEmptyMap were introduced with the claim that they match what cardano-node emitted before, which cardano-node got from cardano-api. Neither holds: cardano-api never mentions NonEmptySet, and cardano-data has no ToJSON instance for its non-empty containers to compare against -- putting the container back in place of the helper does not compile. The helpers are still needed and their output is the only shape such an instance could have; just say so rather than claiming a parity that cannot be checked.
Nothing checked these instances: cardano-node's consistency check runs over the assembled node configuration, in the other repo, and ouroboros-network's tracing sublibraries have no tests at all. Writing one by hand -- as the Peras instances just were -- means hand-listing namespaces, severities and documentation, and a typo in any of them is silent. The checks derive everything from allNamespaces and the namespace-indexed methods, so they need no trace values and run over every traced type: 25 of them here, instantiated at CardanoBlock. A namespace must be non-empty and unique, must have a severity (without one the message cannot be configured), a privacy, a detail level and documentation, and the whole namespace tree must satisfy the same check trace-dispatcher applies to a node configuration. The documentation check found 44 namespaces with no documentFor, or with an empty one. Fixed here: ConsensusStartupException, ClientMetrics and the ten object diffusion namespaces. The rest -- ChainDB, ImmutableDB, LedgerDB and the forge tracer, all inherited from cardano-node -- are recorded in knownUndocumented so that new gaps are still rejected, with a second check that fails if an entry there becomes documented, so the list cannot go stale.
These functions were reimplemented off cardano-api when the era tracing
instances moved here, and had already drifted from it twice without anything
noticing. Pin their output as bytes so that changing it takes an explicit,
reviewed change to a golden file.
Writing it found a third divergence: renderScriptPurpose rendered the spending
purpose's TxIn through cardano-ledger's ToJSON, which shows the index newtype
and yields "<txid>#TxIx {unTxIx = 0}". cardano-api rendered "<txid>#0". Add a
renderTxIn that does the latter.
The golden file was checked by hand against cardano-api-11.5.0.0, the version
cardano-node used before the move: the ScriptWitnessIndex kind/value shapes, the
{"item": ...} wrapper on the purposes that keep their AsItem, the CIP-19 bech32
prefixes, and the txid#index form.
The list of traced types was hand-written and had drifted from Consensus.Tracers: six top-level tracers were missing, all of them ones whose type carries a peer or has no block parameter, which is why they were easy to overlook. TraceGDDEvent, Jumping.TraceEventCsj, Jumping.TraceEventDbf, BlockFetch.TraceFetchClientState, TraceDecisionEvent, KESAgentClientTrace They bring 14 more undocumented namespaces into knownUndocumented, 13 of them KESAgentClientTrace's -- an entire tracer whose messages carry no documentation at all, and which nothing was asserting anything about until now. The peer type is a stand-in: no MetaTrace method looks at it, so this uses () rather than pulling in the node's address types.
LedgerDB, ImmutableDB, VolatileDB, PerasCertDB and PerasVoteDB do not have tracers of their own. ChainDbArgs.updateTracer derives each from the ChainDB tracer, and ChainDB.TraceEvent's allNamespaces maps all of their namespaces in under LedgerEvent, ImmDbEvent, VolatileDbEvent and so on -- cardano-node has no tracer for any of them either. Listing them alongside ChainDB.TraceEvent meant every one of their namespaces was checked twice under two different names, and appeared twice in knownUndocumented: once as ChunkValidation.InvalidChunkFile and again as ImmDbEvent.ChunkValidation.InvalidChunkFile, once as Flavor.V2.BackendTrace.LSM.LSMSnap and again as LedgerEvent.Flavor.V2.BackendTrace.LSM.LSMSnap. Keep ChainDB.TraceEvent, which reaches all of them, and drop the 17 duplicated ratchet entries.
'maximumDef' was the only thing taken from it, in two severity computations. 'foldr max' over the same list gives the same answer, and the main library does not depend on cardano-prelude either, so this keeps a public library out of the dependency tree entirely.
cardano-ledger-api does ship a COMPLETE pragma over the seven
AnyEra*Purpose synonyms, so the wildcard was not future-proofing: it was
reachable today, and a Dijkstra guarding redeemer rendered as
{"kind":"UnknownPlutusPurpose"} in the logs.
Match all seven and drop the wildcard, so that the next era is a compile
error here rather than a marker in an operator's logs. Guarding has no
cardano-api rendering to preserve, so its item renders directly (like
spending and rewarding) and its witness index name is ours.
'purposesByItem' only covered three of the six Conway purposes, leaving
certifying, voting and proposing unpinned -- and those are exactly the
{"item": ...}-wrapped ones the comment above warns about, so the ones
most likely to drift. 'renderTxIn' had no entry either.
Add them, plus the guarding purpose, which needs the Dijkstra era and so
also exercises the other AnyEraScript instance rather than only Conway's.
cardano-node removed the instances for [TraceLabelPeer peer (FetchDecision [Point header])] under IntersectMBO/cardano-node#6667: nothing emits that list, and its Accept/Decline/EmptyPeersFetch namespaces were the stale ones the configuration consistency check rejected. The MetaTrace instance for FetchDecision goes with them; its metricsDocFor advertised a connectedPeers metric that no asMetrics ever produced. What remains is LogFormatting (FetchDecision [Point header]), which TraceDecisionEvent's own rendering needs, matching cardano-node master.
The docstring claimed to catch a namespace left out of 'allNamespaces', but every check iterates 'allNamespaces' itself and 'namespaceFor' is never called, so that mistake -- and a typo appearing in both places -- still passes. Catching it needs trace values, which these types have no 'Arbitrary' or 'Enum' to produce, so narrow the wording instead.
'tracing/golden' sits outside the test component's hs-source-dirs, so without an 'extraSrcFiles' entry haskell.nix pruned it out of the component source, and without '--no-create' tasty-golden then created the missing file and reported a pass. The Nix check was green without comparing anything. Key the golden directories by test name so that adding one is a single line and cannot forget either half.
'tracing' is public API -- HasIssuer, ConvertTxId, and every log shape an operator parses -- but it was not in the list the check walks, so a later change to its .hs files would never ask for a fragment. Its path is the directory itself, since that is its hs-source-dirs.
332726c to
b73febc
Compare
2 participants
Introduce a new public sublibrary 'tracing' with instances for Consensus types,
moved from cardano-node. Also a Claude summary of what needs to be removed after this PR
What
cardano-nodecan delete once it picks this upComparison basis:
cardano-nodec2ebdc87dfe07706a83e52f219e712c60d1b0a56(tag11.1.1,origin/master, 2026-09-04).The sublibrary is a faithful move of node's tracing modules, verified by comparing
formatting-invariant fingerprints rather than raw diffs (the code here is
fourmolu-formatted, so line-by-line diffs are useless): the in-order sequence of
string literals — namespaces, JSON keys, metric names, doc text — and the
token stream with strings, comments and imports stripped.
Result:
BlockReplayProgress,ChainDB,Era/Byron,Era/HardFork,Era/Shelley,FormattingandKESInfoare identical in emitted output (in-order literaldiff = 0).
Consensus.hsis a strict superset — no literal was removed.Node's two recent tracing fixes are both already in:
8f75a3883(BlockFetchnamespace drift) and
9e9cb95fa(severityFor).Delete outright
Cardano/Node/Tracing/Tracers/BlockReplayProgress.hsOuroboros.Consensus.Tracing.BlockReplayProgressCardano/Node/Tracing/Tracers/ChainDB.hsOuroboros.Consensus.Tracing.ChainDBCardano/Node/Tracing/Tracers/Consensus.hsOuroboros.Consensus.Tracing.ConsensusCardano/Node/Tracing/Tracers/ConsensusStartupException.hsOuroboros.Consensus.Tracing.ConsensusStartupExceptionCardano/Node/Tracing/Tracers/HasIssuer.hsOuroboros.Consensus.Tracing.HasIssuerCardano/Node/Tracing/Tracers/KESInfo.hsOuroboros.Consensus.Tracing.KESInfoCardano/Node/Tracing/Era/Byron.hsOuroboros.Consensus.Tracing.Era.ByronCardano/Node/Tracing/Era/HardFork.hsOuroboros.Consensus.Tracing.Era.HardForkCardano/Node/Tracing/Era/Shelley.hsOuroboros.Consensus.Tracing.Era.ShelleyCardano/Node/Tracing/Formatting.hsOuroboros.Consensus.Tracing.FormattingCardano/Node/Tracing/Render.hsOuroboros.Consensus.Tracing.Render+…Tracing.Era.Shelley.RenderRender.hsgoes in its entirety. Itscardano-api-dependent half(
renderScriptHash,renderScriptIntegrityHash,renderScriptPurpose,renderMissingRedeemers,renderIncompleteWithdrawals) has exactly one consumer —node's own
Era/Shelley.hs, which is also deleted — and is reimplemented againstcardano-ledgerinOuroboros.Consensus.Tracing.Era.Shelley.Render. The onesurviving importer,
Tracers/NodeToNode.hs, uses onlyrenderHeaderHashandrenderTxIdForDetails, both of whichOuroboros.Consensus.Tracing.Renderexports.Partial deletion:
Cardano/Node/Queries.hsRemove — all three are verbatim in the sublibrary:
ConvertTxId(class +ByronBlock/ShelleyBlock/HardForkBlockinstances) →Ouroboros.Consensus.Tracing.ConvertTxIdGetKESInfo→Ouroboros.Consensus.Tracing.KESInfoHasKESInfo→Ouroboros.Consensus.Tracing.KESInfoKeep:
MaxKESEvolutions,OperationalCertStartKESPeriod,KESMetricsData,HasKESMetricsData,LedgerQueries, and theNodeKernelDatablock.Import rewrites only (no deletion)
Everything below imports a deleted module and stays; point it at
Ouroboros.Consensus.Tracing(the umbrella re-export, so no orphan instance can besilently lost by importing a subset):
Cardano/Node/Tracing.hsCardano/Node/TraceConstraints.hsCardano/Node/Tracing/Tracers.hsCardano/Node/Tracing/Documentation.hsCardano/Node/Tracing/Consistency.hsCardano/Node/Tracing/Tracers/LedgerMetrics.hsCardano/Node/Tracing/Tracers/NodeToNode.hsCardano/Node/Protocol/Byron.hsCardano/Node/Protocol/Shelley.hsTwo behavioural changes to expect
bench/trace-schemas/TraceMessage.schema.jsonneeds regenerating.ClientMetricshaddocumentFor _ = Just ""in node; here it is keyed on thenamespace and carries real text. Several object-diffusion namespaces gain
documentation too (node has
documentFor _ = Nothing). No namespace is added orremoved by this — only doc strings change.
Cardano/Node/Tracing/Render.hs:290is
Api.AlonzoEraOnwardsDijkstra -> undefined, with unmatched purposes fallingthrough to
Aeson.Null. The sublibrary emits{"guarding": "<script hash>"}and aScriptWitnessIndexGuardingwitness-index name — the latter has nocardano-apicounterpart, so it is a genuinely new string in the logs.