Key-Value Logging instead of LogEvent

[cwe1ss]

resolves #9

As expected, it's not possible in C# to use types with different generics in one List (like LogField<?> in Java).

It would be possible to use the following workaround, but implementations would have to cast/resolve the type anyway.

public interface IField {
  string Key { get; }
}

public class Field<T> : IField {
  string Key { get; }
  T Value { get; }
}

// usage
List<IField> fields;

Open questions/tasks:

• Already uses DateTime instead of DateTimeOffset - DateTime instead of DateTimeOffset; resolves #4 #11 should be merged first.
• @yurishkuro commented in the Java PR that the type could be called just "Field". I'll update this PR if they change it.
• Should we offer a static creation method (e.g. LogField.Create(string key, object value))? What would be the benefit?

[dawallin]

@cwe1ss I think the way you describe to create a list of different generic types is usually how it is done, also the way I do it in #10 .

+1 for the static factory method. Alternatively we could do something like
LogFields.AddField(key, value).AddField(key, value)... for a fluent API to create the List<IField> directly

[bhs]

(I did change to Field from LogField in the pending Java PR, btw)

[cwe1ss]

I renamed it to Field as well and also added a static Field.Of method. Using "Of" is not really common in C# afaik but again, I'm in favor of staying as close as possible to the Java version.

@dawallin if we would have LogFields.AddField(key, value).AddFields(...) and if I'm right, we would need another type (the custom LogFields collection) which would complicate the API in my opinion. It would also result in more list resizes (the collection doesn't know how many more fields will be added) which might not be good.
Perf/allocations are huge topics in the Java repo - I'm not that much into such detailed perf things so I can't say whether C#/.NET has the same issues or not (due to structs or a possibly better garbage collector). Anyway, we of course shouldn't be wasteful.

[dawallin]

Forget the LogFields builder. I agree that it is not worth adding more types and complexity. The static Field factory is enough.

[cwe1ss]

Java now uses two variations: Log(Map<string, ?> fields) and Log(String event). I updated this C# PR to be very similar - but not the same. My signature now looks as follows:

Log(IEnumerable<KeyValuePair<string, object>> fields);
Log(string @event); // "event" is a reserved word in C# so we need the @prefix)

IEnumerable<KeyValuePair<string, object>> is the base type of Dictionary<string, object>. With this, we are able to accept regular .NET dictionaries (which do unique key validations and are therefore a bit slow) and also collections that don't do these checks.

The new builtin Fields collection type does exactly that! It also provides static factory methods for a more fluent API.

Usage:

// Using a Fields collection initializer (does not do unique key checks).
span.Log(new Fields {
    { "event", "soft error" },
    { "type", "cache timeout" },
    { "waited.millis", 1500 }
});

// Using the Fields factory methods (does not do unique key checks).
span.Log(Fields.Create("event", "soft error", "type", "cache timeout", "waited.millis", 1500));

// Using an arbitrary dictionary (does unique key checks).
span.Log(new Dictionary<string, object> {
    { "event", "soft error" },
    { "type", "cache timeout" },
    { "waited.millis", 1500 }
});

// Using the Fields collection with its Add method.
var fields = new Fields();
fields.Add("event", "soft error");
fields.Add("type", "cache timeout");
fields.Add("waited.millis", 1500);
span.Log(fields);

Looking forward to your feedback!!

/cc @bensigelman @dawallin @yurishkuro

[cwe1ss]

@bensigelman @dawallin @yurishkuro sorry to ping you again, but this is a blocker for a release. I think the main question here is just whether you think that having the custom Fields collection is ok or not.

It would be great if you could post a short feedback until Thursday.

[bhs]

@cwe1ss just my two cents, but I would leave the Field "optimization" / syntactic sugar out of the initial API and wait until we see the access patterns more clearly. Not because it's a bad idea, but just because it's not strictly necessary. At least this is what happened with OT-java...

Is KeyValuePair part of the standard lib in C#?

[bhs]

Oh, one more thing: the enumerable of pairs allows for duplicate keys whereas a dictionary does not... comments should probably leave duplicate key semantics undefined since other languages don't even support them.

[dawallin]

I agree with @bensigelman and think we can wait with the extra code to keep the API as small as possible. But maybe it is time to start a util repository somewhere where we can try all these extensions out.

[dawallin]

@bensigelman Yes, KeyValuePair<TKey, TValue> is standard .Net

[dawallin]

The travis build is broken for the test project. Guess this is the problem: 1.0.x host is removed when installing 1.1 preview. Everything works on my machine.

[cwe1ss]

Having a possibility that does not do duplicate checks was intentional. my thinking was that people don't pass duplicate keys in 99,99%, so it's quite wasteful to do the duplicate checks. We could have left it to the trace implementations to decide whether they want to check the duplicates or not.

But I'll change it to accept only dictionaries with unique keys (IDictionary<string, object>). Once we have more users and more opinions, we can decide whether or not we want to move back to the more generic IEnumerable<KeyValuePair<string, object>>. It would work without breaking users. It would only break trace implementations.

I'll also remove the Fields class as it no longer makes sense with dictionaries only.

[bhs]

@cwe1ss I'm actually ok with the IEnumerable, I was just suggesting that you include a comment indicating that duplicate key semantics are undefined. But an IDictionary is also fine with me.

[cwe1ss]

OK - then I'll keep it :)

[cwe1ss]

I removed the fields type and added a comment. I'll merge this right now since this is the last open issue for a release. I'll do a final check in the master-branch myself and will then ask you for a final review before I'll actually release it.