Add support for NHibernate Custom Statistic

[gokhanabatay]

I want to inject a custom statistic implementation into NHibernate, but as I can see, it is not allowed right now.

I think its a good option to make this injectable like 'ISQLExceptionConverter' also configurable via config file, because we need to collect metrics on time consuming things at db calls.

In SessionFactoryImpl, the statistics member is type as a concrete implementation instead of an IStatistics or IStatisticsImplementor.

[NonSerialized]
private StatisticsImpl statistics;

If we just define to statistic object one for session statistic with IStatistics type other is factory statistic with IStatisticsImplementor type then we can easily inject it.

ISessionFactoryImplementor has StatisticsImplementor but its not injectable as I see.

[fredericDelaporte]

For having it injectable with a sound injection contract, we either need to have IStatisticsImplementor inherits IStatistics and internally use IStatisticsImplementor everywhere with an extension point asking for an IStatisticsImplementor, or we should provide a StatisticsBase with default overiddable implementations of IStatisticsImplementor and IStatistics and internally use StatisticsBase with an extension point asking for a StatisticsBase.
The former would be binary breaking and could only be done for a 6.0.

[gokhanabatay]

Most valuable log is here I think, Sql Execution Time and EffectedRowCount

command.ExecuteReader(); 
command.ExecuteNonQuery(); 
command.ExecuteScalar();

but now we cannot collect these metrics.
After that If this is a read operation whole fetch time of ISession.Query<T>.ToList() is very
important and session based but in the implementation it needs to activate whole
<property name="generate_statistics">true</property> and there is a lot of "lock" in this code.
But query elapsedms logging does not need to be locked.

Also operationTreashold is not configurable via config.
Also it could be nice to log executed sql not hql, rowcount and elapsedms both same line, with structure logging like below.

logger.InfoFormat("{sql} {rowcount} {elapsedms}",sql,rowcount,elapsedms);

Structured logging is a very nice solution for pushing data to log view frameworks as a separated column not just a message.

                [MethodImpl(MethodImplOptions.Synchronized)]
		public void QueryExecuted(string hql, int rows, TimeSpan time)
		{
			lock (SyncRoot)
			{
				queryExecutionCount++;
				if (queryExecutionMaxTime < time)
				{
					queryExecutionMaxTime = time;
					queryExecutionMaxTimeQueryString = hql;
				}
				if (hql != null)
				{
					QueryStatistics qs = GetQueryStatistics(hql);
					qs.Executed(rows, time);
				}
			}
			if (operationThreshold < time)
			{
				LogOperation(OperationExecuteQuery, hql, time);
			}
		}

As a summary StatisticsBase with an overrideable and configurable good choice.
But sql execution time, row count and fetch time needs to be separated they don't need to be locked.

[fredericDelaporte]

We may allow injecting a custom statistics implementation, but it is unlikely we will add additional statistics to it.
It is not meant for profiling. For this, consider using profiling tools, like NHProf.

[gokhanabatay]

If we can measure just query execution time and row count without other metrics and locking all other threads it's fine.
We are using Graylog for finding bottlenecks, if we just inject somewhere session based job will done.

[micmerchant]

We are developing an Asp.Net application and have added a custom statistic implementation to achieve that. But to do this, we've changed the NHibenrate code slightly and build our own custom version. We changed the following things:

**************************************************************************	
CLASS ISessionFactory
**************************************************************************

//added:

void InitStatistics(IStatistics stat, IStatisticsImplementor impl);

**************************************************************************
CLASS SessionFactoryImpl
**************************************************************************
//member removed:
[NonSerialized]
private StatisticsImpl statistics;
		
//new members:
[NonSerialized] 
private IStatistics statistics;
[NonSerialized]
private IStatisticsImplementor statisticsImpl;
		
//new method:

public void InitStatistics(IStatistics stat, IStatisticsImplementor impl)
{
	statistics = stat;
	statisticsImpl = impl;
}
//Init method changed to:
private void Init()
{
	var impl = new StatisticsImpl(this);
	statistics = impl;
	statisticsImpl = impl; //new!
	queryPlanCache = new QueryPlanCache(this);
}
		
//StatisticsImplementor Property of
public IStatisticsImplementor StatisticsImplementor
{
	get { return statistics; }
}
		
//chagned to
		
public IStatisticsImplementor StatisticsImplementor
{
	get { return statisticsImpl; }
}

We initialize the statistics in our Session-Manager class and the statistics are stored within the HttpContext due to the Web-Application architecture.

@gokhanabatay

After that If this is a read operation whole fetch time of ISession.Query.ToList() is very
important

Another way for this special case could be, wrapping the Session and write a query interceptor. I've attached ours. Don't worry about the FilteredSession references inside. As I looked into the NHibernate code last, they are also working with interceptors.

QueryInterceptor.zip

[gliljas]

Having profiling at runtime is definitely a good thing. NHibernate could provide the extension points easily, without adding overhead or complexity. I would however consider refactoring the execution "pipeline" before adding something specific, so that things such as command retries etc. could be implemented.

[micmerchant]

@gliljas
https://www.hibernatingrhinos.com/products/nhprof
?

[gliljas]

@micmerchant
Yes, I know. I use it, and it's great, but NHibernate could do well with providing better injection points for such things. I haven't investigated the latest version, but NHProf used to have to do really dirty things in order to inject itself into NHibernate.

And the same pipeline could be used for resiliency etc.

[zeqk]

I need this too, please add custom statistic support

[zeqk]

For having it injectable with a sound injection contract, we either need to have IStatisticsImplementor inherits IStatistics and internally use IStatisticsImplementor everywhere with an extension point asking for an IStatisticsImplementor, or we should provide a StatisticsBase with default overiddable implementations of IStatisticsImplementor and IStatistics and internally use StatisticsBase with an extension point asking for a StatisticsBase.
The former would be binary breaking and could only be done for a 6.0.

Please, can you add this feature to the 6 milestone list? https://github.com/nhibernate/nhibernate-core/milestones/6.0