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

Jump to bottom

feat(logging): easier logging #940

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat(logging): easier logging #940

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 44 additions & 0 deletions ocaml-lsp-server/src/ocaml_lsp_server.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,48 @@ let on_notification server (notification : Client_notification.t) :
in
state

module Log_config = struct
let parse_log_env_var var_name =
match String.split ~on:',' (Sys.getenv var_name) with
| value :: sections -> (Some value, sections)
| [] -> (None, [])
| exception Not_found -> (None, [])

(* note: merlin logs are buffered, so one has to wait until they see the
output; we could flush the buffer after each merlin command dispatch? *)
let with_merlin_log_file f =
let log_file, sections = parse_log_env_var "MERLIN_LOG" in
Copy link
Member

Choose a reason for hiding this comment

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

Is there no way to have merlin write the ocamllsp log? It seems annoying to have to check two different files for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there no way to have merlin write the ocamllsp log?

Not without making changes to merlin, but, currently, we can pass - to both LOG variables to redirect logs to stderr; then we can have them in one place.

From my experience, I found having separate log files quite convenient because of merlin log buffering and how much logs it can produce. We can evolve our logging as the need rises.

--

Also, merlin's buffering is a bit of annoyance for debugging; it would be great to possible flush the buffer after each Document.dispatch call. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

If you don't want merlin's log, can't you just disable it? Or add a marker that distinguishes the two types of logs and then filter in your log viewer? One of the most useful things about logs is the linearized output. Seeing it in a different file is going to reduce the ability to debug anything.

Also, merlin's buffering is a bit of annoyance for debugging; it would be great to possible flush the buffer after each Document.dispatch call. Wdyt?

Sure, that sounds reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But the issue with merlin not exposing its logging internals remains, so we can’t have merlin and olsp use the same file for logging except for stderr, which allows to have a single log. I can send a patch to merlin to fix this, but doesn’t have to stop this PR; shall a user like merlin and olsp to use the same file, they can pass the same file path as both env vars. Does this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to me yet. How would you prevent from the logs from overwriting each other?

Merlin already logs to stderr if the value is set to -. So might as well only stick to that for now. To be clear, here are the properties of a logging we're looking for:

  1. A single environment variable to tweak the various settings.
  2. Messages are written atomically. That is, they should never appear interleaved.
  3. However, messages are still written to a linear log so we can easily see temporal relationships.
  4. We can filter the logs by the "source" or other various useful properties.

I'd like to move forward towards these properties, not away from them.

Merlin_utils.Logger.with_log_file log_file ~sections f

let with_ocamlsp_log_file f =
let log_file, sections = parse_log_env_var "OCAMLLSP_LOG" in
let cleanup =
match log_file with
| None | Some "" -> Fun.id
| Some "-" ->
Log.out := Format.err_formatter;
Fun.id
| Some s ->
let ch = open_out s in
Log.out := Format.formatter_of_out_channel ch;
fun () -> close_out ch
in
let sections =
List.fold_left sections ~f:String.Set.add ~init:String.Set.empty
in
(Log.level :=
function
| None -> true
| Some section -> String.Set.mem sections section);
match f () with
| v ->
cleanup ();
v
| exception e ->
cleanup ();
raise e
end

let start () =
let detached = Fiber.Pool.create () in
let server = Fdecl.create Dyn.opaque in
Expand Down Expand Up @@ -1110,4 +1152,6 @@ let start () =
let run ~read_dot_merlin () =
Merlin_config.should_read_dot_merlin := read_dot_merlin;
Unix.putenv "__MERLIN_MASTER_PID" (string_of_int (Unix.getpid ()));
Log_config.with_ocamlsp_log_file @@ fun () ->
Log_config.with_merlin_log_file @@ fun () ->
Lev_fiber.run ~sigpipe:`Ignore start |> Lev_fiber.Error.ok_exn