(Logs) Submit proven transaction logs #172
Conversation
polydez
force-pushed
the
polydez-logs-submit-proven-transaction
branch
from
a7a3e3a to
9bb8583
Compare
There was a problem hiding this comment.
Thank you! Looks good! I left some comments inline, but also a few general comments:
- Looks like we are missing the RPC part of the flow.
- It seems like child spans inherit fields of parent spans. If this is correct, then we don't need to add things like
COMPONENTto every log message. Same goes for transaction ID which we should be able to set once and then it would be associated with subsequent logs. - For things which may result in kilobytes of data being logged (e.g., full requests/responses), let's use
DEBUGlevel. - Let's make sure that data gets serialized in a usable format. This may require introducing wrapper structs and implementing
Displaytraits on them.
…ForTransactionInput`
…roven-transaction # Conflicts: # store/src/db/mod.rs # store/src/db/tests.rs # store/src/lib.rs
…tting of `AccountId`" This reverts commit 8e77403
|
@bobbinth, I've addressed all comments. If you have a chance, please, take a look! |
…roven-transaction # Conflicts: # block-producer/src/state_view/mod.rs # store/src/state.rs
utils/src/logging.rs
Outdated
| let metadata = envelope.metadata(); | ||
| format!( | ||
| "{{ note_id: {}, note_metadata: {{sender: {}, tag: {} }}}}", | ||
| envelope.note_id().inner(), |
There was a problem hiding this comment.
For note ID, let's use to_hex() here.
store/src/main.rs
Outdated
| #[instrument(target = "miden-store", skip_all)] | ||
| async fn query( |
There was a problem hiding this comment.
Why do we need to instrument this? As I mentioned previously, this command will be run from the CLI - so, not sure we need to collect any logs here.
store/src/server/mod.rs
Outdated
| pub async fn serve( | ||
| config: StoreConfig, | ||
| db: Db, | ||
| ) -> Result<()> { | ||
| info!(target: COMPONENT, %config); |
There was a problem hiding this comment.
Any reason not to add a message here? e.g., so that it looks like so:
info!(target: COMPONENT, %config, "Initializing server");Then, line 28 could be something like this:
info!(target: COMPONENT, "Server initialized");(i.e., I'm not sure we need to log config twice).
store/src/db/mod.rs
Outdated
| pub async fn setup(config: StoreConfig) -> Result<Self, anyhow::Error> { | ||
| info!(target: COMPONENT, %config); |
There was a problem hiding this comment.
Similar comment as above: any reason not to make it:
info!(target: COMPONENT, %config, "Connecting to the database");Also, I'd change "DB" on line 87 below to "database".
rpc/src/server/mod.rs
Outdated
| pub async fn serve(config: RpcConfig) -> Result<()> { | ||
| info!(target: COMPONENT, %config); |
There was a problem hiding this comment.
Similar comment to above: any reason not to change this to:
info!(target: COMPONENT, %config, "Initializing server");Then, we can probably change line 24 to something like this:
info!(target: COMPONENT, "Server initialized");
block-producer/src/server/mod.rs
Outdated
| pub async fn serve(config: BlockProducerConfig) -> Result<()> { | ||
| info!(target: COMPONENT, %config); |
There was a problem hiding this comment.
Let's change this to:
info!(target: COMPONENT, %config, "Initializing server");And line 72 below to:
info!(target: COMPONENT, "Server initialized");…roven-transaction # Conflicts: # block-producer/src/server/mod.rs # rpc/src/server/mod.rs # store/src/server/mod.rs # utils/src/config.rs
Sample output:
Sample output (error):