Miscellaneous traces and counters

[newhoggy]

This PR tracks the work done for IntersectMBO/cardano-node#2261.

There is also work being done on the cardano-node repository for the same ticket: IntersectMBO/cardano-node#2289

[nfrisby]

@newhoggy I think this is for IntersectMBO/cardano-node#2261, is that right?

The existing "metric" pattern seems to do the accumulation of counters in cardano-node:Cardano.Tracing.Metrics. So the Consensus Layer should just emit the "I just now served a block" event, and the downstream "metric" will interpret that by atomically incrementing a counter. And so on for the similar metrics.

[nfrisby]

As you noticed, there is no "I just now served a block" event currently emitted. However, we do trace every mini protocol message sent/received, via the following tracers.

https://github.com/input-output-hk/ouroboros-network/blob/389a00b5c2b89b48aa98a96ca10f72f085b6e32c/ouroboros-consensus/src/Ouroboros/Consensus/Network/NodeToNode.hs#L319-L326

And they can be set here. (I think you can ignore the Serialised variants. I don't yet know what TxSub2 is about.)

https://github.com/input-output-hk/ouroboros-network/blob/389a00b5c2b89b48aa98a96ca10f72f085b6e32c/ouroboros-consensus/src/Ouroboros/Consensus/Node.hs#L122-L126

So in a sense the metrics could use those. But maybe more precise events could should be added for some extra stability, even in the "power user" tracer event API. @dcoutts, what do you think?

[nfrisby]

Are you counting roll forwards and rollbacks for the sake of https://jira.iohk.io/browse/CAD-2371? If so, I think you can instead use the existing SwitchedToAFork event. I elaborated in my comments on that Jira Issue -- I think it has sufficient info for that task.

[newhoggy]

This PR is actually for https://jira.iohk.io/browse/CAD-2487.

I understood roll forward to mean a header was served in chain sync. Is that an accurate interpretation or am I way off base?

[newhoggy]

I rebased the PR to an earlier point in history so I don't have to refactor anything when working on cardano-node.

[nfrisby]

I understood roll forward to mean a header was served in chain sync. Is that an accurate interpretation or am I way off base?

I think that's accurate, yes. (My suggestion of SwitchedToAFork above was because I thought this PR might also relate to CAD-2371.)

The only caveat to that interpretation is that we might send the same header to the same peer multiple times. I think this is ultimately a caveat on the wording of the Issue IntersectMBO/cardano-node#2261: it's just slightly ambiguous.

My guess is that these metrics should indeed count each send of a repeated header, in which case I agree with your interpretation: every RollForward message contains exactly one block header. (A RollBackward includes neither a header nor a block -- it's just a "point".)

[coot]

@nfrisby that's a good point. What I am worried about is that when setting logging output off by passing nullTracer (which is possible though node configuration), one will accidentally remove metrics or vice versa. Also tracing is causing quite a bit cpu usage, if we turn them more of them on this will cause more problems. I don't mind if there are logging tracers that are used for statistics as well, I just prefer the default to be keep them separate, and if not put them on the logging side.

[nfrisby]

What I am worried about is that when setting logging output off by passing nullTracer (which is possible though node configuration), one will accidentally remove metrics or vice versa.

Ah, I see. That seems like a good reason to have two different inputs in the Consensus Layer's interface (say, "loggingTracers" vs "metricTracers" or maybe "countingTracers") , which at some point in the Consensus Layer implementation get combined via <>. They could reuse the same event type, which would avoid the concern I shared above about having to classify each event.

That distinction/duplication could be reflected in the -node UX too.

[newhoggy]

I've separated the counters from the tracers. How does it look now?

[deepfire]

@newhoggy, @nfrisby, I'm afraid that the Counter abstraction is making me feel ill at ease -- for several reasons:

1. It violates the principle of separation of concerns that the contravariant tracers establish -- the library user just posts the information -- the policy (interpretation) happens elsewhere.

2. The separation of concerns allows you to concentrate the policy definition -- whereas if you violate it, you disperse that definition across all source points.

3. Another consequence is that tomorrow you can see someone else willing to enrich that trace -- and it will no longer be a Counter -- it already barely is (are two counters bundled together still a counter? Should it be called Counters instead?).

cc @dcoutts

[newhoggy]

There is concern about getting the abstraction right and the refactoring adds downstream refactoring risk.

Choosing the right abstraction for handling counters and tracers separately seems to be the sole remaining concern.

To make progress on IntersectMBO/cardano-node#2261, I will back out the refactoring changes and merge.

Then I'll create a new PR for the refactoring where more time can be set aside for careful consideration.

[newhoggy]

I've moved the refactoring here: #2902

[newhoggy]

bors merge

[iohk-bors]

Build succeeded:

• buildkite/ouroboros-network
• ci/hydra:Cardano:ouroboros-network:required

[nfrisby]

I like the decision to spit the PR into two separate decisions design steps. 👍

Unfortunately, just to clarify for future reference, it looks like Ouroboros.Network.Counter was still included in this PR and used in Ouroboros.Network.TxSubmission.Inbound. It's all local, so not a huge problem.

But if the work on the follow-up PR #2902 doesn't proceed very quickly, let's go ahead and remove that essentially unused Counter stuff in a small fixup PR. Thanks.