No context on logs

[verdie-g]

Hello, to give a logger to the client the static method https://github.com/aerospike/aerospike-client-csharp/blob/e435cbd952a1749f9e3841fa4073343aea805d7b/Framework/AerospikeClient/Main/Log.cs#L78 Since it's static we can't add any context to the log such as the cluster name and we end up with log such as Add node X.X.X.X 4333 failed: Error -1: Invalid non-TLS address but it's hard to predict the impact with just an ip.

What do you think about these 3 different solutions:

1. Add some context to the log callback with the cluster name?
2. Have an instance logger by client so we can add any context and even business-specific ones
3. Automatically add context to the log message

[BrianNichols]

The clusterId (automatically generated integer) is printed in Node.ToString().

https://github.com/aerospike/aerospike-client-csharp/blob/e435cbd952a1749f9e3841fa4073343aea805d7b/Framework/AerospikeClient/Cluster/Node.cs#L943-L946

The reason the "Add node failed" error message doesn't include clusterId is that the new peer node was not created, so there is no Node instance. The log message (and others like this) could be changed to explicitly add the clusterId like so:

Log.Warn("Add node " + host + ' ' + cluster.clusterId + " failed: " + Util.GetErrorMessage(e));

Is that sufficient?

The alternative is to print clusterName instead. Note that if ClientPolicy.clusterName is set, it must match the server cluster-name configuration.

[verdie-g]

An automatically generated integer doesn't help a lot :/ The cluster name would be a great first step, that would be the 3rd option I was suggesting. The problem I see with that one is that if it's a part of the message, we can't add it as a field in the log. Meaning we won't be able to do aggregation in systems like Kibana.

The 1st solution or 3rd solution could be then considered to have something more robust.

[BrianNichols]

The following could be added to Log.cs:

public delegate void CallbackMessage(string clusterName, Level level, string msg)
public static void SetCallback(CallbackMessage callback)

clusterName would be null (or empty string?) when the message is not associated with a cluster (like log messages in AsyncTimeoutQueue).

If there is a need for further context, the delegate could be defined as:

public class Context {  // maybe struct instead of class?
  Cluster cluster; // can be null.  Or just string clusterName?
  Node node;  // can be null.
}

public delegate void CallbackMessage(Context context, Level level, string msg)

Thoughts?

[verdie-g]

I would go for the second one as it's more future-proof but only the identifiers (i.e. cluster name instead of Cluster) to avoid users calling some Node or Cluster methods in the callback which would be weird. I'll share that with my team to have their thoughts. Thanks.

[BrianNichols]

Ok. The implementation will be:

public sealed class Context
{
    public readonly string clusterName;
}

public delegate void ContextCallback(Context context, Level level, string message)
public static void SetContextCallback(ContextCallback callback)

nodeName is already part of the log message when a node is included in the message. We can always add others fields to Context in the future.

[BrianNichols]

C# client 5.30 is released: https://download.aerospike.com/download/client/csharp/notes.html#5.3.0

[verdie-g]

I (would like to) reopen for a documentation issue: https://github.com/aerospike/aerospike-client-csharp/blob/53c25583b8def75b507bb1a47776361f1500d8b1/Framework/AerospikeClient/Policy/ClientPolicy.cs#L39-L45

1. Can I put an arbitrary string in cluster name?
2. What is the "cluster-name" info command?

[BrianNichols]

1. No. If set, the clusterName must match the cluster-name configuration on the server. This ensures that the specified seed nodes belong to the expected cluster on startup.

service {
    ...
    cluster-name mycluster
}

2. The client uses the "cluster-name" info command to retrieve the configured cluster-name on each server node. The last doc sentence is outdated since "cluster-name" was introduced in server 3.10 and all server versions after that support the "cluster-name" info command. I will remove that last sentence and mention how to configure server cluster-name in the doc.