Skip to content
This repository has been archived by the owner on Dec 8, 2018. It is now read-only.

Add IHealthCheckPublisher for push-based checks #498

Merged
merged 2 commits into from
Oct 9, 2018

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented Oct 7, 2018

IHealthCheckPublisher allows you to configure and run health checks
regularly inside an application, and push the notifications elsewhere.

All publishers are part of a single queue with a configurable period and
timeout.

IHealthCheckPublisher allows you to configure and run health checks
regularly inside an application, and push the notifications elsewhere.

All publishers are part of a single queue with a configurable period and
timeout.
/// <returns>A <see cref="Task"/> which will complete when publishing is complete.</returns>
Task PublishAsync(HealthReport report, CancellationToken cancellationToken);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, this is going to go in sometime soon, please let us know if you have any feedback because it will be hard to change in about a week 😆

@rynowak
Copy link
Member Author

rynowak commented Oct 8, 2018

/cc @davidfowl @pranavkm @Tratcher

cancellation = CancellationTokenSource.CreateLinkedTokenSource(_stopping.Token);
if (timeout != Timeout.InfiniteTimeSpan)
{
cancellation.CancelAfter(timeout);
Copy link
Member

Choose a reason for hiding this comment

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

CancelAfter understands InfiniteTimeSpan, you don't need an explicit check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool beans 👍

_publishers = publishers.ToArray();
}

internal bool IsCancelled => _stopping == null || _stopping.IsCancellationRequested;
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, more bad races, don't null out _stopping

Copy link
Member

Choose a reason for hiding this comment

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

IsStopping

}
catch (Exception ex)
{
// Handle any exceptions that are thrown synchronously - this allows us to start
Copy link
Member

Choose a reason for hiding this comment

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

async/await methods don't throw exceptions synchronously, they capture all exceptions and throw them later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think this here from before I wrote RunPublisherAsync. This code used to call the publisher directly.

I can't imagine a world where we make RunPublisherAsync into a sync implementation - so I'm ok with this change.

/// <see cref="IHealthCheckPublisher"/> instances. The delay is applied once at startup, and does
/// not apply to subsequent iterations. The default value is 5 seconds.
/// </summary>
public TimeSpan Delay { get; set; } = TimeSpan.FromSeconds(5);
Copy link
Member

Choose a reason for hiding this comment

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

Valid range? What if I set 0 or InfinateTimeSpan?

/// Gets or sets the period of <see cref="IHealthCheckPublisher"/> execution. The default value is
/// 30 seconds.
/// </summary>
public TimeSpan Period { get; set; } = TimeSpan.FromSeconds(30);
Copy link
Member

Choose a reason for hiding this comment

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

Valid range? What if I set 0 or InfinateTimeSpan?

@rynowak
Copy link
Member Author

rynowak commented Oct 8, 2018

@Tratcher Updated

@@ -4,6 +4,8 @@
<handlers>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this file even here? It's not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@@ -77,6 +77,8 @@ public Task StartAsync(CancellationToken cancellationToken = default)

public Task StopAsync(CancellationToken cancellationToken = default)
{
_stopping.Cancel();
Copy link
Member

Choose a reason for hiding this comment

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

try/catch Exception, callbacks are invoked synchronously and can throw.

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

Successfully merging this pull request may close these issues.

3 participants