Remove some oversized logs #14054

georgeee · 2023-09-07T12:31:45Z

There are many logs emitted by Mina which are unnecessarily large and in some case become oversized.

This PR aims to fix many of such logs, reducing the amount of data printed.

Explain your changes:

Rewrite all of the logs that print directly or indirectly snark work, transaction or state transition data (as these pieces are potentially unbound in size)

Explain how you tested your changes:

Before the change, cluster's logs were polluted by extra-large logs in .mina-config/mina.log* and oversized logs in .mina-config/mina-oversized-logs.log*
After the change, extra large and oversized logs went away

Checklist:

Dependency versions are unchanged
- Notify Velocity team if dependencies must change in CI
Modified the current draft of release notes with details on what is completed or incomplete within this project
Document code purpose, how to use it
- Mention expected invariants, implicit constraints
Tests were added for the new behavior
- Document test purpose, significance of failures
- Test names should reflect their purpose
All tests pass (CI will check this if you didn't)
Serialized types are in stable-versioned modules
Does this close issues? None

georgeee · 2023-09-07T12:33:55Z

!ci-build-me

georgeee · 2023-09-08T11:47:59Z

!ci-build-me

deepthiskumar · 2023-09-11T20:02:11Z

src/lib/transition_frontier/transition_frontier.ml

-        , `List
-            (List.map user_cmds
-               ~f:(With_status.to_yojson User_command.Valid.to_yojson) ) )
+        , `List (List.map user_cmds ~f:(With_status.to_yojson tx_hash_json)) )


It is this change that's causing the verification key test to fail. You'll want to find using transaction hash here

@deepthiskumar I believe I do now exactly as you say: https://github.com/MinaProtocol/mina/pull/14054/files#diff-76bbbeef12ee2aa92308935beebc5d6b45b2343aeb90e561b3e3cbe76e7aa045R289

Also, awkwardly, although nearly all intg tests and other sections of verify test rely on the same zkapp_to_be_included_in_frontier function, only last two sections of verification-key test fail.

deepthiskumar · 2023-09-12T07:27:30Z

!ci-build-me

deepthiskumar · 2023-09-12T07:28:03Z

!ci-build-me

deepthiskumar · 2023-09-12T08:33:25Z

!ci-build-me

deepthiskumar · 2023-09-12T12:43:10Z

!ci-build-me

psteckler · 2023-09-12T21:50:46Z

!ci-build-me

psteckler · 2023-09-13T02:22:59Z

!ci-build-me

There are many logs emitted by Mina which are unnecessarily large and in some case become oversized. This PR aims to fix many of such logs, reducing the amount of data printed.

georgeee · 2023-09-21T21:15:18Z

!ci-build-me

georgeee · 2023-09-21T21:15:25Z

!ci-nightly-me

georgeee · 2023-09-21T21:18:31Z

https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/510 <- integration tests too

deepthiskumar

I think we should have fee payer part of transactions to help with debugging. We can have a summary type for transactions and display fee payer pk, nonce, fee and signature. Resource_pool.Diff.t already has one, maybe we can add transaction summary to it?

deepthiskumar · 2023-09-22T11:42:06Z

src/lib/block_producer/block_producer.ml

+                [ ( "parent_hash"
+                  , Breadcrumb.parent_hash crumb |> State_hash.to_yojson )
+                ]
+              "Producing new block with parent $parent_hash%!" ;


Could you add protocol state please? These logs are useful when debugging

deepthiskumar · 2023-09-22T11:43:53Z

src/lib/mina_graphql/itn_zkapps.ml

                ~metadata:
-                  [ ("command", Zkapp_command.to_yojson zkapp_command)
-                  ; ("error", `String e)
+                  [ ( "signature"


I think the entire fee payer account update (pk and nonce) might be helpful?

deepthiskumar · 2023-09-22T11:44:25Z

src/lib/mina_lib/mina_subscriptions.ml

-                        else state_hash_data ) ) ) ;
+                       [ ( "state_hash"
+                         , `String (State_hash.to_base58_check hash) )
+                       ] ) ) ;


Could you add protocol state here as well?

deepthiskumar · 2023-09-22T11:46:40Z

src/lib/mina_networking/mina_networking.mli

@@ -9,8 +9,7 @@ exception No_initial_peers

 type Structured_log_events.t +=
  | Gossip_new_state of { state_hash : State_hash.t }
-  | Gossip_transaction_pool_diff of
-      { txns : Transaction_pool.Resource_pool.Diff.t }
+  | Gossip_transaction_pool_diff of { fee_payer_sigs : Signature.t list }


Can we add fee payer key and nonce as well? Helps when debugging

deepthiskumar · 2023-09-22T11:47:14Z

src/lib/network_pool/intf.ml

@@ -366,7 +366,8 @@ module type Transaction_pool_diff_intf = sig
  end

  type Structured_log_events.t +=
-    | Transactions_received of { txns : t; sender : Envelope.Sender.t }
+    | Transactions_received of
+        { fee_payer_sigs : Signature.t list; sender : Envelope.Sender.t }


here as well, fee payer key and nonce.

deepthiskumar · 2023-09-22T11:49:41Z

src/lib/network_pool/pool_sink.ml

-                  ; ("diff", summary)
-                  ]
-                "exceeded capacity from $sender" ;
+              [%log debug] ~metadata "exceeded capacity from $sender" ;


I have used these logs a lot when debugging missed/lost transactions. I'd prefer if we can include at least fee payer part of a transaction.

deepthiskumar · 2023-09-22T11:50:28Z

src/lib/network_pool/pool_sink.ml

-                      [%log debug] "Verified diff: $verified_diff"
-                        ~metadata:
-                          [ ( "verified_diff"
-                            , Diff.verified_to_yojson


georgeee · 2023-09-22T18:48:18Z

!ci-build-me

georgeee · 2023-09-22T18:48:29Z

!ci-nightly-me

georgeee · 2023-09-22T18:49:09Z

https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/514 <- integration tests run

deepthiskumar · 2023-09-22T19:08:24Z

!approved-for-mainnet

georgeee requested review from a team as code owners September 7, 2023 12:31

georgeee self-assigned this Sep 7, 2023

Base automatically changed from georgeee/refactor-graphql-file to berkeley September 8, 2023 11:25

georgeee force-pushed the georgeee/remove-oversized-logs branch from fed95d4 to 5008593 Compare September 8, 2023 11:47

deepthiskumar reviewed Sep 11, 2023

View reviewed changes

georgeee force-pushed the georgeee/remove-oversized-logs branch from 5008593 to e402644 Compare September 11, 2023 22:19

georgeee and others added 5 commits September 21, 2023 23:14

Remove some oversized logs

f876a63

There are many logs emitted by Mina which are unnecessarily large and in some case become oversized. This PR aims to fix many of such logs, reducing the amount of data printed.

Do not print block in uptime_service.ml logs

e185fd0

Nit: rename variables after refactor

c9d0cdd

log transaction hash in intg tests

7d1c2e1

show transaction

9324696

georgeee force-pushed the georgeee/remove-oversized-logs branch from c8653bd to 9324696 Compare September 21, 2023 21:15

deepthiskumar requested changes Sep 22, 2023

View reviewed changes

Trivo25 approved these changes Sep 22, 2023

View reviewed changes

Address review comments

430068d

deepthiskumar approved these changes Sep 22, 2023

View reviewed changes

georgeee merged commit f611438 into berkeley Sep 22, 2023
36 checks passed

georgeee deleted the georgeee/remove-oversized-logs branch September 22, 2023 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some oversized logs #14054

Remove some oversized logs #14054

georgeee commented Sep 7, 2023 •

edited

georgeee commented Sep 7, 2023

georgeee commented Sep 8, 2023

deepthiskumar Sep 11, 2023

georgeee Sep 11, 2023

deepthiskumar commented Sep 12, 2023

deepthiskumar commented Sep 12, 2023

deepthiskumar commented Sep 12, 2023

deepthiskumar commented Sep 12, 2023

psteckler commented Sep 12, 2023

psteckler commented Sep 13, 2023

georgeee commented Sep 21, 2023

georgeee commented Sep 21, 2023

georgeee commented Sep 21, 2023

deepthiskumar left a comment

deepthiskumar Sep 22, 2023

deepthiskumar Sep 22, 2023

deepthiskumar Sep 22, 2023

deepthiskumar Sep 22, 2023

deepthiskumar Sep 22, 2023

deepthiskumar Sep 22, 2023

deepthiskumar Sep 22, 2023

georgeee commented Sep 22, 2023

georgeee commented Sep 22, 2023

georgeee commented Sep 22, 2023

deepthiskumar commented Sep 22, 2023

Remove some oversized logs #14054

Remove some oversized logs #14054

Conversation

georgeee commented Sep 7, 2023 • edited

georgeee commented Sep 7, 2023

georgeee commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthiskumar commented Sep 12, 2023

deepthiskumar commented Sep 12, 2023

deepthiskumar commented Sep 12, 2023

deepthiskumar commented Sep 12, 2023

psteckler commented Sep 12, 2023

psteckler commented Sep 13, 2023

georgeee commented Sep 21, 2023

georgeee commented Sep 21, 2023

georgeee commented Sep 21, 2023

deepthiskumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee commented Sep 22, 2023

georgeee commented Sep 22, 2023

georgeee commented Sep 22, 2023

deepthiskumar commented Sep 22, 2023

georgeee commented Sep 7, 2023 •

edited