removing compile time config load logging from cli #16403

svv232 · 2024-12-05T19:55:47Z

PR does a number of things:

Removes ~logger parameter from load_constants, while adding a new load_constants_with_logging function for the cases where logging is actually required
- This ensures that all of the direct usages of load_constants appear in the diff and decision on whether to use the logging is made explicitly on a case-by-case basis within this PR
Removes ?logger parameter from load_compile_config
- Logging was never provided for the call of load_compile_config explicitly
- The load_compile_config function was introduced in [Part 1 of 2] Constants loader and Config loader #16186 (or predecessors) after adding an optional --config-file parameter to commands in client.ml, use of Logger.create () as a default logger was a mistake, which was already fixed in fix compile config logging #16381
Replaces Logger.create () with Logger.null () in a single occasion of load_config_files being used directly for a CLI command that is expected to produce a short output with a result (mina advanced compile-time-constants)

svv232 · 2024-12-05T19:55:57Z

!ci-build-me

mrmr1993 · 2024-12-05T21:02:44Z

src/lib/runtime_config/runtime_config.ml

@@ -1802,14 +1791,8 @@ module Json_loader : Json_loader_intf = struct
        | Ok loaded_config ->
            combine config loaded_config
        | Error err ->
-            [%log fatal]


This means that we won't see the failure reason in the node logs, and so we'll have a harder time root-causing crashes if node operators hit this :(

for this specific log, won't node operators just see the error in the exception back trace?

It depends on how they're running the node, and whether they're collecting error messages instead of logs. Most only have logs.

svv232 · 2024-12-05T21:21:08Z

!ci-build-me

svv232 · 2024-12-05T21:48:07Z

!ci-build-me

svv232 · 2024-12-05T22:00:46Z

!ci-build-me

svv232 · 2024-12-05T22:42:40Z

!ci-nightly-me

coveralls · 2024-12-06T01:32:37Z

coverage: 60.682%. first build
when pulling 5aa527c on sai/removing-logging-from-cfg-loading
into b6f5f57 on compatible.

src/lib/runtime_config/runtime_config.ml

svv232 · 2024-12-11T20:53:51Z

!ci-build-me

svv232 · 2024-12-11T20:59:45Z

!ci-nightly-me

svv232 · 2024-12-12T05:42:00Z

!ci-build-me

georgeee

I think we need two explicit versions of load_constants: with and without the logging.

In many contexts where logging was removed now it's actually useful to have, and violates no "contract" with users

georgeee · 2024-12-12T09:23:24Z

src/lib/snark_worker/standalone/run_snark_worker.ml

@@ -22,10 +22,8 @@ let command =
       let open Async in
       let open Deferred.Let_syntax in
       let%bind constraint_constants, proof_level =
-         let logger = Logger.create () in


Doesn't look right to me

georgeee · 2024-12-12T12:14:22Z

src/app/batch_txn_tool/batch_txn_tool.ml

-         let%map conf =
-           Runtime_config.Constants.load_constants ~logger config_file
-         in
+         let%map conf = Runtime_config.Constants.load_constants config_file in


Doesn't look right to me

georgeee · 2024-12-12T12:15:09Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -1567,7 +1567,7 @@ let internal_commands ~itn_features logger =
          let open Deferred.Let_syntax in
          let%bind constraint_constants, proof_level, compile_config =
            let%map conf =
-              Runtime_config.Constants.load_constants ~logger config_file
+              Runtime_config.Constants.load_constants config_file


Doesn't look right to me

georgeee · 2024-12-12T12:15:19Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -1599,7 +1599,7 @@ let internal_commands ~itn_features logger =
          let open Deferred.Let_syntax in
          let%bind constraint_constants, proof_level, compile_config =
            let%map conf =
-              Runtime_config.Constants.load_constants ~logger config_file
+              Runtime_config.Constants.load_constants config_file


Doesn't look right to me

georgeee · 2024-12-12T12:15:23Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -1656,7 +1656,7 @@ let internal_commands ~itn_features logger =
          let open Async in
          let%bind constraint_constants, proof_level, compile_config =
            let%map conf =
-              Runtime_config.Constants.load_constants ~logger config_file
+              Runtime_config.Constants.load_constants config_file


Doesn't look right to me

georgeee · 2024-12-12T12:27:25Z

src/app/cli/src/init/client.ml

-       let%map conf =
-         Runtime_config.Constants.load_constants ~logger config_files
-       in
+       let%map conf = Runtime_config.Constants.load_constants config_files in


Doesn't look right to me

georgeee · 2024-12-12T12:28:39Z

src/app/delegation_verify/delegation_verify.ml

-  let%map constants =
-    Runtime_config.Constants.load_constants ~logger config_file
-  in
+  let%map constants = Runtime_config.Constants.load_constants config_file in


Doesn't look right to me

Changes to this whole file should be reverted, I think

src/lib/genesis_ledger_helper/genesis_ledger_helper.ml

georgeee · 2024-12-12T12:32:18Z

src/lib/runtime_config/runtime_config.ml

@@ -2028,13 +2028,14 @@ module Constants : Constants_intf = struct

  (* Use this function if you don't need/want the ledger configuration *)
  let load_constants ?conf_dir ?commit_id_short ?itn_features ?cli_proof_level
-      ~logger config_files =
+      config_files =


I suggest that we rename this function to load_constants_with_logging and have an explicit ~logger parameter: let load_constants = load_constants_with_logging ~logger:(Logger.null ())

georgeee · 2024-12-12T12:32:38Z

src/lib/snark_worker/functor.ml

@@ -363,7 +363,7 @@ module Make (Inputs : Intf.Inputs_intf) :
        let%bind.Deferred constraint_constants, proof_level, compile_config =
          let%map.Deferred config =
            Runtime_config.Constants.load_constants ?conf_dir ?cli_proof_level
-              ~logger config_file
+              config_file


Doesn't look right to me

…snark functor and apps

svv232 · 2024-12-13T16:05:53Z

!ci-build-me

georgeee

This PR fully addresses the issue of logging originated in load_config_files appearing in unexpected contexts.

Usages of load_config_files can be classified as follows:

Direct usages in long-running processes (mina, archive), tests and genesis_ledger_helper
Direct usage in a CLI command
Usages in load_constants and load_compile_config

First category functions well, so no need for a change. Latter two categories are fully addressed on a case-by-case basis.

P.S. left a thread of comments while performing this review, for bookkeeping.

georgeee · 2025-01-09T09:38:05Z

src/lib/runtime_config/runtime_config.ml

@@ -1777,6 +1777,7 @@ module Json_loader : Json_loader_intf = struct
      in
      [%log info] "Reading configuration files $config_files"
        ~metadata:[ ("config_files", `List config_files_paths) ] ;
+


Suggested change

georgeee · 2025-01-09T09:42:24Z

src/app/cli/src/init/client.ml

@@ -43,8 +43,8 @@ let or_error_str ~f_ok ~error = function
  | Error e ->
      sprintf "%s\n%s\n" error (Error.to_string_hum e)

-let load_compile_config ?(logger = Logger.null ()) config_files =
-  let%map conf = Runtime_config.Constants.load_constants ~logger config_files in
+let load_compile_config config_files =


Comment from review:

For usages of load_compile_config with no ~logger provided, things remain the same

For usages of load_compile_config that were supplying ~logger, compiler would mark all of the occasions and these would have appeared in the PR's diff (none appears, hence there were no such usages)

georgeee · 2025-01-09T09:45:32Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -1567,7 +1567,8 @@ let internal_commands ~itn_features logger =
          let open Deferred.Let_syntax in
          let%bind constraint_constants, proof_level, compile_config =
            let%map conf =
-              Runtime_config.Constants.load_constants ~logger config_file
+              Runtime_config.Constants.load_constants_with_logging ~logger


No-op change

Confirm that we want logging on for this component (assuming internal commands aren't used in automated scripts)

georgeee · 2025-01-09T09:45:38Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -1599,7 +1600,8 @@ let internal_commands ~itn_features logger =
          let open Deferred.Let_syntax in
          let%bind constraint_constants, proof_level, compile_config =
            let%map conf =
-              Runtime_config.Constants.load_constants ~logger config_file
+              Runtime_config.Constants.load_constants_with_logging ~logger


No-op change

Confirm that we want logging on for this component (assuming internal commands aren't used in automated scripts)

georgeee · 2025-01-09T09:45:43Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -1656,7 +1658,8 @@ let internal_commands ~itn_features logger =
          let open Async in
          let%bind constraint_constants, proof_level, compile_config =
            let%map conf =
-              Runtime_config.Constants.load_constants ~logger config_file
+              Runtime_config.Constants.load_constants_with_logging ~logger


No-op change

Confirm that we want logging on for this component (assuming internal commands aren't used in automated scripts)

georgeee · 2025-01-09T10:34:01Z

src/app/cli/src/init/client.ml

-         let%map conf =
-           Runtime_config.Constants.load_constants ~logger config_files
-         in
+         let%map conf = Runtime_config.Constants.load_constants config_files in


Confirm that we don't want logging on for this component (CLI utility)

georgeee · 2025-01-09T10:34:10Z

src/app/cli/src/init/client.ml

-         let%map conf =
-           Runtime_config.Constants.load_constants ~logger config_files
-         in
+         let%map conf = Runtime_config.Constants.load_constants config_files in


Confirm that we don't want logging on for this component (CLI utility)

georgeee · 2025-01-09T10:34:52Z

src/app/cli/src/init/client.ml

@@ -1870,7 +1864,7 @@ let compile_time_constants =
       let%map ({ consensus_constants; _ } as precomputed_values), _ =
         (* This is kind of ugly because we are allowing for supplying a runtime_config value directly, rather than force what is read from the environment *)
         (* TODO: See if we can initialize consensus_constants without also initializing the ledger *)
-         let logger = Logger.create () in
+         let logger = Logger.null () in


Confirm that we don't want logging on for this component (CLI utility)

georgeee · 2025-01-09T10:35:15Z

src/app/cli/src/init/client.ml

-          let%map conf =
-            Runtime_config.Constants.load_constants ~logger config_files
-          in
+          let%map conf = Runtime_config.Constants.load_constants config_files in


Confirm that we don't want logging on for this component (CLI utility)

georgeee · 2025-01-09T10:35:48Z

src/app/cli/src/init/client.ml

       let%map conf =
-         Runtime_config.Constants.load_constants ~logger config_files
+         Runtime_config.Constants.load_constants_with_logging


Confirm that we want logging for this test utility

mrmr1993

Crypto changes LGTM

removing compile time config load logging from cli

e824170

svv232 requested a review from a team as a code owner December 5, 2024 19:55

dkijania mentioned this pull request Dec 5, 2024

add e2e tests for mina cli (unexpected log output) #16402

Merged

mrmr1993 reviewed Dec 5, 2024

View reviewed changes

fixing loading in all of the apps to fix CI

63a8ce7

fixing unit test builds

8ea10e5

svv232 requested a review from mrmr1993 December 5, 2024 21:54

remove logger and fixing build for mina lib

5aa527c

svv232 requested a review from a team as a code owner December 5, 2024 22:00

georgeee requested changes Dec 9, 2024

View reviewed changes

src/lib/runtime_config/runtime_config.ml Show resolved Hide resolved

only propogate a logger when one already exists

0880b79

svv232 requested a review from georgeee December 11, 2024 20:25

svv232 added 2 commits December 11, 2024 15:28

remove unecessary logger invocation

2d22bc1

adding logger back to apps while loading json config

85634a1

format the client

2efdc28

georgeee requested changes Dec 12, 2024

View reviewed changes

add load_constants_with_logging definition and use the definition in …

2f8fa03

…snark functor and apps

svv232 requested a review from georgeee December 17, 2024 06:53

georgeee approved these changes Jan 9, 2025

View reviewed changes

georgeee mentioned this pull request Jan 9, 2025

loading compile time constants with null in the cli #16396

Closed

mrmr1993 approved these changes Jan 9, 2025

View reviewed changes

georgeee merged commit c3c7c34 into compatible Jan 9, 2025
57 checks passed

georgeee deleted the sai/removing-logging-from-cfg-loading branch January 9, 2025 12:23

removing compile time config load logging from cli #16403

removing compile time config load logging from cli #16403

Conversation

svv232 commented Dec 5, 2024 • edited by georgeee Loading

svv232 commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 commented Dec 5, 2024

svv232 commented Dec 5, 2024

svv232 commented Dec 5, 2024

svv232 commented Dec 5, 2024

coveralls commented Dec 6, 2024

svv232 commented Dec 11, 2024

svv232 commented Dec 11, 2024

svv232 commented Dec 12, 2024

georgeee 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 commented Dec 13, 2024

georgeee 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

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

mrmr1993 left a comment

Choose a reason for hiding this comment

svv232 commented Dec 5, 2024 •

edited by georgeee

Loading