Thursday, July 23, 2009

Measuring Your Engineers

To say that I am managing an Agile team is not entirely accurate, we are just trying to be agile. Anyway, skipping the story, the team's velocity increased more than 800% for the last 2 iterations, mainly due to management pressure. Put aside other reasons such as requirement gathering, design, headcount, etc.

1. Engineers are most probably working within their comfort zone.

For the past 3 months, I had passed/failed/extended the probation period of the engineers. During the session, they discussed with us their strength, weaknesses and areas for improvement.

2. Most engineers highlighted that they're lacking some knowledge to perform better.

So, last weekend, after having a discussion with the management, I was given a task - create KPIs to give engineers a direction as well as to provide management a way to measure their performance. So there are two things - KPIs and Performance Review. The first thing that got into my mind was - how did my ex-company do it?

In the support team that I worked in, we used a ladder-based system where there were 4 skill levels defined with skills to achieve. We would also set KPIs such as "resolving 30 support cases with customer satisfaction above 75%" or "to be SCJP certified". The progress, results and data would be presented to the management during our performance review. Trust me, people seldom stepped out the room wyith a happy face because the management loved to tell you where you did not do good enough.

I was thinking of the ladder since then.

I then looked into some blogs and Stack Overflow questions, such as this (Joel), this, this (Steve Yegge), this and this, which in general send the same message:

3. Management focuses too much on measuring the performance than improving the products and getting things done.

Something quite similar to this will be - timesheet and the reason I abolished it because it is irrelevant with the team velocity and business values. What does it mean then, shall we not measure the engineers at all? Your HR is certainly expecting some kind of metrics for her as a parameter to make salary adjustment.

What shall we look into then? Let's start from things that we shall never use to measure an engineer:
  • SLOC - Source Lines of Code, the dumbest ever metric. Good programmers write lesser lines.
  • Number of bug fixes - No way! The engineers would have figured out how to compromise this by writing more buggy code. I strongly against this just like how I disagree with giving points to bug fixes.
  • Hours logged - Timesheet is a waste of time.

I had like a 10-minute talk with the engineers yesterday to find out how they want to be evaluated and paid. I think this is important instead of me making decision and guesses based on how other software houses do it. It turned out that they want to be reviewed by the peers, by stories delivered, by code quality, and by having salary within different range they expect to be put an expectation on the skill set they possess.

So here is my summary, the performance metrics will be generally based on the expectations, the results, and the feedbacks:
  • The rating from their team members.
  • The rating from their direct supervisor, basically this person also knows the agility, code quality, velocity, etc. of this person.
  • The total stories delivered.
  • The skill set, which KPIs can be related with this, not entirely though.
Are KPIs necessary? KPIs shall not be mandated, they have to be discussed and proposed by the individuals, based on their comfort level and see how far we want to go above it. If a KPI is not going to help an engineer to produce quality code and better product, drop it. Also, if one doesn't want to play this KPI game as he really has a clear idea about every thing he is doing, don't force him to change the way he handles and manages his job and directions.

The last reminder is that, we shall not expect to have a perfect and fair formula that objectively measure and rate an engineer. Whether someone will be promoted or given a good salary shall be based on human judgments, as we are dealing with humans and humans make important decisions like you always do.

Friday, July 10, 2009

Hibernate session issue during Spring tests

I reckon this is something good to share as it took me a few hours to get the problem resolved.

We are using SpringJUnit4ClassRunner for our DAO tests, the tests passed when they were run in Eclipse, but not in Maven. One of the test classes failed with HibernateSystemException:

org.springframework.orm.hibernate3.HibernateSystemException: Illegal attempt to associate a collection with two open sessions; nested exception is org.hibernate.HibernateException: Illegal attempt to associate a collection with two open sessions

Google search gave me some clue about the problem had something to do with cascading style, transaction, and duplication but they didn't lead me anywhere. I broke at AbstractPersistentCollection.setCurrentSession to find out that the session/session factory (DAOs are instances of HibernateDaoSupport) assigned to one of the DAOs was different.

A second look at the test class which had:
@Autowired
BillingInvoiceDao $;
BillingOrderDao orderDao;
BillingItemDao itemDao;
MembershipDao membershipDao;

@Override
protected void setUpDataAccessObjects() {
membershipDao = DataAccessObjectHelper.getDataAccessObject(MembershipDao.class);
itemDao = DataAccessObjectHelper.getDataAccessObject(BillingItemDao.class);
orderDao = DataAccessObjectHelper.getDataAccessObject(BillingOrderDao.class);
}

I found out that BillingInvoiceDao was not a proxy instance but the other DAOs are. The log may explain something:
[07-11 09:19:30] DEBUG AutowiredAnnotationBeanPostProcessor [main]: Autowiring by type from bean name 'itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest' to bean named 'billingInvoiceDao'
[07-11 09:19:30] DEBUG DefaultListableBeanFactory [main]: Returning cached instance of singleton bean 'org.springframework.transaction.config.internalTransactionAdvisor'
[07-11 09:19:30] DEBUG DefaultListableBeanFactory [main]: Returning cached instance of singleton bean 'org.springframework.transaction.config.internalTransactionAdvisor'
[07-11 09:19:30] DEBUG AnnotationTransactionAttributeSource [main]: Adding transactional method [testCrud] with attribute [PROPAGATION_REQUIRED,ISOLATION_DEFAULT]
[07-11 09:19:30] DEBUG AnnotationAwareAspectJAutoProxyCreator [main]: Creating implicit proxy for bean 'itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest' with 0 common interceptors and 1 specific interceptors
[07-11 09:19:30] DEBUG JdkDynamicAopProxy [main]: Creating JDK dynamic proxy: target source is SingletonTargetSource for target object [itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest@17afcff]
[07-11 09:19:30] DEBUG AnnotationTransactionAttributeSource [main]: Adding transactional method [testCrud] with attribute [PROPAGATION_REQUIRED,ISOLATION_DEFAULT]
[07-11 09:19:30] DEBUG TransactionalTestExecutionListener [main]: Explicit transaction definition [PROPAGATION_REQUIRED,ISOLATION_DEFAULT] found for test context [[TestContext@134b58c testClass = HibernateBillingInvoiceDaoTest, locations = array['classpath:spring/dataAccessTestContext.xml', 'classpath:spring/dataAccessTestContext.billing.xml'], testInstance = itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest@17afcff, testMethod = testCrud@HibernateBillingInvoiceDaoTest, testException = [null]]]
[07-11 09:19:30] DEBUG TransactionalTestExecutionListener [main]: Retrieved @TransactionConfiguration [@org.springframework.test.context.transaction.TransactionConfiguration(defaultRollback=true, transactionManager=mainTxManager)] for test class [class itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest]
[07-11 09:19:30] DEBUG TransactionalTestExecutionListener [main]: Retrieved TransactionConfigurationAttributes [[TransactionConfigurationAttributes@ddc524 transactionManagerName = 'mainTxManager', defaultRollback = true]] for class [class itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest]
[07-11 09:19:30] DEBUG DefaultListableBeanFactory [main]: Returning cached instance of singleton bean 'mainTxManager'
[07-11 09:19:30] DEBUG TransactionalTestExecutionListener [main]: Executing @BeforeTransaction method [public void net.aflexi.cdn.core.test.AbstractDaoTest.setUpBeforeTransaction() throws java.lang.Exception] for test context [[TestContext@134b58c testClass = HibernateBillingInvoiceDaoTest, locations = array['classpath:spring/dataAccessTestContext.xml', 'classpath:spring/dataAccessTestContext.billing.xml'], testInstance = itest.net.aflexi.cdn.billing.HibernateBillingInvoiceDaoTest@17afcff, testMethod = testCrud@HibernateBillingInvoiceDaoTest, testException = [null]]]
[07-11 09:19:30] DEBUG DefaultListableBeanFactory [main]: Returning cached instance of singleton bean 'membershipDao'

That transaction propagation happened after autowiring of beans. The fix of my problem is to @Autowired all DAOs (so that they are consistent) or assign them via the DataAccessObjectHelper as shown in the snippet.

Hope this helps.