fix(sequencer): improve and fix instrumentation

[Fraser999]

Summary

Generally improved instrumentation, including a fix for App::execute_transaction.

Background

There were a couple of async blocks spawned in tokio tasks which were not instrumented, resulting in misleading tracing data for App::execute_transaction. While investigating this, I discovered several instances of tracing fields using Debug output, and also a few functions which seemed to me like they would benefit from being instrumented.

Changes

• Applied the parent tracing span to the two spawned tasks.
• Replaced many Debug fields with Display ones. They were all down to not being skipped via skip_all, so I replaced all instances of skip(...) with skip_all meaning any fields to be included have to be explicitly listed.
• Removed all fields from instrumentation to cut down on tracing/log noise.
• Added instrumentation to the transaction-checking functions and mempool.

Note: I tried to largely preserve the fields which were previously being instrumented, just changing them from Debug to Display. A very few had no Display impl, so I excluded them. I think that almost all of the fields could be omitted in a follow-up PR, since I don't think we get much benefit from including things like addresses or balances in instrumentation, and that only serves to clutter the output. Potentially some of these fields are duplicated in the call chain, so even restricting their inclusion to the relevant top-level function would help.

Testing

Manually ran some tests with instrumentation enabled and eyeballed the output.

[joroshiba]

In general we have added some context information on db writes. I'm unclear on how useful this information is in logging and traces. Want to make sure we avoid noise. Background here of st one point before adding many of the "skip all" pieces logs where so noisy from tracing data they were unusable.

I think it might also be good to add instrumentation in more places (ie mempool service, and app mempool uninstrumented) but good general additions.

[Fraser999]

In general we have added some context information on db writes. I'm unclear on how useful this information is in logging and traces. Want to make sure we avoid noise. Background here of st one point before adding many of the "skip all" pieces logs where so noisy from tracing data they were unusable.

Agreed. The changes I originally made created strictly less noise - I only replaced verbose debug output with more succinct display output. But I agree that even that change led to noisy logs/tracing, so I went ahead and removed all fields from the tracing instrumentation in sequencer. We can re-add any as we see fit in the future, but for now I don't see any of them being useful. I might well have missed places where they would provide useful info in log lines, but in that case we can add them to just the log macro rather than the instrument one.

I think it might also be good to add instrumentation in more places (ie mempool service, and app mempool uninstrumented) but good general additions.

The original commit included instrumentation for the mempool service (handle_check_tx and all the checks were instrumented), but I've now added instrumentation for the public mempool methods.

[SuperFluffy]

Commenting on part of the reasoning behind this PR:

Removed all fields from instrumentation to cut down on tracing/log noise.
The point of contention here is what we view as our primary way of viewing tracing: if OTEL is the primary, then we might want to add even more information in the span. If the tracing-subsciber fmt pretty-printer is the primary, then one needs to be mindful of how cluttered the output becomes.

I tend to view OTEL as our primary way of consuming the data (especially as we go to production), whereas tracing-subscriber is mainly a debugging tool.

I am worried that we throw away information that might be potentially useful to understand how data flows through our system.

[SuperFluffy]

Approval for instrumenting the spawns (with suggestions) and tentatively on all the skip_alls.

I left a comment above re what information we should be putting into spans. It might be fine to have a look at the parent spans (which AFAIK are set up through penumbra-tower-trace), and just inject the requests that trigger the handlers into the parent spans.