Skip to content

Commit 17a13fd

Browse files
bart-vmwarebart-vmware
committed
Fix CA1873: Potentially expensive logging
1 parent 044c1f1 commit 17a13fd

9 files changed

Lines changed: 203 additions & 51 deletions

File tree

‎shared-package.props‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,6 @@
3333
<GeneratePackageOnBuild>True</GeneratePackageOnBuild>
3434
</PropertyGroup>
3535

36-
<PropertyGroup>
37-
<!--
38-
Temporary workaround for CA1873: Evaluation of this argument may be expensive and unnecessary if logging is disabled.
39-
Should be revisited as part of https://github.com/SteeltoeOSS/Steeltoe/issues/969.
40-
-->
41-
<NoWarn>$(NoWarn);CA1873</NoWarn>
42-
</PropertyGroup>
43-
4436
<PropertyGroup Condition="'$(CI)' != ''">
4537
<!--
4638
While deterministic builds are enabled by default in .NET SDK projects, there is an extra property, ContinuousIntegrationBuild,

‎src/Configuration/src/Abstractions/CompositeConfigurationProvider.cs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,7 @@ public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string?
5656
ArgumentNullException.ThrowIfNull(earlierKeys);
5757

5858
string[] earlierKeysArray = earlierKeys as string[] ?? earlierKeys.ToArray();
59-
60-
if (_logger.IsEnabled(LogLevel.Trace))
61-
{
62-
string earlierKeyNames = string.Join(", ", earlierKeysArray.Select(key => $"'{key}'"));
63-
LogGetChildKeys(GetType().Name, earlierKeyNames, parentPath);
64-
}
59+
ExpensiveLogGetChildKeys(earlierKeysArray, parentPath);
6560

6661
IConfiguration? section = parentPath == null ? ConfigurationRoot : ConfigurationRoot?.GetSection(parentPath);
6762

@@ -77,6 +72,15 @@ public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string?
7772
return keys;
7873
}
7974

75+
private void ExpensiveLogGetChildKeys(string[] earlierKeysArray, string? parentPath)
76+
{
77+
if (_logger.IsEnabled(LogLevel.Trace))
78+
{
79+
string earlierKeyNames = string.Join(", ", earlierKeysArray.Select(key => $"'{key}'"));
80+
LogGetChildKeys(GetType().Name, earlierKeyNames, parentPath);
81+
}
82+
}
83+
8084
public virtual bool TryGet(string key, out string? value)
8185
{
8286
ArgumentNullException.ThrowIfNull(key);

‎src/Configuration/src/ConfigServer/ConfigServerConfigurationProvider.cs‎

Lines changed: 67 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,7 @@ public override void Load()
403403
// Update configuration Data dictionary with any results
404404
if (env != null)
405405
{
406-
LogEnvironmentLocated(env.Name, string.Join(", ", env.Profiles.Select(p => $"'{p}'")), env.Label, env.Version, env.State);
406+
ExpensiveLogEnvironmentLocated(env);
407407

408408
if (updateDictionary)
409409
{
@@ -451,6 +451,15 @@ public override void Load()
451451
throw new ConfigServerException("Failed fetching remote configuration from server(s).", error);
452452
}
453453

454+
private void ExpensiveLogEnvironmentLocated(ConfigEnvironment environment)
455+
{
456+
if (_logger.IsEnabled(LogLevel.Debug))
457+
{
458+
string profiles = string.Join(", ", environment.Profiles.Select(profile => $"'{profile}'"));
459+
LogEnvironmentLocated(environment.Name, profiles, environment.Label, environment.Version, environment.State);
460+
}
461+
}
462+
454463
internal void ApplyLastDiscoveryLookupResultToClientOptions(ConfigServerClientOptions optionsSnapshot)
455464
{
456465
DiscoveryLookupResult? lastResult = _lastDiscoveryLookupResult;
@@ -601,7 +610,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
601610

602611
if (requestUri.TryGetUsernamePassword(out string? username, out string? password) && password.Length > 0)
603612
{
604-
LogAddingCredentials(requestUri.ToMaskedString());
613+
ExpensiveLogAddingCredentials(requestUri);
605614

606615
requestMessage.Headers.Authorization =
607616
new AuthenticationHeaderValue("Basic", Convert.ToBase64String(Encoding.ASCII.GetBytes($"{username}:{password}")));
@@ -616,7 +625,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
616625
string accessToken =
617626
await httpClient.GetAccessTokenAsync(accessTokenUri, optionsSnapshot.ClientId, optionsSnapshot.ClientSecret, cancellationToken);
618627

619-
LogAccessTokenFetched(accessTokenUri.ToMaskedString());
628+
ExpensiveLogAccessTokenFetched(accessTokenUri);
620629
requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
621630
}
622631
}
@@ -629,6 +638,24 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
629638
return requestMessage;
630639
}
631640

641+
private void ExpensiveLogAddingCredentials(Uri requestUri)
642+
{
643+
if (_logger.IsEnabled(LogLevel.Debug))
644+
{
645+
string uri = requestUri.ToMaskedString();
646+
LogAddingCredentials(uri);
647+
}
648+
}
649+
650+
private void ExpensiveLogAccessTokenFetched(Uri accessTokenUri)
651+
{
652+
if (_logger.IsEnabled(LogLevel.Debug))
653+
{
654+
string uri = accessTokenUri.ToMaskedString();
655+
LogAccessTokenFetched(uri);
656+
}
657+
}
658+
632659
internal async Task<ConfigEnvironment?> RemoteLoadAsync(ConfigServerClientOptions optionsSnapshot, List<Uri> requestUris, string? label,
633660
CancellationToken cancellationToken)
634661
{
@@ -646,7 +673,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
646673
// Make Config Server URI from settings
647674
Uri uri = BuildConfigServerUri(optionsSnapshot, requestUri, label);
648675

649-
LogTryingToConnect(uri.ToMaskedString());
676+
ExpensiveLogTryingToConnect(uri);
650677
HttpRequestMessage request;
651678

652679
try
@@ -672,7 +699,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
672699
LogSendingHttpRequest();
673700
using HttpResponseMessage response = await httpClient.SendAsync(request, cancellationToken);
674701

675-
LogConfigServerReturnedStatus(response.StatusCode, uri.ToMaskedString());
702+
ExpensiveLogConfigServerReturnedStatus(uri, response.StatusCode);
676703

677704
if (response.StatusCode != HttpStatusCode.OK)
678705
{
@@ -715,6 +742,24 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
715742
return null;
716743
}
717744

745+
private void ExpensiveLogTryingToConnect(Uri requestUri)
746+
{
747+
if (_logger.IsEnabled(LogLevel.Debug))
748+
{
749+
string uri = requestUri.ToMaskedString();
750+
LogTryingToConnect(uri);
751+
}
752+
}
753+
754+
private void ExpensiveLogConfigServerReturnedStatus(Uri requestUri, HttpStatusCode responseStatusCode)
755+
{
756+
if (_logger.IsEnabled(LogLevel.Debug))
757+
{
758+
string uri = requestUri.ToMaskedString();
759+
LogConfigServerReturnedStatus(responseStatusCode, uri);
760+
}
761+
}
762+
718763
/// <summary>
719764
/// Creates the Uri that will be used in accessing the Configuration Server.
720765
/// </summary>
@@ -827,7 +872,7 @@ internal async Task RefreshVaultTokenAsync(ConfigServerClientOptions optionsSnap
827872
Uri uri = BuildVaultRenewUri(optionsSnapshot);
828873
HttpRequestMessage message = await GetVaultRenewRequestMessageAsync(optionsSnapshot, uri, cancellationToken);
829874

830-
LogRenewingVaultToken(obscuredToken, optionsSnapshot.TokenTtl, uri.ToMaskedString());
875+
ExpensiveLogRenewingVaultToken(obscuredToken, optionsSnapshot.TokenTtl, uri);
831876
using HttpResponseMessage response = await httpClient.SendAsync(message, cancellationToken);
832877

833878
if (response.StatusCode != HttpStatusCode.OK)
@@ -853,6 +898,15 @@ private static Uri BuildVaultRenewUri(ConfigServerClientOptions optionsSnapshot)
853898
return new Uri(baseUri + VaultRenewPath, UriKind.RelativeOrAbsolute);
854899
}
855900

901+
private void ExpensiveLogRenewingVaultToken(string obscuredToken, int tokenTtl, Uri requestUri)
902+
{
903+
if (_logger.IsEnabled(LogLevel.Debug))
904+
{
905+
string uri = requestUri.ToMaskedString();
906+
LogRenewingVaultToken(obscuredToken, tokenTtl, uri);
907+
}
908+
}
909+
856910
private async Task<HttpRequestMessage> GetVaultRenewRequestMessageAsync(ConfigServerClientOptions optionsSnapshot, Uri requestUri,
857911
CancellationToken cancellationToken)
858912
{
@@ -867,7 +921,7 @@ private async Task<HttpRequestMessage> GetVaultRenewRequestMessageAsync(ConfigSe
867921
string accessToken =
868922
await httpClient.GetAccessTokenAsync(accessTokenUri, optionsSnapshot.ClientId, optionsSnapshot.ClientSecret, cancellationToken);
869923

870-
LogAccessTokenFetched(accessTokenUri.ToMaskedString());
924+
ExpensiveLogAccessTokenFetched(accessTokenUri);
871925
requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
872926
}
873927

@@ -1019,7 +1073,7 @@ private void ShutdownTimers()
10191073
[LoggerMessage(Level = LogLevel.Debug, Message = "Multiple Config Server uris listed.")]
10201074
private partial void LogMultipleConfigServerUris();
10211075

1022-
[LoggerMessage(Level = LogLevel.Debug,
1076+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true,
10231077
Message = "Located environment with name {Name}, profiles {Profiles}, label {Label}, version {Version} and state {State}.")]
10241078
private partial void LogEnvironmentLocated(string? name, string profiles, string? label, string? version, string? state);
10251079

@@ -1032,10 +1086,10 @@ private void ShutdownTimers()
10321086
[LoggerMessage(Level = LogLevel.Warning, Message = "Failed fetching remote configuration from server(s).")]
10331087
private partial void LogFetchingRemoteConfigurationFailed(Exception? error);
10341088

1035-
[LoggerMessage(Level = LogLevel.Debug, Message = "Adding credentials from '{RequestUri}' to Authorization header.")]
1089+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true, Message = "Adding credentials from '{RequestUri}' to Authorization header.")]
10361090
private partial void LogAddingCredentials(string requestUri);
10371091

1038-
[LoggerMessage(Level = LogLevel.Debug, Message = "Fetched access token from {AccessTokenUri}.")]
1092+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true, Message = "Fetched access token from {AccessTokenUri}.")]
10391093
private partial void LogAccessTokenFetched(string accessTokenUri);
10401094

10411095
[LoggerMessage(Level = LogLevel.Warning, Message = "Failed to fetch access token from '{AccessTokenUri}'.")]
@@ -1044,7 +1098,7 @@ private void ShutdownTimers()
10441098
[LoggerMessage(Level = LogLevel.Trace, Message = "Entered {Method}.")]
10451099
private partial void LogRemoteLoadEntered(string method);
10461100

1047-
[LoggerMessage(Level = LogLevel.Debug, Message = "Trying to connect to Config Server at {RequestUri}.")]
1101+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true, Message = "Trying to connect to Config Server at {RequestUri}.")]
10481102
private partial void LogTryingToConnect(string requestUri);
10491103

10501104
[LoggerMessage(Level = LogLevel.Trace, Message = "Building HTTP request message.")]
@@ -1053,7 +1107,7 @@ private void ShutdownTimers()
10531107
[LoggerMessage(Level = LogLevel.Trace, Message = "Sending HTTP request.")]
10541108
private partial void LogSendingHttpRequest();
10551109

1056-
[LoggerMessage(Level = LogLevel.Debug, Message = "Config Server returned status {StatusCode} for path {RequestUri}.")]
1110+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true, Message = "Config Server returned status {StatusCode} for path {RequestUri}.")]
10571111
private partial void LogConfigServerReturnedStatus(HttpStatusCode statusCode, string requestUri);
10581112

10591113
[LoggerMessage(Level = LogLevel.Trace, Message = "Parsing JSON response.")]
@@ -1065,7 +1119,7 @@ private void ShutdownTimers()
10651119
[LoggerMessage(Level = LogLevel.Error, Message = "Config Server exception for property {Key} of type {Type}.")]
10661120
private partial void LogConfigServerPropertyException(Exception exception, string key, Type type);
10671121

1068-
[LoggerMessage(Level = LogLevel.Debug, Message = "Renewing Vault token {Token} for {Ttl} milliseconds at Uri {Uri}.")]
1122+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true, Message = "Renewing Vault token {Token} for {Ttl} milliseconds at Uri {Uri}.")]
10691123
private partial void LogRenewingVaultToken(string token, int ttl, string uri);
10701124

10711125
[LoggerMessage(Level = LogLevel.Warning, Message = "Renewing Vault token {Token} returned status {Status}.")]

‎src/Configuration/src/ConfigServer/ConfigServerHealthContributor.cs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,19 @@ internal void UpdateHealth(HealthCheckResult health, IList<PropertySource> sourc
7979
names.Add(source.Name);
8080
}
8181

82-
LogReturningPropertySources(string.Join(", ", names));
82+
ExpensiveLogReturningPropertySources(names);
8383
health.Details.Add("propertySources", names);
8484
}
8585

86+
private void ExpensiveLogReturningPropertySources(List<string?> names)
87+
{
88+
if (_logger.IsEnabled(LogLevel.Debug))
89+
{
90+
string propertySources = string.Join(", ", names);
91+
LogReturningPropertySources(propertySources);
92+
}
93+
}
94+
8695
internal async Task<IList<PropertySource>?> GetPropertySourcesAsync(ConfigServerConfigurationProvider provider, ConfigServerClientOptions optionsSnapshot,
8796
CancellationToken cancellationToken)
8897
{
@@ -133,7 +142,7 @@ internal bool IsCacheStale(long accessTime, ConfigServerClientOptions optionsSna
133142
[LoggerMessage(Level = LogLevel.Debug, Message = "Config Server health check returning UP.")]
134143
private partial void LogHealthCheckReturningUp();
135144

136-
[LoggerMessage(Level = LogLevel.Debug, Message = "Returning property sources: {PropertySources}.")]
145+
[LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true, Message = "Returning property sources: {PropertySources}.")]
137146
private partial void LogReturningPropertySources(string propertySources);
138147

139148
[LoggerMessage(Level = LogLevel.Debug, Message = "Cache stale, fetching config server health.")]

0 commit comments

Comments
 (0)