Add warning level log for failover to new endpoint #431
Conversation
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/LogHelper.cs
Outdated
Show resolved
Hide resolved
| @@ -827,6 +829,13 @@ private async Task<T> ExecuteWithFailOverPolicyAsync<T>(IEnumerable<Configuratio | |||
| T result = await funcToExecute(currentClient).ConfigureAwait(false); | |||
| success = true; | |||
|
|
|||
| Uri currentEndpoint = _configClientManager.GetEndpointForClient(currentClient); | |||
|
|
|||
| if (failureOccurred && originalEndpoint != currentEndpoint) | |||
There was a problem hiding this comment.
This would log only when (if) there is a successful operation. We should be logging all endpoints in the same order that they were tried in.
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Constants/LoggingConstants.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
…etProvider into ajusupovic/failover-warning-log
…etProvider into ajusupovic/failover-warning-log
|
|
||
| public static string BuildFailoverToDifferentEndpointMessage(string originalEndpoint, string currentEndpoint) | ||
| { | ||
| return $"{LoggingConstants.RefreshFailedToGetSettingsFromEndpoint} '{originalEndpoint.TrimEnd('/')}'. {LoggingConstants.RefreshTryingToReadFromEndpoint} '{currentEndpoint.TrimEnd('/')}'."; |
There was a problem hiding this comment.
It was the pattern we followed with the original PR that added logging, since the endpoints always have a '/' at the end.
There was a problem hiding this comment.
Okay.
Parameters need validation for null.
There was a problem hiding this comment.
Good catch! We need it throughout this helper class.
There was a problem hiding this comment.
So just for the endpoint parameters because we call TrimEnd right?
There was a problem hiding this comment.
Yes. Without parameter validation it's a possible null-ref.
| if (previousEndpoint != currentEndpoint) | ||
| { | ||
| _logger.LogWarning(LogHelper.BuildFailoverToDifferentEndpointMessage(previousEndpoint.ToString(), currentEndpoint.ToString())); | ||
| } |
There was a problem hiding this comment.
This seems like it should be after the finally block.
There was a problem hiding this comment.
I agree from how it looks, but then it seems like I need to either make an extra call to GetEndpointforClient or update currentEndpoint at the bottom of the while loop and I wasn't sure which made more sense.
There was a problem hiding this comment.
Think about the happy path. The call to GetEndpointForClient is actually unnecessary.
What we need to know is the client we use to make the attempt. We can get the endpoint later if we need to log.
There was a problem hiding this comment.
Oh I got it, thanks
| @@ -74,5 +74,15 @@ public static string BuildPushNotificationUnregisteredEndpointMessage(string res | |||
| { | |||
| return $"{LoggingConstants.PushNotificationUnregisteredEndpoint} '{resourceUri}'."; | |||
| } | |||
|
|
|||
| public static string BuildFailoverToDifferentEndpointMessage(string originalEndpoint, string currentEndpoint) | |||
There was a problem hiding this comment.
How about just BuildFailoverMessage?
| @@ -74,5 +74,15 @@ public static string BuildPushNotificationUnregisteredEndpointMessage(string res | |||
| { | |||
| return $"{LoggingConstants.PushNotificationUnregisteredEndpoint} '{resourceUri}'."; | |||
| } | |||
|
|
|||
| public static string BuildFailoverToDifferentEndpointMessage(string originalEndpoint, string currentEndpoint) | |||
| { | |||
There was a problem hiding this comment.
For the message it looks like it's currently
"Failed to get configuration settings from endpoint '{old endpoint}'. Trying to read from endpoint '{new endpoint}'."
I'm thinking to make the fall back clear:
"Failed to get configuration settings from endpoint '{old endpoint}'. Falling back to endpoint '{new endpoint}'."
There was a problem hiding this comment.
That's a good idea, can we use "failover" instead of "fallback" though? That's the term we use in public docs too.
"Failed to get configuration settings from endpoint '{old endpoint}'. Initiating automatic failover to endpoint '{new endpoint}'."
There was a problem hiding this comment.
Yes. "Failing over to endpoint ..."
No description provided.