Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mirage_runtime: restore converters for log_threshold and allocation_policy #1482

Merged
merged 1 commit into from Nov 10, 2023

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Nov 10, 2023

Previously, they were part of the Arg submodule. Now they're in their own Conv module. The reason is that mirage-monitoring uses this value for dynamically adjusting the log level.

…olicy

Previously, they were part of the Arg submodule. Now they're in their own Conv
module. The reason is that mirage-monitoring uses this value for dynamically
adjusting the log level.
@hannesm
Copy link
Member Author

hannesm commented Nov 10, 2023

I'm open for suggestions, the code in place (mirage-monitoring) is:

let adjust_log_level s = (* s being a string such as "tcp:error,whateer:debug,something:quiet,*:warning" *)
  let ts =
    List.map
      (fun s -> (fst Mirage_runtime.Arg.log_threshold) s) (* with this PR, it'll be Mirage_runtime.Conv.log_threshold *)
      (String.split_on_char ',' s)
  in
  let* oks =
    List.fold_left (fun acc t ->
        let* acc = acc in
        match t with
        | `Ok l -> Ok (l :: acc)
        | `Error msg -> Error msg)
      (Ok []) ts
  in
  Mirage_runtime.set_level ~default:(Logs.level ()) oks;
  Ok `Empty

Copy link
Member

@reynir reynir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's useful to expose allocation_policy. As far as I can see at runtime it is not useful - it is only used in code generation:

mirage/lib/mirage/mirage.ml

Lines 420 to 426 in 4939ca6

let pp_pol ~name =
Fmt.(
any name
++ any " = "
++ any "(match "
++ Mirage_impl_misc.pp_key
++ any " with `Next_fit -> 0 | `First_fit -> 1 | `Best_fit -> 2)")

It would however be very nice to have the log_threshold converter back!

@hannesm
Copy link
Member Author

hannesm commented Nov 10, 2023

ok, let's merge this, and figure later out if we would like to remove the allocation_policy converter. (there's as well the possibility to use mirage-logs.cli and cmdliner-stdlib -- but I'm afraid they're changing the default values and introduce more dependency complexity, so let's wait at least another release with it).

@hannesm hannesm merged commit f5c2b39 into mirage:main Nov 10, 2023
3 checks passed
@hannesm hannesm deleted the restore-runtime-conv branch November 10, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants