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

Adds CanConnect API for use by health checks #13289

Merged
merged 1 commit into from
Sep 11, 2018

Conversation

ajcvickers
Copy link
Member

Fixes #13235

@ajcvickers
Copy link
Member Author

/cc @divega @glennc @rynowak

I didn't add an overload that takes a delegate since, as Diego said previously, once the context instance has been resolved, then the health check can just execute any delegate against it directly without the need for a new method.

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 11, 2018

Providers FYI?

@rynowak
Copy link
Member

rynowak commented Sep 11, 2018

I didn't add an overload that takes a delegate since, as Diego said previously, once the context instance has been resolved, then the health check can just execute any delegate against it directly without the need for a new method.

This seems fine to me.

@rynowak
Copy link
Member

rynowak commented Sep 11, 2018

I would imagine that my usage of this would look something like:

public async Task<HealthCheckResult> CheckHealthAsync(CancellationToken cancellationToken = default)
{
     var context = _context; // Assume I already resolves the DbContext type from DI
     if (!await context.Database.CanConnectAsync(cancellationToken))
     {
          return HealtCheckResult.Unhealthy(...);
     }
         
     var testDelegate = _testDelegate; // Assume the user provided a test delegate somehow
     if (testDelegate != null && !(await testDelegate(context)))
     {
           return HealthCheckResult.Unhealthy(...);
     }

     return HealthCheckResult.Healthy(...);
}

Open question from me:

  • Is is reasonable to expect the user to handle exceptions in their test delegate? or should we surround it with a try/catch.

The health system already handles exceptions that come from health checks, but they are treated differently than a 'graceful' failure. So it's worth knowing if users are likely to expect that throw == graceful failure from a test delegate using DbContext.

@rynowak
Copy link
Member

rynowak commented Sep 11, 2018

Another thing to point out and discuss. Given that this is a new API that providers need to opt in to, I was planning to document as part of the health check implementation that it will only work for providers that have implemented this interface. This means that we wouldn't put any bespoke error check in place for the case where a provider is in use that hasn't implemented the new API. So if you did this, you would see the health system report that the check failed ungracefully, and it would log the exception thrown by EF. Does that sound good enough?

@ajcvickers
Copy link
Member Author

@rynowak I was envisioning it slightly differently in that if a delegate is provided, then I don't think calling CanConnect first is useful. Anything that the delegate does will likely open the connection as a first step anyway.

With regard to exceptions, I think it is reasonable that the delegate handle any exceptions that should be treated simply as "unhealthy" and return false for those. Unhandled exceptions would then be treated in the normal ungraceful place.

With regard to handling providers that don't implement this, what you suggest sounds fine. This will be rare anyway, because all relational providers inherit from the relational base class and so will get the default "Exists" implementation anyway--notice there is no special code for SQL Server or SQLite here.

@ajcvickers ajcvickers merged commit b977e0c into release/2.2 Sep 11, 2018
@smitpatel smitpatel deleted the KnowYourNumbers0911 branch September 11, 2018 23:16
@rynowak
Copy link
Member

rynowak commented Sep 12, 2018

Good stuff, thanks!

@divega
Copy link
Contributor

divega commented Sep 12, 2018

👍

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

Successfully merging this pull request may close these issues.

None yet

5 participants