Log to LSP client #115

bmulvihill · 2018-05-08T00:17:43Z

This PR will change the logging to log to the LSP client instead of to the scry.out tempfile.

I am opening the PR for discussion/ideas. I still have to write specs and such.

Currently the PR does the following

Adds LogMessageParams and MessageType to the protocol
Renames old MessageType to ProtocolMessage
Creates the concept of a Client which can be used to send messages to the LSP client
Allows Logger to be set

faustinoaq · 2018-05-08T01:50:52Z

Thank you, Very useful 👏 👍 🎉

faustinoaq · 2018-05-08T14:53:24Z

This is very nice, although, scry have some very verbose logs like this:

scry/src/scry/completion/dependency_graph.cr

Line 84 in 5e5666c

    
           Log.logger.debug("Finished building the dependancy graph got these nodes:#{graph.each.to_a.map(&.first)}")

So, I suggest to review them and remove the extreme verbosity:

Otherwise the lsp-client/editor would freeze because too many logs 😅

sometimes scry generate MB of debug data:

Sometimes, even GB, see #67 by @bew

bmulvihill · 2018-05-08T17:30:19Z

@faustinoaq Yes I noticed that as well. I can remove some of the more "verbose" logging if that is desirable or we could log that stuff a tempfile like we were doing (i.e. have two loggers, a scry logger and a client logger), but that might get confusing. I am not sure how useful some of these really big logs statements are 🤔

faustinoaq · 2018-05-08T18:54:14Z

@bmulvihill Yeah, that may be confusing. I think we should cleanup logging and just keep client logger.

faustinoaq · 2018-05-09T00:35:13Z

src/scry/workspace.cr

@@ -57,7 +57,6 @@ module Scry

    def get_file(text_document : TextDocumentIdentifier)
      filename = TextDocument.uri_to_filename(text_document.uri)
-      Log.logger.debug("@open_files: #{@open_files}")


Yeah, this one was the most verbose 👍

faustinoaq · 2018-05-13T22:04:16Z

Hey @bmulvihill I already tested this and is very useful and works very nice

@crystal-lang-tools/scry Maybe we can add a log spec on a new PR, WDYT? 😅

bmulvihill · 2018-05-13T23:52:44Z

@faustinoaq sorry I got sidetracked on that file bug last week when working on specs. I’ll try to wrap this up tomorrow

bmulvihill · 2018-05-17T00:19:35Z

@crystal-lang-tools/scry @faustinoaq I removed some of the log calls, and added some specs. I am still not 100% happy w. this, but I am short on time lately. I would appreciate any feedback

bew · 2018-05-17T04:00:50Z

src/scry/log.cr

-      logger = Logger.new(log_file)
-      logger.level = Logger::INFO
-      logger
+      logger = ClientLogger.new(Client.instance.io)
    end

    @@logger : Logger = initialize_logger


No need for a method anymore, you can directly use ClientLogger.new ...

bew · 2018-05-17T04:01:39Z

src/scry/log.cr

-      logger = Logger.new(log_file)
-      logger.level = Logger::INFO
-      logger
+      logger = ClientLogger.new(Client.instance.io)
    end

    @@logger : Logger = initialize_logger

    def self.logger
      @@logger
    end


Note: class_getter logger does the same thing ;)

(or even class_property now that you also have a setter below)

bew · 2018-05-17T04:32:01Z

src/scry/protocol/message_type.cr

@@ -0,0 +1,8 @@
+module Scry


Should be Scry::Protocol ?

I just realized that (all?) the LSP protocol is not in a Protocol module, as I think it should, to properly see on usage when you're using a protocol type or not (that's important!).

This can be done later in another PR though!

Yeah, I'm already working on that 👍

bew · 2018-05-17T04:32:56Z

src/scry/client.cr

+  class Client
+    alias ClientMessage = Initialize | ResponseMessage | NotificationMessage | Nil
+
+    def self.instance(io : IO = STDOUT)


I think this is a bit tricky if you want to use your own IO (i.e: in specs), you need to make sure that the first call to Client.instance has an IO, and you can't change the IO later once it's set.

I think it would be better to keep the Client.instance as a simple getter, and change the architecture: maybe Client doesn't even need to be a singleton?

bew · 2018-05-17T05:07:38Z

As I said in one of the comment, I think Client should not be a singleton.

Here is a way to do it:

Store the client in ClientLogger, and use it in ClientLogger#write to send the log message.
Configure the logger in Scry.start, not as a default in log.cr. You'll also have to instantiate a Client with its IO before the logger init and pass that client to ClientLogger constructor.
And finally, remove the default io param to Client constructor, to force you to be explicit!

This way you never have to use a Client.instance method, you always have the current Client object to send things. And additionally, you can configure things easily, and repeatably (for specs).

Do you see what I mean?

bmulvihill · 2018-05-17T12:36:43Z

I am also wondering if there is any use for the Log class anymore. I just left it in to maintain the interface, but it might not have much value anymore.

bew · 2018-05-17T15:59:21Z

src/scry/log.cr

+        @closed = false
+        @mutex = Mutex.new
+        @io = @client.io
+      end


Did you copy this constructor from Logger?
I would suggest to keep it simple, and use super to call the Logger's normal constructor.
E.g:

def initialize(@client : Client, level = Severity::INFO) super(@client.io, level) end

I don't think we need the rest for now.

But as you like :)

Ahh yes, good call ;)

bew · 2018-05-17T16:02:24Z

Hmm yeah Log is not useful as a class anymore, but we can keep it around as a module? So that you still have to do Log.logger.debug(...)

faustinoaq · 2018-05-18T03:09:56Z

@bmulvihill Hi, I just tried latest changes and this feature still works fine 🎉

Although, I notices a new null on my logs on every request, Why? 😅

BTW, Do you know what happened with the initializer capabilities log we used to have? 😅

Edit:

With "initializer capabilities log" I mean the initializer message:

{ "jsonrpc": "2.0", "id": 1, "method": "initialize", "params": { "processId": 1, "rootPath": "<root-path-here>", "capabilities": {} , "trace": "off"}}

I think we used to log this as well.

Currently I can't see it on scry log 😅

bmulvihill · 2018-05-23T17:20:29Z

@crystal-lang-tools/scry Made some adjustments, null should no longer be logged.

bew

Looks good 😃

@bmulvihill note for later: next time can you make individual commits for new changes? For example you removed the null, by not making a commit for that change only, I have no idea what you did to remove it.

faustinoaq

Works like a charm! 👍

bmulvihill requested a review from faustinoaq May 8, 2018 00:18

bmulvihill force-pushed the log-message branch from 976754d to 9ccd230 Compare May 8, 2018 00:20

faustinoaq requested a review from a team May 8, 2018 01:41

faustinoaq added enhancement review required labels May 8, 2018

faustinoaq added the wip label May 8, 2018

faustinoaq reviewed May 9, 2018

View reviewed changes

bmulvihill force-pushed the log-message branch 2 times, most recently from c56878f to 89c4d6b Compare May 17, 2018 00:17

bmulvihill changed the title ~~[WIP] Log to LSP client~~ Log to LSP client May 17, 2018

bew suggested changes May 17, 2018

View reviewed changes

bew reviewed May 17, 2018

View reviewed changes

bmulvihill force-pushed the log-message branch from eeb793e to c7b21b5 Compare May 17, 2018 19:43

Log to client

3afc1f5

bmulvihill force-pushed the log-message branch from c7b21b5 to 3afc1f5 Compare May 23, 2018 00:08

faustinoaq self-requested a review May 23, 2018 19:39

faustinoaq requested review from laginha87, bew and a team May 23, 2018 19:39

bew approved these changes May 24, 2018

View reviewed changes

faustinoaq approved these changes May 24, 2018

View reviewed changes

faustinoaq removed the wip label May 24, 2018

faustinoaq merged commit bf57569 into master May 24, 2018

faustinoaq deleted the log-message branch May 24, 2018 11:15

faustinoaq removed the review required label Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log to LSP client #115

Log to LSP client #115

bmulvihill commented May 8, 2018

faustinoaq commented May 8, 2018

faustinoaq commented May 8, 2018

bmulvihill commented May 8, 2018

faustinoaq commented May 8, 2018

faustinoaq May 9, 2018

faustinoaq commented May 13, 2018

bmulvihill commented May 13, 2018

bmulvihill commented May 17, 2018

bew May 17, 2018

bew May 17, 2018

bew May 17, 2018

faustinoaq May 17, 2018

bew May 17, 2018

bew commented May 17, 2018

bmulvihill commented May 17, 2018

bew May 17, 2018

bmulvihill May 17, 2018

bew commented May 17, 2018

faustinoaq commented May 18, 2018 •

edited

Loading

bmulvihill commented May 23, 2018

bew left a comment •

edited

Loading

faustinoaq left a comment

Log to LSP client #115

Log to LSP client #115

Conversation

bmulvihill commented May 8, 2018

faustinoaq commented May 8, 2018

faustinoaq commented May 8, 2018

bmulvihill commented May 8, 2018

faustinoaq commented May 8, 2018

Choose a reason for hiding this comment

faustinoaq commented May 13, 2018

bmulvihill commented May 13, 2018

bmulvihill commented May 17, 2018

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

bew commented May 17, 2018

bmulvihill commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew commented May 17, 2018

faustinoaq commented May 18, 2018 • edited Loading

bmulvihill commented May 23, 2018

bew left a comment • edited Loading

Choose a reason for hiding this comment

faustinoaq left a comment

Choose a reason for hiding this comment

faustinoaq commented May 18, 2018 •

edited

Loading

bew left a comment •

edited

Loading