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.

Sign up for GitHub

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

Add ?fail_on_error argument to Lwt_log_core.load_rules, also expose new level_of_string function in the interface #306

Merged
merged 3 commits into from
Dec 23, 2016
Merged

Add ?fail_on_error argument to Lwt_log_core.load_rules, also expose new level_of_string function in the interface #306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6b3e80
Add ?fail_on_error argument to Lwt_log_core.load_rules
dmbaturin Dec 23, 2016
b91b2b2
Add a docstring for load_rules
dmbaturin Dec 23, 2016
0679de3
Fix a typom flush code lines to the left.
dmbaturin Dec 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions src/logger/lwt_log_core.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ let string_of_level = function
| Error -> "error"
| Fatal -> "fatal"

let level_of_string str =
let str = (String.lowercase [@ocaml.warning "-3"]) str in
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right to worry about this, but we can't use lowercase_ascii because Lwt still supports 4.02.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, good I kept it intact then.

match str with
| "debug" -> Ok Debug
| "info" -> Ok Info
| "notice" -> Ok Notice
| "warning" -> Ok Warning
| "error" -> Ok Error
| "fatal" -> Ok Fatal
| _ -> Pervasives.Error (Printf.sprintf "invalid log level (%s)" str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say for this function, to have it evaluate to a level option. The Error result doesn't carry more information than None – there is only one way to fail, and the caller already knows which string they passed in. But to address your question, yes, we should use Result.result instead of Pervasives.result, in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, right now, shall we use option or introduce a dependency on the result compatibility library? If 4.02 support is a goal, option is the easiest way out, and I agree Error doesn't offer much more than None here, but I'll do whichever you tell me to do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer option here :) But we already have the dependency on result (see lwt.opam).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I missed it. Ok, I'll go with option then.


(* +-----------------------------------------------------------------+
| Patterns and rules |
+-----------------------------------------------------------------+ *)
Expand Down Expand Up @@ -101,31 +112,30 @@ let split pattern =
in
loop 0


let rules = ref []

let load_rules' str =
let load_rules' str fail_on_error =
let rec loop = function
| [] ->
[]
| (pattern, level) :: rest ->
let pattern = split pattern in
match (String.lowercase [@ocaml.warning "-3"]) level with
| "debug" -> (pattern, Debug) :: loop rest
| "info" -> (pattern, Info) :: loop rest
| "notice" -> (pattern, Notice) :: loop rest
| "warning" -> (pattern, Warning) :: loop rest
| "error" -> (pattern, Error) :: loop rest
| "fatal" -> (pattern, Fatal) :: loop rest
| level -> log_intern "invalid log level (%s)" level; loop rest
| [] -> []
| (pattern, level_str) :: rest ->
let pattern = split pattern in
let level = level_of_string level_str in
match level with
| Ok level -> (pattern, level) :: loop rest
| Pervasives.Error msg ->
if fail_on_error then raise (Failure msg)
else log_intern "invalid log level (%s)" level_str; loop rest
in
match Lwt_log_rules.rules (Lexing.from_string str) with
| None -> Printf.eprintf "Invalid contents of the LWT_LOG variable\n%!"
| Some l -> rules := loop l
| None ->
if fail_on_error then raise (Failure "Invalid log rules")
else Printf.eprintf "Invalid contents of the LWT_LOG variable\n%!"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're fixing this up anyway, I think the reference to LWT_LOG should be removed – this code can be called when not reading LWT_LOG. Alternatively, one could pass some kind of from_lwt_log parameter to control what this message looks like... but it may be kind of unnecessary. Ideally we would push the generation of the message to the caller, but that would require a breaking change :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just trying to keep the old behaviour completely intact, reference to LWT_LOG does bother me too of course. Shall we just replace the message with "Invalid contents of log rules" or similar?

Copy link
Collaborator

@aantron aantron Dec 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Up to you. Using the same message for both the exception and the log message is fine. I hope nobody was parsing their log for the string LWT_LOG :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my thought exactly... ;)

| Some l -> rules := loop l


let _ =
match try Some(Sys.getenv "LWT_LOG") with Not_found -> None with
| Some str -> load_rules' str
| Some str -> load_rules' str false
| None -> ()

(* +-----------------------------------------------------------------+
Expand Down Expand Up @@ -197,8 +207,8 @@ end

type section = Section.t

let load_rules str =
load_rules' str;
let load_rules ?(fail_on_error=false) str =
load_rules' str fail_on_error;
Section.recompute_levels ()

let add_rule pattern level =
Expand Down
4 changes: 3 additions & 1 deletion src/logger/lwt_log_core.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ type section

val string_of_level : level -> string

val load_rules : string -> unit
val level_of_string : string -> (level, string) result

val load_rules : ?fail_on_error:bool -> string -> unit
Copy link
Collaborator

@aantron aantron Dec 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Should probably document ?fail_on_error as, besides the exception, there is the non-trivial difference that load_rules ~fail_on_error:false will actually load as many rules as it can, while load_rules ~fail_on_error:true will load nothing if any error is encountered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I'll add a docstring.

(** Reset the rules set when parsing the [LWT_LOG] environment variable using this
string. *)

Expand Down