Skip to content

Commit

Permalink
Work on the logging generator. (#4840)
Browse files Browse the repository at this point in the history 
- The generator now produces a warning when asked to log an object
which doesn't implement ToString(), IConvertible, or IFormattable.
Fixed #4835.

- Added support for the Transitive property in the LogProperties attribute. When
set to true, this causes automatic transitive traversal of a complex object, instead
of requiring manual annotations of individual properties. Fixes #4738.

- Introduce the [TagName] attribute to make it possible to control the tag name
used when logging a parameter or property. Fixes #4576.

- Fixed some situations where unnatural errors were produced as a
result of a prior error. The dummy follow-on errors are now avoided.

- Fixed handling of cases where parameters or properties were of type of the
non-generic IEnumerable. The specific type wasn't being recorgnized and treated
as an enumerable.

Co-authored-by: Martin Taillefer <[email protected]>
  • Loading branch information
@geeknoid
geeknoid and Martin Taillefer authored Dec 27, 2023
1 parent 40fdaf9 commit f4315cd
Show file tree
Hide file tree
Showing 34 changed files with 566 additions and 254 deletions.
54 changes: 1 addition & 53 deletions docs/list-of-diagnostics.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,56 +8,6 @@
| `CTXOPTGEN002` | The options context type does not have usable properties |
| `CTXOPTGEN003` | The options context cannot be a ref-like type |


# Design

| Diagnostic ID | Description |
| :---------------- | :---------- |
| `AUTOCLIENTGEN001` | API client interfaces must not be nested types |
| `AUTOCLIENTGEN002` | REST API client does not have methods defined |
| `AUTOCLIENTGEN003` | An API method must not contain more than one REST method attribute |
| `AUTOCLIENTGEN004` | Invalid API method return type |
| `AUTOCLIENTGEN005` | API methods can't be generic |
| `AUTOCLIENTGEN006` | The current HTTP method does not support the body tag |
| `AUTOCLIENTGEN007` | API methods must not be static |
| `AUTOCLIENTGEN008` | HTTP method missing |
| `AUTOCLIENTGEN009` | The API interface cannot be generic |
| `AUTOCLIENTGEN010` | Invalid API interface name |
| `AUTOCLIENTGEN011` | Duplicate body attribute |
| `AUTOCLIENTGEN012` | URL parameter missing from path |
| `AUTOCLIENTGEN013` | REST API method has more than one cancellation token |
| `AUTOCLIENTGEN014` | Missing CancellationToken from REST API method |
| `AUTOCLIENTGEN015` | API method path should not contain query |
| `AUTOCLIENTGEN016` | A REST API method's request name must be unique |
| `AUTOCLIENTGEN017` | Invalid HttpClient name |
| `AUTOCLIENTGEN018` | Invalid dependency name |
| `AUTOCLIENTGEN019` | Invalid header name |
| `AUTOCLIENTGEN020` | Invalid header value |
| `AUTOCLIENTGEN021` | Invalid REST method path |
| `AUTOCLIENTGEN022` | Invalid request name |


# ExtraAnalyzers

| Diagnostic ID | Category | Description |
| :---------------- | :---------- | :---------- |
| `EA0000` | Performance | Use source generated logging methods for improved performance |
| `EA0001` | Performance | Perform message formatting in the body of the logging method |
| `EA0002` | Reliability | Use 'System.TimeProvider' to make the code easier to test |
| `EA0003` | Performance | Use the character-based overloads of 'String.StartsWith' or 'String.EndsWith' |
| `EA0004` | Performance | Make types declared in an executable internal |
| `EA0005` | Performance | Consider using an array instead of a collection |
| `EA0006` | Performance | Replace uses of 'Enum.GetName' and 'Enum.ToString' for improved performance |
| `EA0007` | Performance | Use 'System.ValueTuple' instead of 'System.Tuple' for improved performance |
| `EA0008` | Performance | Use generic collections instead of legacy collections for improved performance |
| `EA0009` | Performance | Use 'System.MemoryExtensions.Split' for improved performance |
| `EA0010` | Correctness | Fire-and-forget async call inside a 'using' block |
| `EA0011` | Performance | Consider removing unnecessary conditional access operator (?) |
| `EA0012` | Performance | Consider removing unnecessary null coalescing assignment (??=) |
| `EA0013` | Performance | Consider removing unnecessary null coalescing operator (??) |
| `EA0014` | Resilience | The async method doesn't support cancellation |


# Experiments

As new functionality is introduced to this repo, new in-development APIs are marked as being experimental. Experimental APIs offer no
Expand All @@ -72,14 +22,12 @@ If you use experimental APIs, you will get one of the diagnostic shown below. Th
using such an API so that you can avoid accidentally depending on experimental features. You may suppress these diagnostics
if desired.


| Diagnostic ID | Description |
| :---------------- | :---------- |
| `EXTEXP0001` | Resilience experiments |
| `EXTEXP0002` | Compliance experiments |
| `EXTEXP0003` | Telemetry experiments |
| `EXTEXP0004` | TimeProvider experiments |
| `EXTEXP0005` | AutoClient experiments |
| `EXTEXP0006` | AsyncState experiments |
| `EXTEXP0007` | Health check experiments |
| `EXTEXP0008` | Resource monitoring experiments |
Expand Down Expand Up @@ -134,7 +82,7 @@ if desired.
| `LOGGEN033` | Method parameter can't be used with a tag provider |
| `LOGGEN034` | Attribute can't be used in this context |
| `LOGGEN035` | The logging method parameter leaks sensitive data |

| `LOGGEN036` | A value being logged doesn't have an effective way to be converted into a string |

# Metrics

Expand Down
100 changes: 52 additions & 48 deletions src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ private void GenLogMethod(LoggingMethod lm)
OutLn();
}

var stateName = PickUniqueName("state", lm.Parameters.Select(p => p.Name));
var stateName = PickUniqueName("state", lm.Parameters.Select(p => p.ParameterName));

OutLn($"var {stateName} = {LoggerMessageHelperType}.ThreadLocalState;");
GenPropertyLoads(lm, stateName, out int numReservedUnclassifiedTags, out int numReservedClassifiedTags);
GenTagWrites(lm, stateName, out int numReservedUnclassifiedTags, out int numReservedClassifiedTags);

OutLn();
OutLn($"{logger}.Log(");
Expand All @@ -97,7 +97,7 @@ private void GenLogMethod(LoggingMethod lm)
OutLn($"{stateName},");
OutLn($"{exceptionArg},");

var lambdaStateName = PickUniqueName("s", lm.TemplateToParameterName.Select(kvp => kvp.Key));
var lambdaStateName = PickUniqueName("s", lm.Templates);

OutLn($"[{GeneratorUtilities.GeneratedCodeAttribute}] static string ({lambdaStateName}, {exceptionLambdaName}) =>");
OutOpenBrace();
Expand Down Expand Up @@ -158,7 +158,7 @@ static bool ShouldStringifyProperty(LoggingProperty p)

if (p.ImplementsISpanFormattable)
{
// pass object as it, it will be formatted directly into the output buffer
// pass object as is, it will be formatted directly into the output buffer
return false;
}

Expand Down Expand Up @@ -211,11 +211,11 @@ static string ConvertPropertyToString(LoggingProperty lp, string arg)
{
if (p.IsException)
{
exceptionArg = p.Name;
exceptionArg = p.ParameterName;

if (p.UsedAsTemplate)
{
exceptionLambdaArg = lm.GetParameterNameInTemplate(p);
exceptionLambdaArg = lm.GetTemplatesForParameter(p)[0];
}

break;
Expand All @@ -235,11 +235,11 @@ static bool NeedsASlot(LoggingMethodParameter p)
return p.IsNormalParameter && !p.HasTagProvider && !p.HasProperties;
}

void GenPropertyLoads(LoggingMethod lm, string stateName, out int numReservedUnclassifiedTags, out int numReservedClassifiedTags)
void GenTagWrites(LoggingMethod lm, string stateName, out int numReservedUnclassifiedTags, out int numReservedClassifiedTags)
{
int numUnclassifiedTags = 0;
int numClassifiedTags = 0;
var tmpVarName = PickUniqueName("tmp", lm.Parameters.Select(p => p.Name));
var tmpVarName = PickUniqueName("tmp", lm.Parameters.Select(p => p.ParameterName));

foreach (var p in lm.Parameters)
{
Expand Down Expand Up @@ -288,20 +288,20 @@ void GenPropertyLoads(LoggingMethod lm, string stateName, out int numReservedUnc
{
if (NeedsASlot(p) && !p.HasDataClassification)
{
var key = $"\"{lm.GetParameterNameInTemplate(p)}\"";
var key = $"\"{p.TagName}\"";
string value;

if (p.IsEnumerable)
{
value = p.PotentiallyNull
? $"{p.NameWithAt} != null ? {LoggerMessageHelperType}.Stringify({p.NameWithAt}) : null"
: $"{LoggerMessageHelperType}.Stringify({p.NameWithAt})";
? $"{p.ParameterNameWithAt} != null ? {LoggerMessageHelperType}.Stringify({p.ParameterNameWithAt}) : null"
: $"{LoggerMessageHelperType}.Stringify({p.ParameterNameWithAt})";
}
else
{
value = ShouldStringifyParameter(p)
? ConvertParameterToString(p, p.NameWithAt)
: p.NameWithAt;
? ConvertParameterToString(p, p.ParameterNameWithAt)
: p.ParameterNameWithAt;
}

OutLn($"{stateName}.TagArray[{--count}] = new({key}, {value});");
Expand Down Expand Up @@ -348,12 +348,12 @@ void GenPropertyLoads(LoggingMethod lm, string stateName, out int numReservedUnc
{
if (NeedsASlot(p) && p.HasDataClassification)
{
var key = $"\"{lm.GetParameterNameInTemplate(p)}\"";
var key = $"\"{p.TagName}\"";
var classification = MakeClassificationValue(p.ClassificationAttributeTypes);

var value = ShouldStringifyParameter(p)
? ConvertParameterToString(p, p.NameWithAt)
: p.NameWithAt;
? ConvertParameterToString(p, p.ParameterNameWithAt)
: p.ParameterNameWithAt;

OutLn($"{stateName}.ClassifiedTagArray[{--count}] = new({key}, {value}, {classification});");
}
Expand Down Expand Up @@ -469,10 +469,10 @@ void GenPropertyLoads(LoggingMethod lm, string stateName, out int numReservedUnc
}
else
{
OutLn($"{stateName}.TagNamePrefix = nameof({p.NameWithAt});");
OutLn($"{stateName}.TagNamePrefix = nameof({p.ParameterNameWithAt});");
}

OutLn($"{p.TagProvider!.ContainingType}.{p.TagProvider.MethodName}({stateName}, {p.NameWithAt});");
OutLn($"{p.TagProvider!.ContainingType}.{p.TagProvider.MethodName}({stateName}, {p.ParameterNameWithAt});");
}
}
}
Expand All @@ -488,20 +488,22 @@ bool GenVariableAssignments(LoggingMethod lm, string lambdaStateName, int numRes
{
if (p.UsedAsTemplate)
{
var key = lm.GetParameterNameInTemplate(p);

var atSign = p.NeedsAtSign ? "@" : string.Empty;
if (p.PotentiallyNull)
{
const string Null = "\"(null)\"";
OutLn($"var {atSign}{key} = {lambdaStateName}.TagArray[{index}].Value ?? {Null};");
}
else
var templates = lm.GetTemplatesForParameter(p);
foreach (var t in templates)
{
OutLn($"var {atSign}{key} = {lambdaStateName}.TagArray[{index}].Value;");
}
var atSign = p.NeedsAtSign ? "@" : string.Empty;
if (p.PotentiallyNull)
{
const string Null = "\"(null)\"";
OutLn($"var {atSign}{t} = {lambdaStateName}.TagArray[{index}].Value ?? {Null};");
}
else
{
OutLn($"var {atSign}{t} = {lambdaStateName}.TagArray[{index}].Value;");
}

generatedAssignments = true;
generatedAssignments = true;
}
}

index--;
Expand All @@ -515,20 +517,22 @@ bool GenVariableAssignments(LoggingMethod lm, string lambdaStateName, int numRes
{
if (p.UsedAsTemplate)
{
var key = lm.GetParameterNameInTemplate(p);

var atSign = p.NeedsAtSign ? "@" : string.Empty;
if (p.PotentiallyNull)
{
const string Null = "\"(null)\"";
OutLn($"var {atSign}{key} = {lambdaStateName}.RedactedTagArray[{index}].Value ?? {Null};");
}
else
var templates = lm.GetTemplatesForParameter(p);
foreach (var t in templates)
{
OutLn($"var {atSign}{key} = {lambdaStateName}.RedactedTagArray[{index}].Value;");
}
var atSign = p.NeedsAtSign ? "@" : string.Empty;
if (p.PotentiallyNull)
{
const string Null = "\"(null)\"";
OutLn($"var {atSign}{t} = {lambdaStateName}.RedactedTagArray[{index}].Value ?? {Null};");
}
else
{
OutLn($"var {atSign}{t} = {lambdaStateName}.RedactedTagArray[{index}].Value;");
}

generatedAssignments = true;
generatedAssignments = true;
}
}

index--;
Expand All @@ -547,7 +551,7 @@ bool GenVariableAssignments(LoggingMethod lm, string lambdaStateName, int numRes
{
if (p.IsLogger)
{
logger = p.Name;
logger = p.ParameterName;
isNullable = p.IsNullable;
break;
}
Expand Down Expand Up @@ -597,7 +601,7 @@ private string AddAtSymbolsToTemplates(string template, IEnumerable<LoggingMetho
_ = stringBuilder.Append(template);
}

_ = stringBuilder.Replace(item.Name, item.NameWithAt);
_ = stringBuilder.Replace(item.ParameterName, item.ParameterNameWithAt);
}

var result = stringBuilder is null
Expand All @@ -618,10 +622,10 @@ private void GenParameters(LoggingMethod lm)
{
if (p.Qualifier != null)
{
return $"{p.Qualifier} {p.Type} {p.NameWithAt}";
return $"{p.Qualifier} {p.Type} {p.ParameterNameWithAt}";
}

return $"{p.Type} {p.NameWithAt}";
return $"{p.Type} {p.ParameterNameWithAt}";
}));
}

Expand All @@ -647,12 +651,12 @@ private string PropertyChainToString(
}

_ = localStringBuilder
.Append(needAts ? property.NameWithAt : property.Name)
.Append(needAts ? property.PropertyNameWithAt : property.PropertyName)
.Append(property.PotentiallyNull ? separator : adjustedNonNullSeparator);
}

// Last item:
_ = localStringBuilder.Append(needAts ? leafProperty.NameWithAt : leafProperty.Name);
_ = localStringBuilder.Append(needAts ? leafProperty.PropertyNameWithAt : leafProperty.PropertyName);

return localStringBuilder.ToString();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Utils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ internal static string GetLoggerMethodLogLevel(LoggingMethod lm)
{
if (p.IsLogLevel)
{
level = p.Name;
level = p.ParameterName;
break;
}
}
Expand Down
36 changes: 31 additions & 5 deletions src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;

namespace Microsoft.Gen.Logging.Model;

Expand All @@ -14,7 +15,7 @@ namespace Microsoft.Gen.Logging.Model;
internal sealed class LoggingMethod
{
public readonly List<LoggingMethodParameter> Parameters = [];
public readonly Dictionary<string, string> TemplateToParameterName = new(StringComparer.OrdinalIgnoreCase);
public readonly List<string> Templates = [];
public string Name = string.Empty;
public string Message = string.Empty;
public int? Level;
Expand All @@ -28,8 +29,33 @@ internal sealed class LoggingMethod
public bool LoggerMemberNullable;
public bool HasXmlDocumentation;

public string GetParameterNameInTemplate(LoggingMethodParameter parameter)
=> TemplateToParameterName.TryGetValue(parameter.Name, out var value)
? value
: parameter.Name;
public LoggingMethodParameter? GetParameterForTemplate(string templateName)
{
foreach (var p in Parameters)
{
if (templateName.Equals(p.ParameterName, StringComparison.OrdinalIgnoreCase))
{
return p;
}
}

return null;
}

public List<string> GetTemplatesForParameter(LoggingMethodParameter lp)
=> GetTemplatesForParameter(lp.ParameterName);

public List<string> GetTemplatesForParameter(string parameterName)
{
HashSet<string> templates = [];
foreach (var t in Templates)
{
if (parameterName.Equals(t, StringComparison.OrdinalIgnoreCase))
{
_ = templates.Add(t);
}
}

return templates.ToList();
}
}
Loading

0 comments on commit f4315cd

Please sign in to comment.