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.

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

Lookout verbose option #37

Merged
merged 1 commit into from
Jul 20, 2018
Merged

Lookout verbose option #37

merged 1 commit into from
Jul 20, 2018

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Jul 19, 2018

Second (zero) part of #29

@smacker smacker requested review from carlosms and bzz July 19, 2018 16:37
@bzz bzz mentioned this pull request Jul 20, 2018
3 tasks
@bzz
Copy link
Contributor

bzz commented Jul 20, 2018

Do you think there is a value to have some logs printed for opening/closing a connection in non-verbose mode?

Right now experience using time ./lookout review ipv4://localhost:10302 is a bit confusing - after running it, one needs to wait ~3-4sec when nothing happens until BEGIN RESULT shows up.

Suggestion:

  1. Success
Connected to analyzer at XXX
Sending NotifyReview request
BEGIN RESUL

if something like this is printed, a user would have more patience.

  1. Failure
    Right now, if time ./lookout review ipv4://localhost:666 just has no feedback to user at all.
    Suggestion:
Failed to connect to analyzer at XXX
Retrying (1 out of Y)
...
Failed to connect to analyzer at XXX
Retrying (2 out of Y)

Having both of those is important part of SDK: it would allow to wave the mistakes related to networking for a user and thus will help building trust in SDK (he does not need to check with nc manually if server is reachable, etc)

Also, do you think there is a value of applying same -v for dummy analyzer as well?
May be does not need to be that detailed, but may be nice to have

Copy link
Contributor

@bzz bzz left a comment

Choose a reason for hiding this comment

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

Failure to connect to analyzer without -v it's very hard to identify: does it taking to long? does it fail to connect?

@smacker
Copy link
Contributor Author

smacker commented Jul 20, 2018

@bzz

Failure to connect to analyzer without -v it's very hard to identify: does it taking to long? does it fail to connect?

this is why commits are in opposite order to issue bullets.

With the prev commit it will fail fast with correct error.

Failed to connect to analyzer at XXX
Retrying (1 out of Y)

we removed it in prev commit

Connected to analyzer at XXX
Sending NotifyReview request

would be nice 👍 let's update issue/create new one with description of proper success logging across binaries and implement it.

@bzz
Copy link
Contributor

bzz commented Jul 20, 2018

Got it, thank you for explaining - assume you are talking about #34

would be nice 👍 let's update issue/create new one with description of proper success logging across binaries and implement it.

Improving in separate issue sounds good - would you please create one so the feedback is not getting lost?

Copy link
Contributor

@bzz bzz left a comment

Choose a reason for hiding this comment

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

SGTM, after issue for logging is created

@smacker smacker mentioned this pull request Jul 20, 2018
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
@smacker
Copy link
Contributor Author

smacker commented Jul 20, 2018

rebased on master. Merge after CI is green.

@smacker smacker merged commit 56c27f4 into src-d:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants