Support single key:value pair for ISpan.Log

[ndrwrbgs]

Per the specification

Required parameters

One or more key:value pairs, where the keys must be strings and the values may have any type at all. Some OpenTracing implementations may handle more (or more of) certain log values than others.

Problem: A dictionary is far from free in C#, creating one just for a single key:value pair (and then the cost in using it in the tracing library implementation) can be avoided by adding an overload on the interface.

[cwe1ss]

Would the existing ISpan Log(string @event); cover your scenario?

#91 would also make it (slightly) easier to implement custom, more efficient, log types.

I'll backlog this to wait for more people who want this.

[ndrwrbgs]

Unfortunately no because Log(string) forces a key on you. The API says you should be able to supply key:value pairs, but this version of Log only allows you to supply a value.

[ndrwrbgs]

I'll backlog this to wait for more people who want this.

Unfortunately I doubt people will realize this with any haste. Performance is something that people find far later in the game, if at all :( But if performance is not a high priority item for opentracing-csharp I can keep such things I notice to myself :)

[cwe1ss]

But if performance is not a high priority item for opentracing-csharp I can keep such things I notice to myself :)

That's an unnecessary/destructive comment.

Backlog just means that it's not scheduled for any release. That's not my project alone so feedback from other people is essential. E.g. it would be interesting to hear how often people pass one vs. many elements to the method etc.

Could you please also add the actual signature change/addition you're proposing.

[ndrwrbgs]

I'm sorry, I did not mean it as destructive as it sounds - I meant it more genuinely. I do not mean to imply the project has intentions of being slow - but rather than it would be perfectly reasonable at this stage of it's lifecycle to not be preoccupied with performance yet - and if that's the case then I can hold my tongue until major players are using it and pushing for changes - taking on a reactive rather than proactive approach which would be just as valid at this point.

My fear of 'backlog' here would be that in my experience when an item that is made solely for performance isn't causing a system to grind to a halt (and is more of a single in many cuts), that item often doesn't get taken up later. That said, of course community feedback would be great I just meant to say I would caution on performance items from taking no input as a negative signal.

The API change that would enable tracers to optimize as they deem necessary would be either

• void Log(string key, string value)
• void Log(KeyValuePair<string, string>)

Compiled, they're essentially the same, so it really makes no difference to me which flavor so long as we can avoid allocating a dictionary just to send 2 strings.

[ThomasSkyldahl]

A more optimal way could be to accept an IEnumerable<KeyValuePair<string,object>> instead of a dictionary, I can't find anywhere in the spec that I specifies the keys should be unique, so a Dictionary is a limitation not expressed by the spec

[cwe1ss]

@ThomasSkyldahl PR #91 would bring that. However, it obviously still isn't as performant/easy to use as a separate Log(string, object value).

@opentracing/opentracing-c-maintainers (and anyone else): My request for more feedback here is mostly whether this is an "important enough" use case or whether people will need more than one key:value pair in most cases anyway and therefore would make this a negligible performance improvement.

Sure, it would be "just two more methods" but the question is where do we draw the line. This should be a conscious, well-discussed decision.

[carlosalberto]

@cwe1ss My opinion is that this would be more of an "Enhancement" - also, I'd discuss that in the Java scenario as well.

[ThomasSkyldahl]

@cwe1ss ahh great didn't see that one.

I would argue that if the API has Log(IEnumerable<KeyValue<string,object>> entries) and Log(string key, object value) it should be enough, there is no need to wrap two arguments in an extra object to unwrap and log them inside the lower levels in the code.

This is kind of back to the whole performance thing in the orginal issue description.

if people really want the Log(KeyValue<string,object> entry) they could make an Extension method in their own code.

[cwe1ss]

@ThomasSkyldahl I would argue that if the API has Log(IEnumerable<KeyValue<string,object>> entries) and Log(string key, object value) it should be enough, there is no need to wrap two arguments in an extra object to unwrap and log them inside the lower levels in the code.

The problem is that we don't have Log(string key, object value) right now. We only have Log(string @event). So the question is, do we need 3 overloads:

~Log(string @event);
+Log(string key, object value);
~Log(IEnumerable<KeyValuePair<string, object>> fields);

[ThomasSkyldahl]

@cwe1ss i see your point and there are even more if you include the DateTimeOffset versions

A few notes:

Its kind of strange that the actual value in the simple Log(string @event) isn't of type object like
all the other overloads, I know the standard convention for log event is string but its a convention not a spec.

the other thing is the hidden meaning of what the Log(string @event) method actually does

You have to actually read the docs to know that it logs {{"event": @event}}
a better name would be LogEvent(object @event) if the method should be kept at all.

I would argue that Log(string key, object value) is more understandable and doesn't hide what it does, and is easy enough to write.

another point about all the Log(DateTimeOffset timestamp, ....) it would be better if the parameter order was kept so that the API would look something like

Log(string key, object value);
Log(string key, object value, DateTimeOffset timestamp);
Log(IEnumerable<KeyValuePair<string, object>> fields)
Log(IEnumerable<KeyValuePair<string, object>> fields, DateTimeOffset timestamp)

see: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/member-overloading

I know the java API is structured like the current implementation, but java API's aren't always perfect :)

[cwe1ss]

Log(string @event) existed before key:value logging was introduced so one aspect of this is backwards compatibility - at least in Java. The C# api is still pretty young and with a much smaller user base so it might be an option to remove/deprecate that overload in favor of the more flexible Log(string key, object value). I would probably prefer that over having both overloads as typing span.Log(LogFields.Event, “foo”) isn’t too bad and more explicit anyway.

@opentracing/opentracing-c-maintainers thoughts?

@ThomasSkyldahl could you open a separate issue regarding the order of parameters?