8000 New Rule: Consistent Naming for Logged Types · Issue #17 · Suchiman/SerilogAnalyzer · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

New Rule: Consistent Naming for Logged Types #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its m 8000 aintainers and the community.

Sign up for GitHub

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

Jump to bottom
Open
normanhh3 opened this issue Mar 30, 2017 · 7 comments
Open

New Rule: Consistent Naming for Logged Types #17

normanhh3 opened this issue Mar 30, 2017 · 7 comments

Comments

@normanhh3
Copy link

In order to get the most value out of the log entries, property names need to be consistent for object types that are logged.

For example, logging a type User as user, User and uSer all make it MUCH harder to filter the log entries.

My proposal is that a rule ensure that logged property names are based on the type that is being logged by default.

It would be good to include a SerilogPropertyNameToTypeMapping.json that would allow for overriding the "standard" mapping from a type to a property name.

Does this sound like something that fits within the vision of this tool?
@Suchiman
Copy link
Owner

Consistent naming is something that bothered me for some while now.
We are, for example, only rarely using destructuring on complex types which makes this idea unsuitable for many usecases (that would only be int or string).
Nonetheless, this feature seems plausible and easy to implement so i'll try to make it available optionally on non primitive types.

@nblumhardt
Copy link
Contributor

This could be a bit problematic for things like:

Account from; to;
Log.Information("Transferring {Amount} from {From} to {To}", 100, from, to);

Seems like the hit rate for this rule might be patchy.

@normanhh3
Copy link
Author
normanhh3 commented Mar 30, 2017 via email

@kevygreen
Copy link

I think this type of feature would be really useful! This would be a good option for all Properties to make sure they are the same type.

I've been planning out an integration with ElasticSearch and Kibana, the big thing would be making sure that Properties with the same name have the same type so they can be properly indexed. E.g. If it's a number then always a number, same for date, same for string, etc. This would also apply for complex types being serialised.

A second helper could be that if you are logging a reference type, that it force (or suggest) using a particular name to keep things consistent. The level, e.g. suggest, warn, force could be per type.

You could configure the types in a JSON doc as such:
{
TypesToNames:{
:{ Name:, Level:<Suggest,Warn,Force> }
...
},
NamesToType:{
:{ Type:, Level:<Suggest,Warn,Force> }
...
}
}

You would assume a TypesToNames entry would also be the equivalent of adding it to the NamesToType list. This could be done in code to avoid duplicates in the file.

If the analyzer doesn't find the keyword in the list it could offer an auto-complete to add to the JSON document.

I'd be willing to help with this, but not 100% sure where to start.

@Suchiman
Copy link
Owner

@kevygreen You could start around in this region for the analysis part:

SerilogAnalyzer/SerilogAnalyzer/SerilogAnalyzer/DiagnosticAnalyzer.cs

Lines 170 to 239 in b78fb7f

var messageTemplateDiagnostics = AnalyzingMessageTemplateParser.Analyze(messageTemplate);
foreach (var templateDiagnostic in messageTemplateDiagnostics)
{
if (templateDiagnostic is PropertyToken property)
{
properties.Add(property);
continue;
}
if (templateDiagnostic is MessageTemplateDiagnostic diagnostic)
{
hasErrors = true;
ReportDiagnostic(ref context, ref literalSpan, stringText, exactPositions, TemplateRule, diagnostic);
}
}
var messageTemplateArgumentIndex = invocationArguments.IndexOf(argument);
arguments = invocationArguments.Skip(messageTemplateArgumentIndex + 1).Select(x =>
{
var location = x.GetLocation().SourceSpan;
return new SourceArgument { Argument = x, StartIndex = location. 8000 Start, Length = location.Length };
}).ToList();
break;
}
}
// do properties match up?
if (!hasErrors && literalSpan != default(TextSpan) && (arguments.Count > 0 || properties.Count > 0))
{
var diagnostics = PropertyBindingAnalyzer.AnalyzeProperties(properties, arguments);
foreach (var diagnostic in diagnostics)
{
ReportDiagnostic(ref context, ref literalSpan, stringText, exactPositions, PropertyBindingRule, diagnostic);
}
// check that all anonymous objects have destructuring hints in the message template
if (arguments.Count == properties.Count)
{
for (int i = 0; i < arguments.Count; i++)
{
var argument = arguments[i];
var argumentInfo = context.SemanticModel.GetTypeInfo(argument.Argument.Expression, context.CancellationToken);
if (argumentInfo.Type?.IsAnonymousType ?? false)
{
var property = properties[i];
if (!property.RawText.StartsWith("{@", StringComparison.Ordinal))
{
ReportDiagnostic(ref context, ref literalSpan, stringText, exactPositions, DestructureAnonymousObjectsRule, new MessageTemplateDiagnostic(property.StartIndex, property.Length, property.PropertyName));
}
}
}
}
// are there duplicate property names?
var usedNames = new HashSet<string>();
foreach (var property in properties)
{
if (!property.IsPositional && !usedNames.Add(property.PropertyName))
{
ReportDiagnostic(ref context, ref literalSpan, stringText, exactPositions, UniquePropertyNameRule, new MessageTemplateDiagnostic(property.StartIndex, property.Length, property.PropertyName));
}
var firstCharacter = property.PropertyName[0];
if (!Char.IsDigit(firstCharacter) && !Char.IsUpper(firstCharacter))
{
ReportDiagnostic(ref context, ref literalSpan, stringText, exactPositions, PascalPropertyNameRule, new MessageTemplateDiagnostic(property.StartIndex, property.Length, property.PropertyName));
}
}
}

The configuration bit is tricky, roslyn has nothing to offer for this, but i've been thinking that if you manage to get the file path of the file you're analyzing (perhaps from the SyntaxNodeAnalysisContext.Compilation), then look for .seriloganalyzer (similiar to .editorconfig) in the current directory and if not, check the parent directory and so on. I just hope this doesn't affect performance too bad if you do this for every logger statement you encounter in a document.

@normanhh3
Copy link
Author

@Suchiman is there any mechanism for maintaining state across files being analyzed or do the analyzers only work one file at a time, in process isolation?

If not, a global static might be one way to improve performance and cache the config?

@Suchiman
Copy link
Owner
Suchiman commented Jan 24, 2019 •
edited
Loading

@normanhh3 there's no builtin analyzer mechanism for maintaining state. Depends on what sort of analyzer you write, usually they operate on one file or one syntax location though and they might be executed out of process (VS) or inside the csc.exe. As long as it's running inside VS, you can MEF Import visual studio services into the analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nblumhardt @normanhh3 @kevygreen @Suchiman
0