Improves logging message for failed analysis by se7entyse7en · Pull Request #442 · src-d/lookout

se7entyse7en · 2018-12-31T14:48:49Z

This closes #368.

server/server.go

se7entyse7en · 2018-12-31T15:51:10Z

UPDATE: this comments now refers to #443

This PR is composed by two commits. Actually the first one is enough to close #368. Anyway with the second commit I tried to remove some duplicated code. Let me know what do you think (as obviously I don't have the big picture 😛yet), in any case it can be easily removed and it was very helpful for studying the code in depth 👍.

One thing to note regarding the duplicated code removal is that there could be a slight worsening in performance given that switch on the event type is now done once for each analyzer https://github.com/src-d/lookout/pull/442/files#diff-91bbeda7eb98a7adc57b9e47e2cf5c2bR161.

cmd/server-test/error_analyzer_test.go

server/server.go

cmd/server-test/error_analyzer_test.go

se7entyse7en · 2019-01-02T11:23:37Z

Splitted the refactor part into a separate PR.

server/server.go

dpordomingo · 2019-01-02T13:43:50Z

I saw some messages from lookout-staging bot.
Some of them may be addressed, and others might not. In any case, we should proceed as they were posted by a real user:

addressing them if possible,
explaining why they can not be addressed, or
marking them as resolved if they make no sense and no explanation is needed.

se7entyse7en · 2019-01-02T14:57:42Z

@dpordomingo Resolved and added comments on lookout-staging's messages.

dpordomingo

LGTM

server/server.go

smacker · 2019-01-04T11:01:01Z

server/server.go

+					errMessage = fmt.Sprintf("code: %s - message: %s", grpcStatus.Code(), grpcStatus.Message())
+				}
+
+				aLogger.Errorf(err, "analysis failed: %s", errMessage)


aren't we repeat code&message here?

Are you talking about errors without a friendly message? In that case the logged message is something like this:

msg="analysis failed: code: Unknown - message: review error" analyzer=Dummy app=lookoutd error="rpc error: code = Unknown desc = review error"

so code and message are repeated in the error object of the log context. Maybe something as follows is better?

if err != nil { grpcStatus := status.Convert(err) errMessage := "analysis failed" friendlyMessage, ok := logErrorMessages[grpcStatus.Code()] if ok { errMessage = fmt.Sprintf("%s: %s", errMessage, friendlyMessage) } aLogger.Errorf(err, errMessage) ... }

Yep. It makes more sense for me.

Addressed here.

carlosms · 2019-01-04T11:32:57Z

cmd/server-test/error_analyzer_test.go


 func TestErrorAnalyzerIntegrationSuite(t *testing.T) {
-	suite.Run(t, new(ErrorAnalyzerIntegrationSuite))
+	suite.Run(t, &ErrorAnalyzerIntegrationSuite{


Usually Test...Suite is just a wrapper for one stretchr/testify suite. With these two suite.Run inside the same test we get:

go test -v -parallel 1 -tags='integration' github.com/src-d/lookout/cmd/server-test -run TestErrorAnalyzerIntegrationSuite/TestAnalyzerErr === RUN TestErrorAnalyzerIntegrationSuite === RUN TestErrorAnalyzerIntegrationSuite/TestAnalyzerErr === RUN TestErrorAnalyzerIntegrationSuite/TestAnalyzerErr#01 --- PASS: TestErrorAnalyzerIntegrationSuite (15.60s) --- PASS: TestErrorAnalyzerIntegrationSuite/TestAnalyzerErr (8.33s) --- PASS: TestErrorAnalyzerIntegrationSuite/TestAnalyzerErr#01 (7.26s)

If one of the suites fails, it's hard to know which one is it. Also, it's hard to run only one of them, since they are bundled togueter.

Since you made a different mock analyzer for the timeout error, you could have two Test...Suite, probably in a different file, e.g. timeout_analyzer_test.go. startAnalyzer could be moved to common_test.go, maybe renaming it to startMockAnalyzer to avoid confusion.

Addressed here

carlosms · 2019-01-04T11:36:51Z

server/server.go


+var grpcErrorMessages = map[lookout.EventType]map[codes.Code]string{
+	pb.PushEventType: map[codes.Code]string{
+		codes.DeadlineExceeded: "timeout exceeded, try increasing analyzerPushTimeout",


analyzerPushTimeout is an internal name that will not be familiar to a user. Let's change this instead to

Suggested change

codes.DeadlineExceeded: "timeout exceeded, try increasing analyzerPushTimeout",

codes.DeadlineExceeded: "timeout exceeded, try increasing analyzer_push in config.yml",

Same for the other message below.

Addressed here.

cmd/server-test/timeout_analyzer_test.go

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

carlosms · 2019-01-04T15:22:04Z

cmd/server-test/error_analyzer_test.go


 func TestErrorAnalyzerIntegrationSuite(t *testing.T) {
-	suite.Run(t, new(ErrorAnalyzerIntegrationSuite))
+	suite.Run(t, &ErrorAnalyzerIntegrationSuite{


Now that ErrorAnalyzerIntegrationSuite and TimeoutErrorAnalyzerIntegrationSuite are different types maybe we could revert the changes for the new fields (configFile, analyzer, errMessage), and call suite.Run(t, new(ErrorAnalyzerIntegrationSuite)) here.

Addressed here.

…yzerIntegrationSuite Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

cmd/server-test/timeout_analyzer_test.go

carlosms

👏

se7entyse7en · 2019-01-04T15:41:51Z

I'm gonna rebase to master and squash some commits before merging 🎉.

smacker

👍

dpordomingo

too late, but there in case you want to address it in a separated PR

dpordomingo · 2019-01-08T13:39:02Z

cmd/server-test/timeout_analyzer_test.go

 	suite.sendEvent(successJSON)

-	suite.GrepTrue(suite.r, suite.errMessage)
+	suite.GrepTrue(suite.r, `msg="analysis failed: timeout exceeded, try increasing analyzer_review in config.yml" analyzer=Dummy app=lookoutd error="rpc error: code = DeadlineExceeded desc = context deadline exceeded"`)


@carlosms @se7entyse7en The config file could be a different file if passed with --config=; I'd purpose /config.yml/the config file/s, or similar, to cover those cases.

lookout-staging bot reviewed Dec 31, 2018

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

server/server.go Outdated Show resolved Hide resolved

server/server.go Show resolved Hide resolved

se7entyse7en force-pushed the better-failure-logging branch 2 times, most recently from c7aeb3e to e586c2c Compare December 31, 2018 15:33

se7entyse7en force-pushed the better-failure-logging branch 3 times, most recently from c75df77 to 744eb7c Compare December 31, 2018 18:33

lookout-staging bot reviewed Dec 31, 2018

View reviewed changes

cmd/server-test/error_analyzer_test.go Outdated Show resolved Hide resolved

server/server.go Outdated Show resolved Hide resolved

server/server.go Outdated Show resolved Hide resolved

server/server.go Show resolved Hide resolved

se7entyse7en commented Dec 31, 2018

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

se7entyse7en force-pushed the better-failure-logging branch from 932fa6e to 578836d Compare December 31, 2018 19:31

lookout-staging bot reviewed Jan 2, 2019

View reviewed changes

cmd/server-test/error_analyzer_test.go Outdated Show resolved Hide resolved

cmd/server-test/error_analyzer_test.go Outdated Show resolved Hide resolved

cmd/server-test/error_analyzer_test.go Outdated Show resolved Hide resolved

se7entyse7en force-pushed the better-failure-logging branch 2 times, most recently from 42709cb to 8b79c93 Compare January 2, 2019 10:28

se7entyse7en requested a review from a team January 2, 2019 10:45

se7entyse7en force-pushed the better-failure-logging branch from 8b79c93 to 0871118 Compare January 2, 2019 11:21

se7entyse7en changed the title ~~Improves logging message for failed analysis and removes duplicated logic~~ Improves logging message for failed analysis Jan 2, 2019

se7entyse7en mentioned this pull request Jan 2, 2019

Removes duplicated logic in server module #443

Closed

se7entyse7en force-pushed the better-failure-logging branch from 0871118 to 2958e90 Compare January 2, 2019 11:38

lookout-staging bot reviewed Jan 2, 2019

View reviewed changes

server/server.go Show resolved Hide resolved

dpordomingo approved these changes Jan 2, 2019

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

se7entyse7en force-pushed the better-failure-logging branch from 2958e90 to e7484c6 Compare January 2, 2019 18:17

smacker reviewed Jan 4, 2019

View reviewed changes

carlosms reviewed Jan 4, 2019

View reviewed changes

lookout-staging bot reviewed Jan 4, 2019

View reviewed changes

cmd/server-test/timeout_analyzer_test.go Show resolved Hide resolved

cmd/server-test/timeout_analyzer_test.go Show resolved Hide resolved

cmd/server-test/timeout_analyzer_test.go Show resolved Hide resolved

cmd/server-test/timeout_analyzer_test.go Outdated Show resolved Hide resolved

se7entyse7en added 3 commits January 4, 2019 15:40

Improved logging message in case of failed analysis

3c6314c

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Added a test for timeout error

9192f3b

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Dep ensured

b642088

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

se7entyse7en added 3 commits January 4, 2019 15:40

Avoided duplicated info in error message

415404d

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Made error messages even more friendlier

393df7e

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

Refactored tests

091605f

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

se7entyse7en force-pushed the better-failure-logging branch from 838465a to 091605f Compare January 4, 2019 14:41

carlosms reviewed Jan 4, 2019

View reviewed changes

Reverted change on ErrorAnalyzerIntegrationSuite and TimeoutErrorAnal…

a8b5efa

…yzerIntegrationSuite Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>

lookout-staging bot reviewed Jan 4, 2019

View reviewed changes

cmd/server-test/timeout_analyzer_test.go Show resolved Hide resolved

cmd/server-test/timeout_analyzer_test.go Show resolved Hide resolved

carlosms approved these changes Jan 4, 2019

View reviewed changes

smacker approved these changes Jan 4, 2019

View reviewed changes

se7entyse7en merged commit 7501ce9 into master Jan 4, 2019

dpordomingo reviewed Jan 8, 2019

View reviewed changes

dpordomingo deleted the better-failure-logging branch January 8, 2019 13:40

dpordomingo mentioned this pull request Jan 9, 2019

Duplicated comments when analyzer returns dupes #263

Closed

smacker mentioned this pull request Jan 9, 2019

Improve logic for posted comments filter #455

Open

	codes.DeadlineExceeded: "timeout exceeded, try increasing analyzerPushTimeout",
	codes.DeadlineExceeded: "timeout exceeded, try increasing analyzer_push in config.yml",

Conversation

se7entyse7en commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

se7entyse7en commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

se7entyse7en commented Jan 2, 2019

Uh oh!

Uh oh!

dpordomingo commented Jan 2, 2019

Uh oh!

se7entyse7en commented Jan 2, 2019

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

se7entyse7en Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlosms left a comment

Choose a reason for hiding this comment

Uh oh!

se7entyse7en commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

se7entyse7en commented Dec 31, 2018 •

edited

Loading

se7entyse7en commented Dec 31, 2018 •

edited

Loading

se7entyse7en Jan 4, 2019 •

edited

Loading

se7entyse7en commented Jan 4, 2019 •

edited

Loading

dpordomingo Jan 8, 2019 •

edited

Loading