Improve logs in daemon #352

krhubert · 2018-08-08T19:21:49Z

PR for discussion about dameon logs.

Below are logs in text and json formats.

$ ./core
INFO[0000] Starting MESG Core vX.X.X                    
INFO[0000] Server listens on /mesg/server.sock          
INFO[0000] Server listens on [::]:50052                 
INFO[0002] finished unary call with code OK              grpc.code=OK grpc.method=ExecuteTask grpc.service=api.Core grpc.start_time="2018-08-08T21:15:30+02:00" grpc.time_ms=26.072 span.kind=server system=grpc

{"level":"info","msg":"Starting MESG Core vX.X.X","time":"2018-08-08T21:16:37+02:00"}
{"level":"info","msg":"Server listens on [::]:50052","time":"2018-08-08T21:16:37+02:00"}
{"level":"info","msg":"Server listens on /mesg/server.sock","time":"2018-08-08T21:16:37+02:00"}
{"grpc.code":"OK","grpc.method":"ExecuteTask","grpc.service":"api.Core","grpc.start_time":"2018-08-08T21:16:43+02:00","grpc.time_ms":27.99,"level":"info","msg":"finished unary call with code OK","span.kind":"server","system":"grpc","time":"2018-08-08T21:16:43+02:00"}

@mesg-foundation/core Waiting for your feedback.

antho1404 · 2018-08-09T04:28:40Z

api/server.go

-	s.instance = grpc.NewServer()
+	s.instance = grpc.NewServer(
+		grpc.StreamInterceptor(grpc_middleware.ChainStreamServer(
+			grpc_logrus.StreamServerInterceptor(log.NewEntry(log.StandardLogger())),


I was thinking this would log all the data in the stream but I cannot see any log from that. I wonder what this is for ? Maybe the StreamClientInterceptor is the one to use.
And should this log all events from the stream or just the connection to the stream ?

antho1404 · 2018-08-09T05:23:40Z

execution/complete.go

@@ -32,9 +31,6 @@ func (execution *Execution) Complete(output string, data map[string]interface{})
 	execution.Output = output
 	execution.OutputData = data



I guess you removed the logs here because there is not reason to have them with the rpc middleware

antho1404 · 2018-08-09T05:27:05Z

My logs doesn't look like your

MESG Core is running
time="2018-08-09T05:24:51Z" level=info msg="Starting MESG Core dev local"
time="2018-08-09T05:24:51Z" level=info msg="Server listens on [::]:50052"
time="2018-08-09T05:24:51Z" level=info msg="Server listens on /mesg/server.sock"

Probably because I'm using the ./dev-core that is running the core inside docker and then reading the logs from docker. The logs like that are not really readable

antho1404 · 2018-08-09T10:25:49Z

Everything is good, I just want to fix few things before merge:

update the go dependencies in the dockerfile go get github.com/sirupsen/logrus && go get github.com/grpc-ecosystem/go-grpc-middleware
update the changelog

Also I like the colored output that is only available with TTY so maybe we should support another flag that might be the default with "colored" or something like that that would put core in a TTY mode and have the colored outputs but this is definitely not required for this PR

krhubert · 2018-08-10T08:39:28Z

@antho1404 done, to merge.

antho1404 · 2018-08-11T08:36:24Z

I'm ok with this, I'm just thinking should we add the colors on this PR or another one ? the fix is quite easy logrus.SetFormatter(&logrus.TextFormatter{ForceColors: true})

I really think that a nice readable log for human is important but having something in plain text or json is great like that it gives a good flexibility.

Any feedbacks ? @mesg-foundation/core

krhubert · 2018-08-11T13:17:36Z

I'm ok to add some flag to have colors with logs. But forcing color is a bad idea.

antho1404 · 2018-08-12T06:55:46Z

Sorry I wasn't clear enough in my proposition of the fix, I don't want to replace the text version with the colored one, the text version is really good and a must have, i was talking about another mode like described in my previous comment with "colored".

We would have the 3 following modes:

colored (default)
text
json

I would prefer colored logs by default because it's more user friendly and I think developers who have the need to extract logs will be more comfortable playing with the terminal and using flags etc... and developer who are less comfortable they would prefer something more "human readable" but I would love to have other feedbacks on that

NicolasMahe

Please remove support for colored.

Also the mesg-core start should accept flag to configuration the LogFormat and LogLevel of the Core.

antho1404

It is not possible to configure the logs when starting the core with the CLI

MESG_LOG_FORMAT=json ./dev-cli start
./dev-cli logs

That is why a flag for the start command is needed. This flag will send the right env variable to the daemon inside docker

antho1404 · 2018-08-16T00:32:12Z

Ok for me except the PR for dependencies is merged so we have to update this branch and add these dependencies in the vendor

github.com/sirupsen/logrus
github.com/grpc-ecosystem/go-grpc-middleware

NicolasMahe

Looks good!

@krhubert Please merge dev and add the new packages to vendor.

NicolasMahe · 2018-08-16T08:49:27Z

cmd/start.go

@@ -18,7 +21,41 @@ var Start = &cobra.Command{
 	DisableAutoGenTag: true,
 }

+// logFormatValue reprenets log format flag value.


Typo on "reprenets"

NicolasMahe · 2018-08-16T08:49:34Z

cmd/start.go

+func (v *logFormatValue) Type() string   { return "string" }
+func (v *logFormatValue) String() string { return string(*v) }
+
+// logLevelValue reprenets log level flag value.


Typo on "reprenets"

krhubert · 2018-08-16T14:03:29Z

@NicolasMahe done

krhubert assigned krhubert and unassigned krhubert Aug 8, 2018

krhubert force-pushed the daemon-logs branch 2 times, most recently from 6c3972b to 1cabcd2 Compare August 8, 2018 19:29

antho1404 reviewed Aug 9, 2018

View reviewed changes

krhubert force-pushed the daemon-logs branch from 1cabcd2 to 5f369c7 Compare August 9, 2018 17:06

NicolasMahe requested changes Aug 13, 2018

View reviewed changes

antho1404 requested changes Aug 13, 2018

View reviewed changes

krhubert force-pushed the daemon-logs branch from 7a063d2 to 3228394 Compare August 15, 2018 10:58

krhubert added 8 commits August 16, 2018 10:02

Make logger configurable

ea5d772

Create function to configure global logger

9f47d39

Replace standard log with logrus

63a6cd7

Use grpc middleware for logging rpc calls

6b89e50

Remove trailing spaces from proto files

434217a

Use logrus pkg prefix

300473d

Update CHANGELOG

68dad6f

Add --log-format and --log-level flags to start cmd

008efdd

NicolasMahe requested changes Aug 16, 2018

View reviewed changes

krhubert added 2 commits August 16, 2018 11:02

Fix typo

53a06e4

Vendor

8e7095c

krhubert force-pushed the daemon-logs branch from 38fa7c1 to 8e7095c Compare August 16, 2018 09:44

antho1404 approved these changes Aug 16, 2018

View reviewed changes

NicolasMahe approved these changes Aug 17, 2018

View reviewed changes

NicolasMahe merged commit 2168689 into dev Aug 17, 2018

NicolasMahe deleted the daemon-logs branch August 17, 2018 04:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve logs in daemon #352

Improve logs in daemon #352

krhubert commented Aug 8, 2018

antho1404 Aug 9, 2018

antho1404 Aug 9, 2018

krhubert Aug 9, 2018

antho1404 commented Aug 9, 2018

antho1404 commented Aug 9, 2018 •

edited by krhubert

Loading

krhubert commented Aug 10, 2018

antho1404 commented Aug 11, 2018

krhubert commented Aug 11, 2018

antho1404 commented Aug 12, 2018

NicolasMahe left a comment

antho1404 left a comment

antho1404 commented Aug 16, 2018 •

edited

Loading

NicolasMahe left a comment •

edited

Loading

NicolasMahe Aug 16, 2018

NicolasMahe Aug 16, 2018

krhubert commented Aug 16, 2018

		@@ -32,9 +31,6 @@ func (execution *Execution) Complete(output string, data map[string]interface{})
		execution.Output = output
		execution.OutputData = data

Improve logs in daemon #352

Improve logs in daemon #352

Conversation

krhubert commented Aug 8, 2018

antho1404 Aug 9, 2018

Choose a reason for hiding this comment

antho1404 Aug 9, 2018

Choose a reason for hiding this comment

krhubert Aug 9, 2018

Choose a reason for hiding this comment

antho1404 commented Aug 9, 2018

antho1404 commented Aug 9, 2018 • edited by krhubert Loading

krhubert commented Aug 10, 2018

antho1404 commented Aug 11, 2018

krhubert commented Aug 11, 2018

antho1404 commented Aug 12, 2018

NicolasMahe left a comment

Choose a reason for hiding this comment

antho1404 left a comment

Choose a reason for hiding this comment

antho1404 commented Aug 16, 2018 • edited Loading

NicolasMahe left a comment • edited Loading

Choose a reason for hiding this comment

NicolasMahe Aug 16, 2018

Choose a reason for hiding this comment

NicolasMahe Aug 16, 2018

Choose a reason for hiding this comment

krhubert commented Aug 16, 2018

antho1404 commented Aug 9, 2018 •

edited by krhubert

Loading

antho1404 commented Aug 16, 2018 •

edited

Loading

NicolasMahe left a comment •

edited

Loading