Added SetTag and WithTag for use with OpenTracing.Tag.Tags #72

Falco20019 · 2018-03-01T22:31:57Z

I found the usage of OpenTracing.Tag.Tags somewhat inconvinient, since it was not usable with Fluent API.

ndrwrbgs · 2018-03-02T05:11:03Z

src/OpenTracing/Mock/MockSpan.cs

+
+            public override string ToString()
+            {
+                return $"Timestamp: {Timestamp}, Fields: " + string.Join("; ", this.Fields.Select(e => $"{e.Key} = {e.Value}"));


Just a note - unrelated change

Agreed. Please remove this.

Thanks for noticing, it just slipped into the PR from my local changes.

ndrwrbgs · 2018-03-02T05:13:01Z

@Falco20019 any reason you could not just use the ISpanBuilder.SetTag(string, string) style? What prompts using the AbstractTag and children?

ndrwrbgs

If we are going to have AbstractTag<T> at all, then it makes sense to have these methods.

Tangential note - having it requires CallVirts which aren't the best in terms of performance, and Tracing should have a low footprint. But that's for another day.

cwe1ss

I'm not a fan of the existing non-fluent syntax either so I really like this idea.

We now have feature-parity with Java and I'd like to stay as close to them as possible so ideally this should be proposed in Java as well and merged only if both languages agree on it. Are you interested in creating a similar Java PR? If not, I'll do it.

Note that I flagged it as "breaking-change" as it adds members to the interface and therefore breaks implementations.

cwe1ss · 2018-03-02T09:01:32Z

src/OpenTracing/ISpan.cs

@@ -28,6 +29,9 @@ public interface ISpan
        /// <summary>Same as <see cref="SetTag(string,string)"/> but for numeric values.</summary>
        ISpan SetTag(string key, double value);

+        /// <summary>Set a tag on the Span using the helper in type.</summary>


Not sure if it's the fact that English is not my first language but this summary sounds weird. What's the "helper in type"?

„Type“ was a reference to the parameter name. Sorry, English isn’t my first language so I didn’t notice it prior. Will adjust it.

I renamed it to "tagSetter" and made it a paramref.

cwe1ss · 2018-03-02T09:02:59Z

src/OpenTracing/Mock/MockSpan.cs

+
+            public override string ToString()
+            {
+                return $"Timestamp: {Timestamp}, Fields: " + string.Join("; ", this.Fields.Select(e => $"{e.Key} = {e.Value}"));


Agreed. Please remove this.

dawallin · 2018-03-02T09:12:13Z

I found the usage of OpenTracing.Tag.Tags somewhat inconvinient, since it was not usable with Fluent API.

I don't see why it is not fluent, but I do agree it feels strange with the AbstractTag that you have to do span.SetTag({inttag}.Key, {value}). That said I don't like adding a generic method that all implementations have to implement, we should try to keep the contact surface as small as possible.

Tangential note - having it requires CallVirts which aren't the best in terms of performance, and Tracing should have a low footprint. But that's for another day.

Maybe I'm wrong but I think this is not needed if you for example implement public ISpan SetTag<bool>(AbstractTag<bool> type, bool value), I think the compiler will choose this method without any need for a CallVirt.

Could we solve this with extensions instead? What about

    public static class SpanBuilderExtensions
    {
        public static ISpanBuilder WithTag(this ISpanBuilder spanBuilder, AbstractTag<bool> key, bool value)
        {
            return spanBuilder.WithTag(key.Key, value);
        }

        public static ISpanBuilder WithTag(this ISpanBuilder spanBuilder, AbstractTag<int> key, int value)
        {
            return spanBuilder.WithTag(key.Key, value);
        }

        public static ISpanBuilder WithTag(this ISpanBuilder spanBuilder, AbstractTag<double> key, double value)
        {
            return spanBuilder.WithTag(key.Key, value);
        }

        public static ISpanBuilder WithTag(this ISpanBuilder spanBuilder, AbstractTag<string> key, string value)
        {
            return spanBuilder.WithTag(key.Key, value);
        }
    }
    public static class SpanExtensions
    {
        public static ISpan SetTag(this ISpan span, AbstractTag<string> key, string value)
        {
            return span.SetTag(key.Key, value);
        }

        public static ISpan SetTag(this ISpan span, AbstractTag<bool> key, bool value)
        {
            return span.SetTag(key.Key, value);
        }

        public static ISpan SetTag(this ISpan span, AbstractTag<int> key, int value)
        {
            return span.SetTag(key.Key, value);
        }

        public static ISpan SetTag(this ISpan span, AbstractTag<double> key, double value)
        {
            return span.SetTag(key.Key, value);
        }
    }

cwe1ss · 2018-03-02T11:40:09Z

@dawallin: I don't see why it is not fluent, but I do agree it feels strange with the AbstractTag that you have to do span.SetTag({inttag}.Key, {value}).

Having to use tag.Key looses the type safety for the value so it kind of defeats their reason of existence.

@dawallin: Could we solve this with extensions instead?

Yes, we could. I'm not sure if we want though as this no longer gives implementations the possibility to optimize behavior. E.g. by using extension methods, it would no longer be possible to do an immediate noop for these calls in a NoopTracer - it would always have to go through this extension method. This is probably a very small difference though.

@ndrwrbgs: Tangential note - having it requires CallVirts which aren't the best in terms of performance, and Tracing should have a low footprint. But that's for another day.

Do you have some perf comparisons/blog posts for this somewhere? I've done, some, googling, and it seems like C# uses callvirt in many cases anyway to ensure this != null so I wonder if this is something that we can even control properly?!

Falco20019 · 2018-03-02T11:57:18Z

@dawallin One distinctive feature of Fluent API is the chaining. And you can’t chain the Abstract<T>.Set calls.

I also thought about using extensions instead to avoid making it a breaking change by using ISpan SetTag<TTagType>(this ISpan Span, AbstractTag<TTagType> type, TTagType value); instead. I will have a look at the performance since I am building the C# Jaeger client right now. But @cwe1ss is right that this would make it a little bit more inefficient for NoopTracer. Don’t know yet if the difference is of any significance (using CalllVirts or for the NoopTracer).

Ammend + forced push didn't work for whatever reason...

Falco20019 · 2018-03-02T14:22:49Z

I did some performance tests using the following piece of code:

const int cmds = 1000000;
List<long> elapsedTicks = new List<long>();
var span = tracer.BuildSpan("Test").Start();

Stopwatch sw = new Stopwatch();
for (int iteration = 0; iteration < 10; iteration++)
{
    sw.Restart();
    for (int i = 0; i < cmds; i++)
    {
        //span.SetTag(Tags.Component, "grpc");
        Tags.Component.Set(span, "grpc");
    }
    sw.Stop();
    elapsedTicks.Add(sw.ElapsedTicks);
}

var elapsedAvg = elapsedTicks.Skip(2).Average() / cmds;

I tried the following implementations and reported the average ticks per call.

Method	Ticks/call
Extending `ISpan` by `ISpan SetTag<T>(AbstractTag<T> tagSetter, T value)` calling `SetObjectTag(tagSetter.Key, value))`	0.779871125
Extending `ISpan` by `ISpan SetTag(AbstractTag<...> tagSetter, ... value)` calling `SetObjectTag(tagSetter.Key, value))`	0.7455159375
Extension method `static ISpan SetTag<T>(this ISpan span, AbstractTag<T> tagSetter, T value)` calling `tagSetter.Set(span, value)`	0.795057625
Extension method `static ISpan SetTag(this ISpan span, AbstractTag<...> tagSetter, ... value)` calling `span.SetTag(tagSetter.Key, value))`	0.7327926875
Directly calling `StringTag.Set(span, value)`	0.803877

The method proposed by @dawallin is of course the fastest since it doesnt use generics and sets the span directly (4th result). Doing it directly in the class by extending the interface is about the same (2nd result) and would allow to implement the NoopSpan more officiently. Using generics increases the call times by 4-8%. I don't understand why direclty calling the StringTag.Set method (5th result) is so much worse, since it's technically nearly the same as with the extension method (4th result), but I repeated the whole measuring 10 times and it's always at around 0.8 ticks.

I repeated everything for the NoopSpan:

Method	Ticks/call
Extending `ISpan` by `ISpan SetTag<T>(AbstractTag<T> tagSetter, T value)`	0.1043355
Extending `ISpan` by `ISpan SetTag(AbstractTag<...> tagSetter, ... value)`	0.038422875
Extension method `static ISpan SetTag<T>(this ISpan span, AbstractTag<T> tagSetter, T value)` calling `tagSetter.Set(span, value)`	0.3302765
Extension method `static ISpan SetTag(this ISpan span, AbstractTag<...> tagSetter, ... value)` calling `span.SetTag(tagSetter.Key, value))`	0.302098125
Directly calling `StringTag.Set(span, value)`	0.343799

Looking at the results, the fastest seems to be option 2, doing it in the interface and without generics. Option 1 would still be a better future-proof fit in case that other types need to be supported. Otherwise, everytime a new type is added, this would be a breaking change.

ndrwrbgs · 2018-03-02T18:37:29Z

@cwe1ss

We now have feature-parity with Java and I'd like to stay as close to them as possible so ideally this should be proposed in Java as well and merged only if both languages agree on it. Are you interested in creating a similar Java PR? If not, I'll do it.

If we are going to try to keep these together indefinitely does it make sense to just merge the repos and have java + csharp in 1 repo so that issues and changes are done together?

@dawallin

Maybe I'm wrong but I think this is not needed if you for example implement public ISpan SetTag(AbstractTag type, bool value), I think the compiler will choose this method without any need for a CallVirt.

The CallVirt is done because of the abstract method - specifically AbstractTag<T>.Set.

@Falco20019
It's a nit, but I'd recommend using BenchmarkDotNet when doing perf tests. It handles many cases folks often omit, and it enables easily debugging things like Memory, Disassembly, Inlining - which are useful if someone is investigating further any differences in perf the test exhibit.

cwe1ss · 2018-03-02T23:20:06Z

@ndrwrbgs: If we are going to try to keep these together indefinitely does it make sense to just merge the repos and have java + csharp in 1 repo so that issues and changes are done together?

No idea what the best process is here... It's currently pretty hard to synchronize and keep track of all languages. gRPC even puts ALL languages into one repository but this probably has quite a few disadvantages as well. Anyway, I guess this is something that should best be discussed e.g. in https://gitter.im/opentracing/cross-language

Regarding this issue, I'm in favor of option 2 (having separate methods on the interface) as this gives tracers maximum flexibility. Also, adding a new type (e.g. DateTime) is a breaking change anyway as we would also have to add the regular SetTag(string key, DateTime value) method.

Falco20019 · 2018-03-08T21:35:19Z

@cwe1ss All points from the review were resolved. So please let me know if I can be of any further assistance here once you decided which option I should use in the PR. I saw that you are in favor of option 2.

@ndrwrbgs @dawallin I would update the code to apply option 2 if there are no further opinions.

cwe1ss · 2018-03-13T11:22:02Z

I created a similar perf test with BenchmarkDotNet.

I've been testing the following signatures with MockTracer (calling SetObjectTag()) and NoopTracer (doing a no-op):

ISpan SetTag_Int(string key, int value);

ISpan SetTag_AbstractGeneric<TTagValue>(AbstractTag<TTagValue> tag, TTagValue value);

ISpan SetTag_AbstractTyped(AbstractTag<int> tag, int value);

ISpan SetTag_IntTag(IntTag tag, int value);

Result:

BenchmarkDotNet=v0.10.13, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.248)
Intel Core i5-4300U CPU 1.90GHz (Haswell), 1 CPU, 4 logical cores and 2 physical cores
Frequency=2435765 Hz, Resolution=410.5486 ns, Timer=TSC
.NET Core SDK=2.1.4
  [Host]     : .NET Core 2.0.5 (CoreCLR 4.6.26020.03, CoreFX 4.6.26018.01), 64bit RyuJIT
  DefaultJob : .NET Core 2.0.5 (CoreCLR 4.6.26020.03, CoreFX 4.6.26018.01), 64bit RyuJIT

Method	Mean	Error	StdDev
MockSpan_IntTag	77.927 ns	1.5573 ns	1.5992 ns
MockSpan_AbstractTag_Generic	90.484 ns	1.5414 ns	1.4418 ns
MockSpan_AbstractTag_Typed	77.102 ns	1.5754 ns	1.6178 ns
MockSpan_IntTag_Key	80.962 ns	1.5437 ns	1.4440 ns
MockSpan_Const	76.666 ns	1.5525 ns	1.5943 ns
MockSpan_Const_Overload	77.437 ns	1.5908 ns	1.4880 ns
NoopSpan_IntTag	4.040 ns	0.1024 ns	0.0958 ns
NoopSpan_AbstractTag_Generic	13.419 ns	0.2553 ns	0.2263 ns
NoopSpan_AbstractTag_Typed	4.625 ns	0.1388 ns	0.2573 ns
NoopSpan_IntTag_Key	5.052 ns	0.1431 ns	0.1757 ns
NoopSpan_Const	3.021 ns	0.0989 ns	0.0925 ns
NoopSpan_Const_Overload	3.068 ns	0.0985 ns	0.1054 ns

Using one SetTag<T>(AbstractTag<T> tag, T value) method is considerably slower so I would rule that out. It's a little bit more future-proof but it doesn't properly support the existing IntOrStringTag and adding new types is a breaking change anyway, as we also have to add a regular SetTag(string key, NewType value).

It's surprising to me that using AbstractTag<int> is sometimes faster than using IntTag directly (I ran it a few times). I would still rule that out though as this method still doesn't properly support IntOrStringTag.

I would therefore prefer using the actual Tag-types directly! This would ensure, all existing Tag types are supported and it also looks better in IntelliSense IMO. Adding a new Tag type is a breaking change but as I've said before, this is breaking anyway.

To sum it up, I would prefer the following additions to our interfaces:

// ISpan
ISpan SetTag(BooleanTag tag, bool value);
ISpan SetTag(IntOrStringTag tag, string value);
ISpan SetTag(IntTag tag, int value);
ISpan SetTag(StringTag tag, string value);

// ISpanBuilder
ISpanBuilder WithTag(BooleanTag tag, bool value);
ISpanBuilder WithTag(IntOrStringTag tag, string value);
ISpanBuilder WithTag(IntTag tag, int value);
ISpanBuilder WithTag(StringTag tag, string value);

Thoughts?

Falco20019 · 2018-03-13T12:09:52Z

LGTM. Stats showed that IntOrStringTag or IntTag are the way to go (as expected) and I'm with you on the better readability of the IntTag variant :)

cwe1ss

LGTM!! Thank you! I'll leave it open for a few days to give people a chance to comment.

Falco20019 · 2018-04-06T14:09:20Z

I'm working on jaegertracing/jaeger-client-csharp#44 and it would be great to have this. For now, I will have to get the key to use it with SpanBuilder.WithTag.

Falco20019 · 2018-04-24T12:55:34Z

I just finished reviewing the opentracing-tutorial from @saschaholesch and lesson03 would have benefited of this PR. I only cross-link it so that someone may update the tutorial when this gets released.

cwe1ss · 2018-04-26T09:11:18Z

src/OpenTracing/Mock/MockSpan.cs

@@ -157,6 +158,30 @@ public ISpan SetTag(string key, string value)
            return SetObjectTag(key, value);
        }

+        public ISpan SetTag(BooleanTag tag, bool value)
+        {
+            tag.Set(this, value);


Should we just call SetObjectTag(tag.Key, value) in these new methods? Seems more readable than doing the additional jump through tag.Set().

@Falco20019 what do you think? Could you update this if you agree?

Sorry for the delay. Will do it tomorrow :) was out of office the last days and had some other projects to work on yesterday and today.

cwe1ss · 2018-04-26T19:05:09Z

I'm also still bullish about this. I created opentracing/opentracing-java#271 to get some feedback from the Java folks!

cwe1ss · 2018-05-02T07:49:27Z

@Falco20019 Thanks for updating the PR!

I'll merge this once we have agreement on what should be included in the next version - see #92. (There hasn't been any objections yet so we should be good to go soon)

cwe1ss · 2018-05-14T18:19:27Z

There hasn't been any concerns yet, so I'll merge it. Thank you very much @Falco20019 for creating the PR - this will be a great addition!

@ all: Feel free to post any concerns in #92 before we do the next release or create a new issue afterwards.

Added SetTag and WithTag for use with OpenTracing.Tag.Tags

3e4c97f

ndrwrbgs reviewed Mar 2, 2018

View reviewed changes

ndrwrbgs approved these changes Mar 2, 2018

View reviewed changes

cwe1ss added enhancement breaking-change labels Mar 2, 2018

cwe1ss requested changes Mar 2, 2018

View reviewed changes

Falco20019 added 2 commits March 2, 2018 04:43

Applied changes from review

3832e6a

Applied unsafed change

69c1370

Ammend + forced push didn't work for whatever reason...

Implemented tag helper versions

8ce4049

cwe1ss approved these changes Mar 13, 2018

View reviewed changes

cwe1ss added this to the Backlog milestone Mar 16, 2018

cwe1ss reviewed Apr 26, 2018

View reviewed changes

cwe1ss added breaks-instrumentation A breaking change for users of the OpenTracing API breaks-tracers A breaking change for tracers and removed breaking-change labels Apr 26, 2018

cwe1ss mentioned this pull request Apr 26, 2018

Add setTag/withTag overloads that accept io.opentracing.tag types opentracing/opentracing-java#271

Open

cwe1ss removed the breaks-instrumentation A breaking change for users of the OpenTracing API label Apr 26, 2018

cwe1ss mentioned this pull request Apr 26, 2018

v0.12.0 Release Plan #92

Closed

MikeGoldsmith approved these changes Apr 27, 2018

View reviewed changes

cwe1ss modified the milestones: Backlog, 0.12.0 May 1, 2018

Using SetObjectTag directly

00a52c5

Falco20019 mentioned this pull request May 2, 2018

Added C# support grpc-ecosystem/grpc-opentracing#42

Closed

cwe1ss mentioned this pull request May 7, 2018

Use IEnumerable<KeyValuePair<string, object>> for Log() methods (again) #91

Merged

cwe1ss merged commit 31fc4db into opentracing:master May 14, 2018

Falco20019 deleted the tag-helper branch May 16, 2018 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SetTag and WithTag for use with OpenTracing.Tag.Tags #72

Added SetTag and WithTag for use with OpenTracing.Tag.Tags #72

Falco20019 commented Mar 1, 2018

ndrwrbgs Mar 2, 2018

cwe1ss Mar 2, 2018

Falco20019 Mar 2, 2018

ndrwrbgs commented Mar 2, 2018

ndrwrbgs left a comment

cwe1ss left a comment

cwe1ss Mar 2, 2018

Falco20019 Mar 2, 2018

Falco20019 Mar 2, 2018

cwe1ss Mar 2, 2018

dawallin commented Mar 2, 2018

cwe1ss commented Mar 2, 2018

Falco20019 commented Mar 2, 2018 •

edited

Loading

Falco20019 commented Mar 2, 2018 •

edited

Loading

ndrwrbgs commented Mar 2, 2018

cwe1ss commented Mar 2, 2018

Falco20019 commented Mar 8, 2018

cwe1ss commented Mar 13, 2018

Falco20019 commented Mar 13, 2018

cwe1ss left a comment

Falco20019 commented Apr 6, 2018

Falco20019 commented Apr 24, 2018

cwe1ss Apr 26, 2018

cwe1ss May 1, 2018

Falco20019 May 1, 2018

cwe1ss commented Apr 26, 2018

cwe1ss commented May 2, 2018

cwe1ss commented May 14, 2018

Added SetTag and WithTag for use with OpenTracing.Tag.Tags #72

Added SetTag and WithTag for use with OpenTracing.Tag.Tags #72

Conversation

Falco20019 commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndrwrbgs commented Mar 2, 2018

ndrwrbgs left a comment

Choose a reason for hiding this comment

cwe1ss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawallin commented Mar 2, 2018

cwe1ss commented Mar 2, 2018

Falco20019 commented Mar 2, 2018 • edited Loading

Falco20019 commented Mar 2, 2018 • edited Loading

ndrwrbgs commented Mar 2, 2018

cwe1ss commented Mar 2, 2018

Falco20019 commented Mar 8, 2018

cwe1ss commented Mar 13, 2018

Falco20019 commented Mar 13, 2018

cwe1ss left a comment

Choose a reason for hiding this comment

Falco20019 commented Apr 6, 2018

Falco20019 commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwe1ss commented Apr 26, 2018

cwe1ss commented May 2, 2018

cwe1ss commented May 14, 2018

Falco20019 commented Mar 2, 2018 •

edited

Loading

Falco20019 commented Mar 2, 2018 •

edited

Loading