Skip to content

Commit

Permalink
Logger Improvements (#194)
Browse files Browse the repository at this point in the history 
* Always check if log level is enabled

* Don't create string unless we are logging

* Remove unnecessary wrapping and generics

* Don't allow passing strings to logger

Since the caller often doesn't check if a given log level is enabled
before trying to log we shouldn't allow them to log a string directly,
rather use a function to avoid allocating the string if the log level
isn't enabled.

* Add back public API surface
  • Loading branch information
@nathanmascitelli
nathanmascitelli authored Jan 3, 2024
1 parent 4838030 commit e86849d
Show file tree
Hide file tree
Showing 11 changed files with 112 additions and 256 deletions.
24 changes: 12 additions & 12 deletions src/Unleash/Communication/UnleashApiClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ internal class UnleashApiClient : IUnleashApiClient
private int featureRequestsSkipped = 0;
private int metricsRequestsToSkip = 0;
private int metricsRequestsSkipped = 0;
private readonly int[] backoffResponses =
private readonly int[] backoffResponses =
new int[]
{
429,
500,
502,
503,
504
504
};
private readonly int[] configurationErrorResponses =
new int[]
Expand All @@ -45,8 +45,8 @@ internal class UnleashApiClient : IUnleashApiClient
404,
};
public UnleashApiClient(
HttpClient httpClient,
IJsonSerializer jsonSerializer,
HttpClient httpClient,
IJsonSerializer jsonSerializer,
UnleashApiClientRequestHeaders clientRequestHeaders,
EventCallbackConfig eventConfig,
string projectId = null)
Expand Down Expand Up @@ -108,7 +108,7 @@ private async Task<FetchTogglesResult> HandleErrorResponse(HttpResponseMessage r
}

var error = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
Logger.Trace($"UNLEASH: Error {response.StatusCode} from server in '{nameof(FetchToggles)}': " + error);
Logger.Trace(() => $"UNLEASH: Error {response.StatusCode} from server in '{nameof(FetchToggles)}': " + error);
eventConfig?.RaiseError(new ErrorEvent() { ErrorType = ErrorType.Client, StatusCode = response.StatusCode, Resource = resourceUri });

return new FetchTogglesResult
Expand All @@ -120,7 +120,7 @@ private async Task<FetchTogglesResult> HandleErrorResponse(HttpResponseMessage r
private void Backoff(HttpResponseMessage response)
{
featureRequestsToSkip = Math.Min(10, featureRequestsToSkip + 1);
Logger.Warn($"UNLEASH: Backing off due to {response.StatusCode} from server in '{nameof(FetchToggles)}'.");
Logger.Warn(() => $"UNLEASH: Backing off due to {response.StatusCode} from server in '{nameof(FetchToggles)}'.");
}

private void ConfigurationError(HttpResponseMessage response, string requestUri)
Expand All @@ -129,15 +129,15 @@ private void ConfigurationError(HttpResponseMessage response, string requestUri)

if (response.StatusCode == HttpStatusCode.NotFound)
{
Logger.Error($"UNLEASH: Error when fetching toggles, {requestUri} responded NOT_FOUND (404) which means your API url most likely needs correction.'.");
Logger.Error(() => $"UNLEASH: Error when fetching toggles, {requestUri} responded NOT_FOUND (404) which means your API url most likely needs correction.'.");
}
else if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.Forbidden)
{
Logger.Error($"UNLEASH: Error when fetching toggles, {requestUri} responded FORBIDDEN (403) which means your API token is not valid.");
Logger.Error(() => $"UNLEASH: Error when fetching toggles, {requestUri} responded FORBIDDEN (403) which means your API token is not valid.");
}
else
{
Logger.Error($"UNLEASH: Configuration error due to {response.StatusCode} from server in '{nameof(FetchToggles)}'.");
Logger.Error(() => $"UNLEASH: Configuration error due to {response.StatusCode} from server in '{nameof(FetchToggles)}'.");
}
}

Expand All @@ -147,7 +147,7 @@ private async Task<FetchTogglesResult> HandleSuccessResponse(HttpResponseMessage

var newEtag = response.Headers.ETag?.Tag;
if (newEtag == etag)
{
{
return new FetchTogglesResult
{
HasChanged = false,
Expand Down Expand Up @@ -198,7 +198,7 @@ public async Task<bool> RegisterClient(ClientRegistration registration, Cancella
return true;

var error = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
Logger.Trace($"UNLEASH: Error {response.StatusCode} from request '{requestUri}' in '{nameof(UnleashApiClient)}': " + error);
Logger.Trace(() => $"UNLEASH: Error {response.StatusCode} from request '{requestUri}' in '{nameof(UnleashApiClient)}': " + error);
eventConfig?.RaiseError(new ErrorEvent() { Resource = requestUri, ErrorType = ErrorType.Client, StatusCode = response.StatusCode });

return false;
Expand Down Expand Up @@ -266,7 +266,7 @@ private async Task HandleMetricsErrorResponse(HttpResponseMessage response, stri
}

var error = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
Logger.Trace($"UNLEASH: Error {response.StatusCode} from request '{requestUri}' in '{nameof(UnleashApiClient)}': " + error);
Logger.Trace(() => $"UNLEASH: Error {response.StatusCode} from request '{requestUri}' in '{nameof(UnleashApiClient)}': " + error);
eventConfig?.RaiseError(new ErrorEvent() { Resource = requestUri, ErrorType = ErrorType.Client, StatusCode = response.StatusCode });
}

Expand Down
17 changes: 9 additions & 8 deletions src/Unleash/DefaultUnleash.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ public DefaultUnleash(UnleashSettings settings, bool overrideDefaultStrategies,

services = new UnleashServices(settings, EventConfig, strategyMap);

Logger.Info($"UNLEASH: Unleash instance number { currentInstanceNo } is initialized and configured with: {settings}");
Logger.Info(() => $"UNLEASH: Unleash instance number { currentInstanceNo } is initialized and configured with: {settings}");

if (currentInstanceNo >= ErrorOnInstanceCount)
{
Logger.Error($"UNLEASH: Unleash instance count for this process is now {currentInstanceNo}.");
Logger.Error("Ideally you should only need 1 instance of Unleash per app/process, we strongly recommend setting up Unleash as a singleton.");
Logger.Error(() => $"UNLEASH: Unleash instance count for this process is now {currentInstanceNo}.");
Logger.Error(() => "Ideally you should only need 1 instance of Unleash per app/process, we strongly recommend setting up Unleash as a singleton.");
}
}

Expand Down Expand Up @@ -143,7 +143,8 @@ private bool DetermineIsEnabledAndStrategy(
strategy = null;
if (featureToggle == null)
{
Logger.Warn($"UNLEASH: Feature flag {toggleName} not present, returning default setting: {defaultSetting}");
Logger.Warn(() => $"UNLEASH: Feature flag {toggleName} not present, returning default setting: {defaultSetting}");

return defaultSetting;
}

Expand Down Expand Up @@ -355,7 +356,7 @@ public void ConfigureEvents(Action<EventCallbackConfig> callback)
{
if (callback == null)
{
Logger.Error($"UNLEASH: Unleash->ConfigureEvents parameter callback is null");
Logger.Error(() => $"UNLEASH: Unleash->ConfigureEvents parameter callback is null");
return;
}

Expand All @@ -365,15 +366,15 @@ public void ConfigureEvents(Action<EventCallbackConfig> callback)
}
catch (Exception ex)
{
Logger.Error($"UNLEASH: Unleash->ConfigureEvents executing callback threw exception: {ex.Message}");
Logger.Error(() => $"UNLEASH: Unleash->ConfigureEvents executing callback threw exception: {ex.Message}");
}
}

private void EmitImpressionEvent(string type, UnleashContext context, bool enabled, string name, string variant = null)
{
if (EventConfig?.ImpressionEvent == null)
{
Logger.Error($"UNLEASH: Unleash->ImpressionData callback is null, unable to emit event");
Logger.Error(() => $"UNLEASH: Unleash->ImpressionData callback is null, unable to emit event");
return;
}

Expand All @@ -391,7 +392,7 @@ private void EmitImpressionEvent(string type, UnleashContext context, bool enabl
}
catch (Exception ex)
{
Logger.Error($"UNLEASH: Emitting impression event callback threw exception: {ex.Message}");
Logger.Error(() => $"UNLEASH: Emitting impression event callback threw exception: {ex.Message}");
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/Unleash/Internal/CachedFilesLoader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public CachedFilesResult EnsureExistsAndLoad()
}
catch (IOException ex)
{
Logger.ErrorException($"UNLEASH: Unhandled exception when writing to ETag file '{etagFile}'.", ex);
Logger.Error(() => $"UNLEASH: Unhandled exception when writing to ETag file '{etagFile}'.", ex);
eventConfig?.RaiseError(new ErrorEvent() { Error = ex, ErrorType = ErrorType.FileCache });
}
}
Expand All @@ -61,7 +61,7 @@ public CachedFilesResult EnsureExistsAndLoad()
}
catch (IOException ex)
{
Logger.ErrorException($"UNLEASH: Unhandled exception when reading from ETag file '{etagFile}'.", ex);
Logger.Error(() => $"UNLEASH: Unhandled exception when reading from ETag file '{etagFile}'.", ex);
eventConfig?.RaiseError(new ErrorEvent() { Error = ex, ErrorType = ErrorType.FileCache });
}
}
Expand All @@ -76,7 +76,7 @@ public CachedFilesResult EnsureExistsAndLoad()
}
catch (IOException ex)
{
Logger.ErrorException($"UNLEASH: Unhandled exception when writing to toggle file '{toggleFile}'.", ex);
Logger.Error(() => $"UNLEASH: Unhandled exception when writing to toggle file '{toggleFile}'.", ex);
eventConfig?.RaiseError(new ErrorEvent() { Error = ex, ErrorType = ErrorType.FileCache });
}
}
Expand All @@ -91,7 +91,7 @@ public CachedFilesResult EnsureExistsAndLoad()
}
catch (IOException ex)
{
Logger.ErrorException($"UNLEASH: Unhandled exception when reading from toggle file '{toggleFile}'.", ex);
Logger.Error(() => $"UNLEASH: Unhandled exception when reading from toggle file '{toggleFile}'.", ex);
eventConfig?.RaiseError(new ErrorEvent() { Error = ex, ErrorType = ErrorType.FileCache });
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Unleash/Internal/UnleashExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ internal static void SafeTimerChange(this Timer timer, dynamic dueTime, dynamic
{
// race condition with Dispose can cause trigger to be called when underlying
// timer is being disposed - and a change will fail in this case.
// see
// see
// https://msdn.microsoft.com/en-us/library/b97tkt95(v=vs.110).aspx#Anchor_2
if (disposeEnded)
{
Expand Down Expand Up @@ -98,7 +98,7 @@ internal static string GetLocalIpAddress()
}
catch (Exception exception)
{
Logger.Trace("UNLEASH: Failed to extract local ip address", exception);
Logger.Trace(() => "UNLEASH: Failed to extract local ip address", exception);
return "undefined";
}
}
Expand Down
Loading

0 comments on commit e86849d

Please sign in to comment.