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

Write to log when error is shown in PR detail view #1199

Merged
merged 14 commits into from Aug 25, 2017

Conversation

Projects
None yet
2 participants
@jcansdale
Contributor

jcansdale commented Aug 25, 2017

These commits were originally intended to help track down the 'Couldn't find merge base' issue #1190. This issue hasn't been resolved yet, but I think having the extra logging and exception detail will be useful stand alone.

When the following error is displayed, the exception detail will also be written to the log.
couldnt find merge base

@@ -350,7 +345,7 @@ static bool HasPreamble(string file, Encoding encoding)
{
foreach (var b in encoding.GetPreamble())
{
if(b != stream.ReadByte())
if (b != stream.ReadByte())

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Look @shana, a space! ;-)

@jcansdale

jcansdale Aug 25, 2017

Contributor

Look @shana, a space! ;-)

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

rejoice

@shana

shana Aug 25, 2017

Collaborator

rejoice

@@ -351,6 +354,7 @@ public override void Initialize(ViewWithData data)
.ObserveOn(RxApp.MainThreadScheduler)
.Catch<IPullRequestModel, Exception>(ex =>
{
log.Error("Error observing GetPullRequest", ex);

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Add logs when exception is displayed on UI.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Add logs when exception is displayed on UI.

@jcansdale jcansdale requested review from grokys and shana Aug 25, 2017

Show outdated Hide outdated src/GitHub.App/Services/PullRequestService.cs
if (sha == null)
{
throw new NotFoundException($"Couldn't find merge base between {pullRequest.Base.Sha} and {pullRequest.Head.Sha}.");

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

This exception is now thrown from GetPullRequestMergeBase (with more info about what went wrong).

@jcansdale

jcansdale Aug 25, 2017

Contributor

This exception is now thrown from GetPullRequestMergeBase (with more info about what went wrong).

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

What happens if an exception is thrown from a Observable.Defer(async () =>?

@jcansdale

jcansdale Aug 25, 2017

Contributor

What happens if an exception is thrown from a Observable.Defer(async () =>?

Show outdated Hide outdated src/UnitTests/GitHub.App/Services/GitClientTests.cs
{
await gitClient.GetPullRequestMergeBase(repo, baseUri, headUri, baseSha, headSha, baseRef, headRef);
}
catch (NotFoundException) { }

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

Ignoring an exception in a test is weird. Is it supposed to throw or not?

@shana

shana Aug 25, 2017

Collaborator

Ignoring an exception in a test is weird. Is it supposed to throw or not?

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

It's certainly a bit unusual. The method it's calling would previously return a null when a comment couldn't be fetched/found. It now throws an exception.

The test is to check whether or not Fetch was called. It doesn't care about the exception or exception message. There is another test for that.

@jcansdale

jcansdale Aug 25, 2017

Contributor

It's certainly a bit unusual. The method it's calling would previously return a null when a comment couldn't be fetched/found. It now throws an exception.

The test is to check whether or not Fetch was called. It doesn't care about the exception or exception message. There is another test for that.

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

Ah ok, in that case we should probably document that with a comment, so we know in a year from now what the assumption here is.

@shana

shana Aug 25, 2017

Collaborator

Ah ok, in that case we should probably document that with a comment, so we know in a year from now what the assumption here is.

@@ -394,7 +394,7 @@ public Task<bool> IsModified(IRepository repository, string path, byte[] content
baseCommit = repo.Lookup<Commit>(baseSha);
if (baseCommit == null)
{
return null;
throw new NotFoundException($"Couldn't find {baseSha} after fetching from {baseCloneUrl}:{baseRef}.");

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

So I understand why you'd want the details of the shas in the log, but this error message is incredibly cryptic, there is no way anyone is going to know what is going on with an error like this, and this is going to be displayed directly to a user where it's going to be completely useless.

We should have a message for the user, and another with more details for the log. If the log is only showing the message and stacktrace, we can set InnerException with the log details and have the outer exception be for the user.

@shana

shana Aug 25, 2017

Collaborator

So I understand why you'd want the details of the shas in the log, but this error message is incredibly cryptic, there is no way anyone is going to know what is going on with an error like this, and this is going to be displayed directly to a user where it's going to be completely useless.

We should have a message for the user, and another with more details for the log. If the log is only showing the message and stacktrace, we can set InnerException with the log details and have the outer exception be for the user.

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

Also, are all the callers of this method expecting an exception to be thrown and handling it properly?

@shana

shana Aug 25, 2017

Collaborator

Also, are all the callers of this method expecting an exception to be thrown and handling it properly?

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Callers of this method previously turned the null into an exception themselves. I've also tested it by making this method always throw to check that nothing bad happens. The exception message is displayed as expected.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Callers of this method previously turned the null into an exception themselves. I've also tested it by making this method always throw to check that nothing bad happens. The exception message is displayed as expected.

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

I wonder what the user facing message should say? I suspect that for some reason, fetch doesn't always work or there is some race condition. I guess we could suggest thry try again or maybe refresh. I'll investagate...

@jcansdale

jcansdale Aug 25, 2017

Contributor

I wonder what the user facing message should say? I suspect that for some reason, fetch doesn't always work or there is some race condition. I guess we could suggest thry try again or maybe refresh. I'll investagate...

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

Yeah, an indication of what the user action should be in the case of this error would be great!

@shana

shana Aug 25, 2017

Collaborator

Yeah, an indication of what the user action should be in the case of this error would be great!

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Here we go:
image

@jcansdale

jcansdale Aug 25, 2017

Contributor

Here we go:
image

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

So "merge base" may be meaningless to a user, and try again might not be helpful either if the action that the user performed to get here isn't available from this screen.

What is the action that failed here? Opening a file? Showing the PR details? The first part of the message should say what was the action that failed.

What is try again? Refresh? Click on the PR button in the toolbar and then clicking on the PR again? The second part should say what the user can do on this screen to attempt to make it work.

Assuming it was the PR that failed to load, then we should probably say something like

The Pull Request failed to load. Please check your network connection and click refresh to try again.

Adjust accordingly for the failed action.

@shana

shana Aug 25, 2017

Collaborator

So "merge base" may be meaningless to a user, and try again might not be helpful either if the action that the user performed to get here isn't available from this screen.

What is the action that failed here? Opening a file? Showing the PR details? The first part of the message should say what was the action that failed.

What is try again? Refresh? Click on the PR button in the toolbar and then clicking on the PR again? The second part should say what the user can do on this screen to attempt to make it work.

Assuming it was the PR that failed to load, then we should probably say something like

The Pull Request failed to load. Please check your network connection and click refresh to try again.

Adjust accordingly for the failed action.

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Okay, I've chained it to variants of your suggestion above.

My concern was that we hide the real issue behind a workaround. I'm still keen to get to the bottom of what's going wrong. My hunch is that it's a race condition in gitlib2sharp. I'll see if I can reproduce the issue with some standalone code... 😄

@jcansdale

jcansdale Aug 25, 2017

Contributor

Okay, I've chained it to variants of your suggestion above.

My concern was that we hide the real issue behind a workaround. I'm still keen to get to the bottom of what's going wrong. My hunch is that it's a race condition in gitlib2sharp. I'll see if I can reproduce the issue with some standalone code... 😄

This comment has been minimized.

@shana

shana Aug 25, 2017

Collaborator

Yes, we don't want to hide the issue in a workaround. If you're concerned about people not reporting it, you can add "If this issue persists, please let us know at support@github.com"

@shana

shana Aug 25, 2017

Collaborator

Yes, we don't want to hide the issue in a workaround. If you're concerned about people not reporting it, you can add "If this issue persists, please let us know at support@github.com"

This comment has been minimized.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Done. Also confirmed that nested exceptions appear in logs.

@jcansdale

jcansdale Aug 25, 2017

Contributor

Done. Also confirmed that nested exceptions appear in logs.

jcansdale added some commits Aug 25, 2017

Remove null check after GetPullRequestMergeBase
This method now throws `NotFoundException` rather than returning null.
@shana

shana approved these changes Aug 25, 2017

:shipit:

@jcansdale jcansdale merged commit 19bf7aa into master Aug 25, 2017

5 checks passed

GitHub CLA @jcansdale has accepted the GitHub Contributor License Agreement.
Details
VisualStudio Build #7769644 succeeded in 96s
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins/build_log Jenkins Build Log
Details

@jcansdale jcansdale deleted the jcansdale/add-more-logging branch Aug 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment