+972 52-548-6969
Code quality gateways
I just merged a Pull Request from one of our guys. This is a pretty important piece of code, so it went through two rounds of code reviews before it actually hit the PR stage.
That was the point where the tests run (our test suite takes over an hour to run, so we run a limited test frequently, and leave the rest for later), and we got three failing tests. The funny thing was, it wasn’t a functional test that failed, it was the code quality gateways tests.
The RavenDB team has grown quite a lot, and we are hiring again, and it is easy to lose knowledge of the “unimportant” things. Stuff that doesn’t bite you in the ass right now, for example. But will most assuredly will bite you in the ass (hard) at a later point in time.
In this case, an exception class was introduced, but it didn’t have the proper serialization. Now, we don’t actually use AppDomains all that often (at all, really), but our users sometimes do, and getting a “your exception could not be serialized” makes for an annoying debug session. Another couple of tests related to missing asserts on new configuration values. We want to make sure that we have a new configuration value is actually connected, parsed and working. And we actually have a test that would fail if you aren’t checking all the configuration properties.
There are quite a few such tests, from making sure that we don’t have void async methods to ensuring that the tests don’t leak connections or resources (which could harm other tests and complicate root cause analysis).
We also started to make use of code analyzers, for things like keeping all complex log statements in conditionals (to save allocations) to validating all async calls are using ConfigureAwait(false).
Those aren’t big things, but getting them right, all the time, give us a really cool product, and a maintainable codebase.
Comments
I apologise with this comment being something of a tangent, but with respect to "keeping all complex log statements in conditionals (to save allocations)" what is your opinion of that, generally? Are you only doing that because you're a framework/service and you're optimising heavily, or would you do this generally?
I ask because I was once considering a logging API similar to: "logger.Log(Action<string>)", that would only evaluate the expression to be logged if the logging level was high enough. This means that log output that wouldn't pass the "log level" filter wouldn't even be generated (and some could be costly e.g. serialising a command-invoker-style Command object into the log output). A colleague of mine said he'd prefer all the code was executed all the time so that changing the config couldn't change the behaviour. These would be slightly unusual-yet-possible things like variables being closed-over etc.
Whoops I made an error in my comment (when do we get code compilation errors in blog comments?). The API I was proposing was "ILogger.Log(LogLevel, Func<string>)".
I'm curious, but what are you using to run the code quality tests? I'd be interested in seeing the list of code quality rules you have in place.
I also have used Mono.Cecil to write some architectural tests, which verify some concepts we have in our code base, e.g. naming conventions or layering guidelines. Especially if you have no physical barrier (e.g. assemblies) which hinder people to introduce unwanted dependencies, this is really helpful.
Neil, You might not be aware, but the lambda is actually allocated and passed to the Log() method, which might or might not invoke it, but the lambda is allocated, the variables are captured, etc.
We actually had a memory usage issue because of this. C# uses a single type for all captured variables inside a method. So if you have two lambdas, there is a type that is the union of all the values they capture. We had a long lived lambda that ended up silently holding a very big array because we had a log statement in this manner to report its size.
Log statements in high call sites will generate a lot of allocations in this manner, which is what I'm trying to avoid
Brian, Those are standard C# xunit tests. I'm usually using reflection for that to assert things, or using Roslyn analyzers
I am using static code analysis for this. Roslyn makes it easy to define custom rules.
When you use string.format() style in your logging calls you should pay no allocation overhead (at least if you're using nlog), because all formatting is done 'inside', only if loggin is enabled. So you've got only a function call + some if-check inside.
Okay that makes sense. So the only real option to a) avoid the allocations and b) avoid effort it takes to generate the log output, is to wrap the call to logging in a conditional of some sort. My "trick" of passing a function pointer doesn't really help, because any variables that might be used by the lambda are made ready anyway. So really for me it would need to be something like
"if (logger.LogLevelIncludes(LogLevel.Whatever) { logger.Log(LogLevel.Whatever, [build the log output] }"
Rafal, That isn't actually true. See the full details here: http://ayende.com/blog/172545/the-hidden-costs-of-allocations?key=08667b02cf924cb5bb3e583509653019
Neil, Yes, and there is a subtle issue with lambdas for logging that is there _even if they aren't being called_. See the full details here: http://ayende.com/blog/172545/the-hidden-costs-of-allocations?key=08667b02cf924cb5bb3e583509653019
Oren, it looks like the "have"word is missing here: "We want to make sure that we {have?} a new configuration value is actually connected".
Yaroslav , Thanks
Oren, long time no talk, have you played with NDepend since it supports code rule as C# LINQ queries? (v4.0, May 2012) For example the "Exceptions must be serializable" rule can be written this way:
---[[[ warnif count > 0 from t in Application.Types where t.IsExceptionClass && !t.HasAttribute ("System.SerializableAttribute") select t ]]]---
and the "Async method must not return void" can be written this way:
---[[[ warnif count > 0 from m in Application.Methods where m.IsAsync && m.ReturnType != null && m.ReturnType.FullName == "System.Void" select m ]]]---
The rule set can be run in VS while the dev is working, not more often than after each compilation, but way before (gated) checkin. See more LINQ rules here: http://www.ndepend.com/default-rules/webframe.html
Comment preview