-
Notifications
You must be signed in to change notification settings - Fork 9
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
Added debugging option to Readiness and Liveness probe #36
Added debugging option to Readiness and Liveness probe #36
Conversation
@Aaronontheweb I set a pull request to make sure I'm on the right path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggested changes
@@ -102,16 +102,19 @@ public AkkaHealthCheck(HealthCheckSettings settings, ExtendedActorSystem system) | |||
public static IActorRef StartTransportActor(ITransportSettings settings, ExtendedActorSystem system, | |||
ProbeKind probeKind, IActorRef probe) | |||
{ | |||
var _log = system.Log; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use a local variable here. Just call ActorSystem.Log
.
If you can create a custom ILoggingAdapter
that names itself "AkkaHealthCheck` that would be ideal and stick it into a private readonly field that would be ideal - that way it's clear where the log source comes from.
I would also make the logging level configurable - turning on akka.loglevel = DEBUG
turns on a massive firehose of log messages coming from all sorts of systems and this data will get lost in the shuffle. Better to have a HOCON option that specifies the loglevel to be used for all debug / diagnositic logging used by Akka.HealthCheck's internals (i.e. everything that runs inside the built-in transports, built-in probes, AND the AkkaHealthCheck
plugin itself).
By default this value should be Debug
, but a user can change the HOCON setting to change it to INFO
if need be. I think that'd be a better option - keeps the verbosity under control.
Receive<GetCurrentReadiness>(_ => Sender.Tell(_readinessStatus)); | ||
Receive<GetCurrentReadiness>(_ => { | ||
Sender.Tell(_readinessStatus); | ||
_log.Debug("Rediness Status {0} reported", _readinessStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ReadinessStatus
override its ToString
method so the output is human-readable? Otherwise the default formatting may not be all that nice here.
if (settings is FileTransportSettings fileTransport) | ||
switch (probeKind) | ||
{ | ||
case ProbeKind.Liveness: | ||
_log.Debug("Liveness file transport created"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to log the actual settings - where is the file being written, which network port is being opened, etc...
|
||
# Log the complete configuration at INFO level when the actor system is started. | ||
# This is useful when you are uncertain of what configuration is used. | ||
log-config-on-start = on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these settings being parsed into the AkkaHealthCheckSettings
class anywhere?
Pull request for issue #33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor stylistic nitpick, but once that's done we're good to go.
#### 0.2.0 March 15 2019 #### | ||
**Now running on Akka.NET v1.3.12** | ||
#### 0.2.1 March 20th 2019 #### | ||
**Added startup log information option** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
} | ||
|
||
[Fact(DisplayName = "Should be able load non-default log options")] | ||
public void Should_load_non_default_Log_Settings() | ||
{ | ||
var hocon = ConfigurationFactory.ParseString(@" | ||
akka.healthcheck.log-config-on-start = false | ||
"); | ||
akka.healthcheck{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -81,10 +82,10 @@ public AkkaHealthCheck(HealthCheckSettings settings, ExtendedActorSystem system) | |||
|
|||
// Need to set up transports (possibly) | |||
LivenessTransportActor = StartTransportActor(Settings.LivenessTransportSettings, system, ProbeKind.Liveness, | |||
LivenessProbe); | |||
LivenessProbe, Settings.LogInfoEvents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -44,6 +44,8 @@ public HealthCheckSettings(Config healthcheckConfig) | |||
ReadinessTransportSettings = PopulateSettings(healthcheckConfig.GetConfig("readiness"), ReadinessTransport); | |||
|
|||
LogConfigOnStart = healthcheckConfig.GetBoolean("log-config-on-start"); | |||
|
|||
LogInfoEvents = healthcheckConfig.GetBoolean("log-info"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -22,16 +22,19 @@ public sealed class LivenessTransportActor : ReceiveActor | |||
private readonly IActorRef _livenessProbe; | |||
private readonly ILoggingAdapter _log = Context.GetLogger(); | |||
private readonly IStatusTransport _statusTransport; | |||
private readonly bool logInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use _logInfo
here (style)
@@ -22,16 +22,20 @@ public sealed class ReadinessTransportActor : ReceiveActor | |||
private readonly ILoggingAdapter _log = Context.GetLogger(); | |||
private readonly IActorRef _readinessProbe; | |||
private readonly IStatusTransport _statusTransport; | |||
private readonly bool logInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
@@ -13,6 +13,7 @@ static void Main(string[] args) | |||
akka{ | |||
healthcheck{ | |||
log-config-on-start = on | |||
log-info = on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Added debugging option to Readiness * Added transfer type message * Fixed suggested error * Incorrect change * Removed test that is not needed * Missing an s * Added log info message * Updated config * Updated ReadMeFile * Updated Release Notes _2 * Updated variable name in transport actors
Staring to Add the debugging option for Liveness and Readiness Probe