Enhancement: Re-implement the Tracing overall implementations - Addressing the Simplicity and Extensibility

[mikependon]

TL;DR Currently, the ITrace interface as being the base of all tracing capabilities, contains the methods that are equivalent to the existing extended operations for the IDbConnection.

See below for the Insert operation.

/// <summary>
/// A method that is being raised before the actual 'Insert' operation execution.
/// </summary>
/// <param name="log">The cancellable log object referenced by the Insert' execution.</param>
void BeforeInsert(CancellableTraceLog log);

/// <summary>
/// A method that is being raised after the actual Insert' operation execution.
/// </summary>
/// <param name="log">The log object referenced by the 'Insert' execution.</param>
void AfterInsert(TraceLog log);

This is true to all other operations (i.e.: BatchQuery, Query, Update, Merge, Update, etc etc).

In the actual operation, inside the library, if the trace argument is passed with the ITrace object, we do call the Before<Method> BEFORE the actual execution. We also do call the After<Method> AFTER the actual execution with some important information captured during the execution.

The implementation is good and robust on its own and it is also addressing the operational-tracing scenarios if we are to look at it on the users POV.

Concerns

The current implementation is very close for extensibility in which it does not conform with the "O" on the SOLID principles. In addition to this, even though the user can trace the execution of the operations, but there is no way for the user to customize its own execution (on its own perusal/way).

We therefore found out that, the current implementation requires a major refactoring when it comes to implementation.

Proposal

To cover the simplicity of the implementations and the freedom of the tracing to the users of the library, we are planning to refactor the ITrace interface to only contains 2 methods.

• BeforeExecution(CancellableTraceLog log);
• AfterExecution(TraceLog log);

The method BeforeExecution stands as the entry point for all the operations BEFORE the actual execution towards the database. The method AfterExecution stands as the exit point of the existing operation AFTER the actual execution from the database.

With this, a new property that handles the execution key is needed to be added into the TraceLog object, the base class for the argument that is being used for all the tracing (i.e.: CancellableTraceLog and TraceLog itself).

On the actual extended methods, the new argument named traceKey is also needed to be added to all the existing operations (i.e.: Insert, Delete, Update, Merge, etc)

See the sample proposal code below.

In the Insert operation, we will add an additional argument named traceKey. But the default value would be "Insert". In case the key is not passed, the "Insert" tracing will be executed.

public static object Insert<TEntity>(this IDbConnection connection,
	string tableName,
	TEntity entity,
	IEnumerable<Field> fields = null,
	string hints = null,
	int? commandTimeout = null,
	IDbTransaction transaction = null,
	string traceKey = "Insert",
	ITrace trace = null,
	IStatementBuilder statementBuilder = null)
	where TEntity : class
{
	...
}

Then, when you call this operation, you as a user will have an option to pass your customized key. In the example below, you call a TraceFactory.Create() that creates an instance of your customized ITrace object, and passing your own customized trace key.

var trace = TraceFactory.Create();
using (var connection = new SqlConnection(ConnectionString))
{
     var entity = new EntityModel
     {
          Property = "Value",
          ....
     };
     var id = connection.Insert<EntityModel>(entity,
          traceKey: "MyCustomTraceKeyForInsert",
          trace: trace);
}

The default value of the traceKey argument is "Insert" since you are calling an Insert operation. For the Query operation, it will be "Query", same goes for the others.

Sample code for your customized ITrace object.

public class MyCustomTrace : ITrace
{
     public BeforeExecution(CancellableTraceLog log)
     {
          if (log.Key == "Insert")
          {
               // This is for the insert operation
          }
          else if (log.Key == "MyCustomTraceKeyForInsert")
          {
               // This is for the insert operation (with your customized tracing key)
          }
          else if (log.Key == "Query")
          {
               // This is for the query operation
          }
          else
          {
               log.Cancel(true);
          }
     }

     public AfterExecution(TraceLog log)
     {
          if (log.Key == "Insert")
          {
               // This is for the insert operation
          }
          else if (log.Key == "MyCustomTraceKeyForInsert")
          {
               // This is for the insert operation (with your customized tracing key)
          }
          else if (log.Key == "Query")
          {
               // This is for the query operation
          }
     }
}

The implementation is very extensible and dynamic in the user's POV.

Drawbacks

This new proposal will create a breaking changes to those users who had already implemented the tracing on their solutions.

Mitigations

To avoid the breaking changes on the users POV, we can create a gist named TraceBase that would stand as the base trace object when tracing tracing all the operations.

Then, in the existing implementation (your class), you have to inherit this TraceBase class and have a minimal code change to override the existing method, instead of implementing the ITrace object.

In short, instead of this...

public class YourTraceClass : ITrace
{
     ...
}

Do this...

public class YourTraceClass : TraceBase
{
     ...
}

We are happy if the community of .NET could share their thoughts on this. 🙇🏼

[cajuncoding]

This looks like a great simplification, and will save a lot of ceremonial code, especially since my use case is really only to ILogger the raw SQL to app insights for metrics & investigations.

I understand the value of having string keys, (even though I don’t have a use case for a custom key)… but it would be good to not require clients to have to hard code literals for the default keys, so providing const values to use would be cleaner:

instead of:
if (log.Key == "Insert")

this would be cleaner:
if (log.Key == RepoDbTraceKey.Insert)

[mikependon]

@cajuncoding - thanks, but the purpose is to give this flexibility to the user.

Imagine, you would like to call the Insert operation but you would like to provide your custom trace key to have a targetted tracing (i.e.: InsertProduct for Product entity and InsertOrder for Order entity). Then, you can always put a logic on your ITrace class to have a targetted handler for this. Of course, if not overriden, the key would be Insert by default since the operation in used is Insert.

In this case, was not it putting a constant class that holds the keys would again limit the implementations?

I understand the value of having string keys, (even though I don’t have a use case for a custom key)… but it would be good to not require clients to have to hard code literals for the default keys, so providing const values to use would be cleaner:

I am thinking that the library consumer would maintain their own trace keys.

[cajuncoding]

Sure I was affirming that the flexibility is great. I was only recommending to also offer the default keys as constants…to save all consumers from having to do this themselves, reduce room for error, and make the feature a little more intuitive!

This should in no way reduce the flexibility of your design; only offer those that don’t use custom keys to not have to use string literals. Hope this helps clarify my recommendation.

[mikependon]

IKYDK. The thing in C# is, if you create an argument with a default value equals to whatever constant (either explicit value ("Insert") or implicit value (TraceKeys.Insert)) will still reflect as string literal "Insert" in the compiler 😄

(In which the TraceKeys.Insert = "Insert")

[cajuncoding]

Sure, the compiler will replace all instances of a constant with the literal.

My recommendation was only concerning readability and even a little discoverability benefit….ultimately a set of provided constants for those default strings offers the flexibility you want (no less), and for those that use only those (not custom) then they have less maintenance and less risk of errors; in case it changes in RepoDb no updates are needed, and even if the constant name change and at least our compile would break letting us know. Whereby if we code our own constants, and something in RepoDb changes then then the code never complains and something could easily go unnoticed.

I Hope this helps clarify. :-)

[mikependon]

@cajuncoding - no worry, this is not a big deal at all. We will introduce the TraceKeys class that would contain all the constants for the operation keys. Thanks for this recommendation :strong:

[cajuncoding]

Yep that makes sense! Thanks…yeah it’s a small thing but sometimes super useful.

[karli1]

Other enhancements:

First, I find it confusing that the "log.parameter" is actually the entity and not the parameters-collection... Maybe a better name (e.g. entity) would be preferable

Second, it would be good to access the actual parameter collection for tracing/logging purpose

[mikependon]

@karli1 the log.Parameter property contains the accessible passed parameters to the actual execution. The type of this parameter could be differed based on the operation invoked. It could be any of the following.

• Actual entity (Insert, Update, Merge, etc)
• A anonymous type (raw SQL)
• A query object (QueryGroup, QueryField)
• A Dictionary
• An internal CommandParameter.

The values are accessible anytime during the tracing.

Hope this answer both of your questions.

[karli1]

@mikependon Thank you for the clarification. Still, I would ask for both of my points.

1. if there are different types of parameter (which is the case), still I would not name it parameter (because parameter is, in the context of the interaction with the database a clearly defined term).

2. Still, for me the trace should be able to write (show) what is send to the database. The SQL-statement is displayed fine, but it uses the @parameter syntax (which is btw very good and necessary). However, the actual parameters are not accessible (e.g. when I update an entity).

[mikependon]

@karli1 maybe it is time to revisit the implementation to enable the actual DbParameter itself.

[mikependon]

As an additional enhancement to this major refactoring, we have decided to rename and simplify the Parameter property of the TraceLog object into a Parameters with a type of IEnumerable<IDbDataParameter>. With this, the developer is able to manipulate the parameters passed to the actual DbCommand object prior to the actual execution.

public IEnumerable<IDbDataParameter> Parameters { get; set; }

Where in the future, can be used as below.

public class MyCustomTrace : ITrace
{
     public void Before(CancellableTraceLog log)
     {
          if (log.Key == "MyCustomKey")
          {
               foreach (var parameter in log.Parameters)
               {
                    ...
               }
          }
     }
}

Please share your thoughts and reservations.

[mikependon]

The changes and updates for this User Story will be available to next releases > RepoDB v1.13.0-alpha1.