Removes duplicated logic in server module by se7entyse7en · Pull Request #443 · src-d/lookout

se7entyse7en · 2019-01-02T11:22:06Z

This depends on #442. Please see here.

dpordomingo · 2019-01-02T11:29:54Z

Drone is failing because a transient failure.
You can rerun it restarting the job.
To do so, you just need to login, and then click RESTART

If you're not allowed to restart a job, I think you should ask @src-d/infrastructure creating an issue into infrastructure repo

lookout-staging · 2019-01-02T11:32:11Z

cmd/server-test/error_analyzer_test.go


+type sleepyErrAnalyzer struct{}
+
+func (a *sleepyErrAnalyzer) NotifyReviewEvent(ctx context.Context, e *lookout.ReviewEvent) (*lookout.EventResponse, error) {


line is 124 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2019-01-02T11:32:12Z

cmd/server-test/error_analyzer_test.go

 	suite.sendEvent(successJSON)

-	suite.GrepTrue(suite.r, `msg="analysis failed" analyzer=Dummy app=lookoutd error="rpc error: code = Unknown desc = review error"`)
+	suite.GrepTrue(suite.r, `msg="analysis failed: code: Unknown - message: review error" analyzer=Dummy app=lookoutd error="rpc error: code = Unknown desc = review error"`)


line is 170 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2019-01-02T11:32:12Z

cmd/server-test/error_analyzer_test.go

+	suite.Run(t, &ErrorAnalyzerIntegrationSuite{
+		configFile: dummyConfigFile,
+		analyzer:   &errAnalyzer{},
+		errMessage: `msg="analysis failed: code: Unknown - message: review error" analyzer=Dummy app=lookoutd error="rpc error: code = Unknown desc = review error"`,


line is 159 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2019-01-02T11:32:12Z

cmd/server-test/error_analyzer_test.go

+	suite.Run(t, &ErrorAnalyzerIntegrationSuite{
+		configFile: dummyConfigFileWithTimeouts,
+		analyzer:   &sleepyErrAnalyzer{},
+		errMessage: `msg="analysis failed: timeout exceeded, try increasing analyzerReviewTimeout" analyzer=Dummy app=lookoutd error="rpc error: code = DeadlineExceeded desc = context deadline exceeded"`,


line is 198 characters (lll)

If you have feedback about this comment, please, tell us.

lookout-staging · 2019-01-02T11:32:12Z

server/server.go

+	getAnalyzerNotifier := func(ctx context.Context, settings map[string]interface{}) (analyzerNotifier, error) {
+		st := grpchelper.ToPBStruct(settings)
+		switch ev := e.(type) {
+		case *lookout.ReviewEvent:


duplicate of server/server.go:172-179 (dupl)

If you have feedback about this comment, please, tell us.

lookout-staging · 2019-01-02T11:32:12Z

server/server.go

+				func(ctx context.Context, a lookout.AnalyzerClient) (*pb.EventResponse, error) {
+					return a.NotifyReviewEvent(ctx, ev)
+				}, s.analyzerReviewTimeout}, nil
+		case *lookout.PushEvent:


duplicate of server/server.go:164-171 (dupl)

If you have feedback about this comment, please, tell us.

lookout-staging · 2019-01-02T11:32:12Z

server/server.go

 }

-func (s *Server) concurrentRequest(ctx context.Context, conf map[string]lookout.AnalyzerConfig, send reqSent) ([]lookout.AnalyzerComments, error) {
+func (s *Server) concurrentRequest(ctx context.Context, conf map[string]lookout.AnalyzerConfig, send reqSent, logErrorMessages map[codes.Code]string) ([]lookout.AnalyzerComments, error) {


line is 187 characters (lll)

If you have feedback about this comment, please, tell us.

dpordomingo

Thanks for caring!
I think it's a good idea caring about removing duplicate code, but there are some suggestions that I'd like to do.

There is also a personal concern about changing the behavior of Server.HandleReview(Context, *ReviewEvent, bool) error when processing unsuported Events.

I also wonder if the initial implementation was coded that way (with that dupe code) considering any planned change, that might cause that both event handlers were totally different.

dpordomingo · 2019-01-02T12:05:57Z

server/server.go

+				ev.Configuration = *st
+			}
+			return analyzerNotifier{
+				func(ctx context.Context, a lookout.AnalyzerClient) (*pb.EventResponse, error) {


We try to avoid structs initialized with positional arguments because they're harder to read and can cause problems in some cases (like changing the order of properties having the same type, or adding new properties).

imho I'd prefer:

return analyzerNotifier{ notify: func... timeot: s.analyzerReviewTimeout }, nil

dpordomingo · 2019-01-02T12:18:27Z

server/server.go

+					return a.NotifyPushEvent(ctx, ev)
+				}, s.analyzerPushTimeout}, nil
+		default:
+			ctxlog.Get(ctx).Debugf("ignoring unsupported event: %s", ev)


I think we're moving the logic to validate the event too deep

s.HandleEvent(ctx, strangeEvent) -> s.AnalyzeAndComment(ctx, strangeEvent, isSafePosting) -> s.analyze(ctx, strangeEvent, isSafePosting) -> s.getSender(strangeEvent) -> getAnalyzerNotifier(ctx, errorMsgMap) -> if unsupported return fmt.Errorf("unsupported")

instead of how we did initially:

s.HandleEvent(ctx, strangeEvent) -> if unsupported fmt.Debug("unsupported")

I do not see the benefit of moving that validation so deep.

That's another point against the refactor. The reason for this was to avoid checking twice the type of the event using the switch.

dpordomingo · 2019-01-02T12:24:33Z

server/server.go

+				}, s.analyzerPushTimeout}, nil
+		default:
+			ctxlog.Get(ctx).Debugf("ignoring unsupported event: %s", ev)
+			return analyzerNotifier{}, fmt.Errorf("unsupported event: %s", ev)


I might be mistaken, but this PR is also changing the s.HandleEvent(ctx, strangeEvent) behavior when it is sent an event that was not a ReviewEvent nor a PushEvent;
If I'm rightm the s.HandleEvent(ctx, strangeEvent) will fail with an unsupported error message, instead of just logging a debug unsupported message.

If that's true, I think we should not change the behavior in a refactor PR or without a clear reasons to do so.

You're right. With these changes the HandleEvent method fails in case on supported event. Another thing to note with this refactor is that in case of unsupported event the check is so deep that it even spawns a goroutine for each analyzer. And this is a big no-no in my opionion.

dpordomingo · 2019-01-02T12:26:38Z

server/server.go

+}

+func (s *Server) analyze(ctx context.Context, e lookout.Event, safePosting bool) ([]lookout.AnalyzerComments, error) {
+	ctxlog.Get(ctx).Infof("processing event type %d", e.Type())


I see more cryptic the new error message: event type 2 instead of pull request

Agree on this too, in fact an idea could be to add a stringer to EventType in lookout-sdk.

In other place we are doing reflect.TypeOf(e).String()

dpordomingo · 2019-01-02T12:27:44Z

server/server.go

+}

+func (s *Server) analyze(ctx context.Context, e lookout.Event, safePosting bool) ([]lookout.AnalyzerComments, error) {
+	ctxlog.Get(ctx).Infof("processing event type %d", e.Type())


I think we're also missing info about the event provider (see 1326678#diff-91bbeda7eb98a7adc57b9e47e2cf5c2bL188)

You're right, I forgot to add it in to the log ctx.

dpordomingo · 2019-01-02T12:31:08Z

server/server.go

+	if err == nil {
+		if err := s.post(ctx, e, comments, safePosting); err != nil {
+			s.status(ctx, e, lookout.ErrorAnalysisStatus)
+			err = fmt.Errorf("posting analysis failed: %s", err)


This assignment does not apply over first err definition #line-124, but over its own scope #line-126

We avoid it, doing early returns; it means, that we stop processing as soon as an error condition is met; example:

if err := doSomething(); err != nil { return fmt.Errorf("new deep error") // we stop processing } // do other things

instead of moving errors and reusing variables containing it, like in your example:

var err error if err := doSomething(); err != nil { // we shadow the upper err definition err = fmt.Errorf("new deep error") // this definition does not apply over the upper scope } return err // nothing was overriden; we got <nil>

You can see an example about it at: https://play.golang.org/p/I75A6xUtJgN

dpordomingo · 2019-01-02T12:31:36Z

server/server.go

+			s.status(ctx, e, lookout.SuccessAnalysisStatus)
+		}
+	}
+	return err


we always introduce a new line after every code block

dpordomingo · 2019-01-02T12:32:54Z

server/server.go

+	var comments []lookout.AnalyzerComments
 	if err := e.Validate(); err != nil {
-		return err
+		return comments, err


we use to avoid indirections when doing an early return caused by an error; example:

func doSomething() (string, error) { var msg string // do things, operate with msg... // at some point, we check an error, and we do: if err := checkSomething(); err != nil { return "", err // instead of `return msg, err`; relying on a previous value of `msg` } }

That way, we do explicit the things that we return

dpordomingo · 2019-01-02T12:33:11Z

server/server.go

 	conf, err := s.getConfig(ctx, e)
 	if err != nil {
-		return err
+		return comments, err


dpordomingo · 2019-01-02T12:33:44Z

server/server.go

+		var comments []*lookout.Comment
+		analyzerNotifier, err := getAnalyzerNotifier(ctx, settings)
+		if err != nil {
+			return comments, err


idem. about returning errors.

dpordomingo · 2019-01-02T12:42:36Z

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-02T15:00:06Z

I also wonder if the initial implementation was coded that way (with that dupe code) considering any planned change, that might cause that both event handlers were totally different.

Exactly, I was thinking the same thing that I could have been missing the big picture and future planned change.

se7entyse7en · 2019-01-02T15:22:44Z

Overall it seems that the are some possible performance issues by eye. And even if it removes some dup code, it may trade-off the linearity of the steps too much.

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

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

cmd/server-test/dummy_analyzer_test.go

 	suite.sendEvent(successJSON)
 	suite.GrepAll(suite.r, []string{
-		`processing pull request`,
+		`processing event type 2`,


hm. The previous message was much better.

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

Usually, it's a good idea to remove duplicated code, but I see 2 problems with this PR:

Before there was a much more clear separation of handling different events. Despite the fact that right now we handle them in a very similar manner it can be not true anymore in the future. (for example when we start processing push event instead of just ignoring it) I believe this refactoring would make such potential changes very difficult. Same applies to new types of events.
As it was already mentioned the check is too deep (it runs go-routines for unsupported events!)

Though I agree a current code of this module needs some improvements.

se7entyse7en · 2019-01-04T13:55:43Z

I'm gonna close this PR ;)

se7entyse7en mentioned this pull request Jan 2, 2019

Improves logging message for failed analysis #442

Merged

lookout-staging bot reviewed Jan 2, 2019

View reviewed changes

se7entyse7en force-pushed the remove-dup-code branch from 77b37fc to 1326678 Compare January 2, 2019 11:39

dpordomingo suggested changes Jan 2, 2019

View reviewed changes

dpordomingo assigned se7entyse7en Jan 2, 2019

dpordomingo added the enhancement New feature or request label Jan 2, 2019

se7entyse7en added 4 commits January 2, 2019 19:17

Improved logging message in case of failed analysis

2b505fc

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

Added a test for timeout error

7974a6d

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

Dep ensured

e7484c6

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

Refactored server module in order to remove some duplicated code

d76d5b5

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

se7entyse7en force-pushed the remove-dup-code branch from 1326678 to d76d5b5 Compare January 2, 2019 18:19

smacker reviewed Jan 4, 2019

View reviewed changes

se7entyse7en closed this Jan 4, 2019

dpordomingo deleted the remove-dup-code branch January 9, 2019 10:54


		type sleepyErrAnalyzer struct{}

		func (a sleepyErrAnalyzer) NotifyReviewEvent(ctx context.Context, e lookout.ReviewEvent) (*lookout.EventResponse, error) {

Conversation

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

Uh oh!

Uh oh!

dpordomingo commented Jan 2, 2019

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

lookout-staging bot Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

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!

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!

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!

dpordomingo commented Jan 2, 2019

Uh oh!

se7entyse7en commented Jan 2, 2019

Uh oh!

se7entyse7en commented Jan 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smacker commented Jan 4, 2019

Uh oh!

se7entyse7en commented Jan 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

se7entyse7en commented Jan 2, 2019 •

edited

Loading