added MockTracer and MockTracer.Tests projects #54
Conversation
|
Hi @Aaronontheweb, we're just in the process of collecting a Language Working Group together and restarting work on C#. API changes are on pause until then, but a MockTracer would be welcome. @carlosalberto do you mind reviewing this? |
|
Important to note that this is based off the current C# APIs which are quite a bit outdated in relation to the java ones (e.g. Scope doesn't exist). |
|
Yes, there's about to be quite the overhaul. I'm going to add a note to the README. |
Train has left the station in that regard. I'm taking a dependency on OpenTracing in a commercial product we're building. I'm going with whatever packages are available. Delaying adoption and implementation of the OpenTracing standard because of some API changes isn't a feasible option. I'd be happy to support ActiveSpan, Scopes, et al when they're implemented. IMHO, merge this and just update both when the new APIs come out. Better to have people adopt the API as-is even if it's a moving target than delay indefinitely. |
|
@Aaronontheweb Sure, I think the plan is to first do a full review of your PR and then merge it - and, later on, iterate and bring the I will review it fully later today, but overall it looks great ;) |
|
Also would love the feedback from @cwe1ss who seems to be back ;) |
|
yep, I'm back. :) I'm committed to investing a few days here so I'd also prefer getting the API updated ASAP and then having this PR target the updated master. Is this code based on the java mock-tracer? |
|
Thanks, much appreciated! @cwe1ss yep, follows the Java |
|
Although, an implementation note: had to use .NET Standard 1.1 in order to get access to some of the APIs we use on |
|
Should update to the latest API now that that's in, but that should be easy and not need to block initial review. I do have one question @carlosalberto and @cwe1ss though The attached implements MockTracer following Java like I did for the APIs. Do we want to follow the same approach - port then make changes - as that change, or should I make comments on potential locations for improvements for a Mock/InMemory tracer in this review? E.g. are we reviewing for validity/parity-with-java or a full review? |
|
I'd say for this version we should just port the code and keep functionality as similar as possible. This allows us to release this version ASAP and therefore unblock everyone from writing contrib-libs and tracers. We can discuss functional improvements afterwards. @Aaronontheweb Do you have time in the next few days to update this PR? If so, please just put the files into a "Mock" subfolder in the main OpenTracing assembly. If we decide to have multiple packages (#55), I'll fix all of them in one go after everything is merged. |
| namespace OpenTracing.MockTracer | ||
| { | ||
| /// <inheritdoc cref="ISpan"/> | ||
| public class MockSpan : ISpan |
There was a problem hiding this comment.
Java's is final, can make this sealed and simplify the Dispose implementation
| /// </summary> | ||
| private static long _nextIdCounter = 0; | ||
|
|
||
| public static long NextId() |
There was a problem hiding this comment.
Looks like this can be private (to match Java)
| public long TraceId => _context.TraceId; | ||
| public long SpanId => _context.SpanId; | ||
|
|
||
| private readonly MockTracer _tracer; |
| return SetObjectTag(key, value); | ||
| } | ||
|
|
||
| private ISpan SetObjectTag(string key, object value) |
There was a problem hiding this comment.
Java uses synchronized here. C# doesn't have this, so explicit locks would probably be the best approximation
|
|
||
| public override string ToString() | ||
| { | ||
| return $"TraceId: {TraceId}, SpanId: {SpanId}, ParentId: {ParentId}, OperationName: {OperationName}"; |
There was a problem hiding this comment.
[nit] JSON format like java (specifically, prefix and suffix { and }, respectively)
| /// easy to test assertions against the OpenMetrics API and its use | ||
| /// inside frameworks. | ||
| /// </summary> | ||
| public class MockTracer : ITracer |
There was a problem hiding this comment.
@cwe1ss explicitly, perhaps you can make a branch with those two that @Aaronontheweb can base off of?
| /// </summary> | ||
| public class MockTracer : ITracer | ||
| { | ||
| private ConcurrentQueue<MockSpan> _finishedSpans = new ConcurrentQueue<MockSpan>(); |
There was a problem hiding this comment.
[nit] It is possible to make this readonly if using locks like java uses synchronized for access to this structure, but probably not worth the effort (e.g. who really cares if this private field is readonly, the cases where it matters are pretty small).
There was a problem hiding this comment.
I removed the Concurrent* stuff and went back to regular Lists and locking to be closer to Java. The PR was missing some locks anyway.
| _finishedSpans.Enqueue(span); | ||
| } | ||
|
|
||
| public IEnumerable<MockSpan> FinishedSpans => _finishedSpans; |
There was a problem hiding this comment.
Java does
@return a copy of all finish()ed MockSpans started by this MockTracer (since construction or the last call to
* MockTracer.reset()).However by using a lazily evaluated IEnumerable rather than an eagerly evaluated List, the following code would not do what is expected
// pseudo-code
var tracer = new MockTracer(...);
ISpan a = ...;
tracer.FinishSpan(a);
var justA = tracer.FinishedSpans;
ISpan b = ...;
tracer.FinishSpan(b);
var aAndB= tracer.FinishSpans;
// At this point, justA == aAndB and both have both a and b| /// <summary> | ||
| /// <see cref="IPropagator"/> implementation that uses <see cref="ITextMap"/> internally. | ||
| /// </summary> | ||
| public sealed class TextPropagator : IPropagator |
There was a problem hiding this comment.
Add counterpart for Propagator PRINTER = new Propagator...?
|
|
||
| public ISpan Start() | ||
| { | ||
| if (_startTime == DateTimeOffset.MinValue) // value was not set by builder |
There was a problem hiding this comment.
Probably just pending merge with #50, but this does not handle ITracer.ActiveSpan
|
I updated your PR because I want to get as much done as possible while I have time. I hope that's ok! The code should now be very close to the Java version. There's the following differences:
Feedback and new reviews welcome! |
|
@ndrwrbgs regarding #54 (comment): I do not really understand your scenario. Why would you have e.g. a |
|
sorry @cwe1ss , been traveling on business and slow to respond. No problem at all. Would just be glad to see this stuff in OpenTracing. If you need to modify the PR, please go ahead by all means. |
|
@cwe1ss is there anything else I need to do here? |
I'd rather not get into the technical details of why this was necessary, but let's let it suffice to say that OpenTracing is an interfaces specification, and folks should be able to do what they want within those interfaces unless the interface has specified otherwise. If that is not convincing then consider this -- if SpanContext was supposed to be a simple IN MEMORY representation of a dictionary, it wouldn't have had to be an interface. It's quite possible it's calling off to some database, or it's deserializing a wire-based context lazily (e.g. avoid deserialization costs when the code doesn't read from it). Our internal tracing resulted in a similar situation to these two, and resulted in code similar to this review where I was hoping that the |
There was a problem hiding this comment.
Main concerns - be explicit in intent (implement IEquatable if equality is to be exposed by a type), and the question about whether this is ~=TestTracer or ~=InMemoryTracer, the later of which could be used for the former, but the former of which may not be performant enough for the later.
src/OpenTracing/Mock/MockSpan.cs
Outdated
| } | ||
| } | ||
|
|
||
| public struct Reference |
There was a problem hiding this comment.
[nit] you should generally prefer to implement IEquatable anyway, especially if used in a distinct collection. It'll help you avoid subtle bugs and other-dev-proof your code (e.g. right now it's not very clear that we have constraints on this type properly implementing equality. I'm also inclined to believe (e.g. I wrote it then second guessed myself but don't want to measure right now) that the default equality comparer will result in boxing for a struct, unless you've implemented IEquatable.
There was a problem hiding this comment.
I checked that ( e.g. https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/how-to-define-value-equality-for-a-type ) and it's true that the default equality comparison for structs is slow (as it uses reflection). I'll change it.
|
|
||
| public ISpan SetBaggageItem(string key, string value) | ||
| { | ||
| lock (_lock) |
There was a problem hiding this comment.
:( locks hurt
System.Collections.Concurrent avoid locks, and definitely minimize them (e.g. why is the string format inside the lock?)
This is fine if this is genuinely a MockTracer, only used for testing, but it almost seems like it's an InMemoryTracer. If we are not going to have a separate InMemoryTracer, then I'd suggest we ensure that this tracer is also optimized for production code for folks who want to instrument but do not want to send their data off to another system quite yet.
Actually, I revise the above to question explicitly -- for a developer instrumenting but not yet set up with a trace sink location, what options do we plan to expose for them to be able to, for instance, just hook up all trace data to go to standard out (ideally we expose an event so they can send to whatever out they wish).
There was a problem hiding this comment.
This tracer is meant to be used for (unit) testing and definitely not for production. Performance therefore isn't that much of a concern.
Using Concurrent would be nice but it's not enough (as e.g. _baggage is a regular variable that is replaced) so we need this locking logic anyway. Right now it's at least consistent and more similar to what Java has.
Actually, I revise the above to question explicitly -- for a developer instrumenting but not yet set up with a trace sink location, what options do we plan to expose for them to be able to, for instance, just hook up all trace data to go to standard out (ideally we expose an event so they can send to whatever out they wish).
I guess this would be a good fit for a opentracing-contrib project. Something like this shouldn't be part of the core API library as it probably needs a different release cycle etc.
you could review the current code if you want/have time. |
|
@ndrwrbgs regarding your general question: Would be great if we could discuss this in a separate issue. I think it's a little bit off-topic here as it seems to be a broader issue/question. Maybe you can also provide some short pseudo code and information about what actual values you are trying to access and where to help me better understand your scenario -e.g. is it in your instrumentation code or in your tracer code? (if you're writing your own tracer code) etc. I'm just beginning to use this stuff as well so please excuse me if I'm not of much help. You can also ask your question in the "specification"-repo or on gitter if you think it's a general topic and if you want to reach a broader audience. |
As Christian mentioned it, I think this could be discussed in a different issue, and I'd go as far as discussing it in the opentracing-java to get further feedback - this would help to have those features, if/when validated, not only here, but also in the other opentracing-supported languages ;) |
|
I changed the Just to put some time constraint on this as well, I'll probably merge this on Tuesday morning UTC if there's no further objections. |
| public bool Equals(Reference other) | ||
| { | ||
| return other != null && | ||
| EqualityComparer<MockSpanContext>.Default.Equals(Context, other.Context) && |
There was a problem hiding this comment.
EqualityComparer<MockSpanContext>.Default wow, VS generates weird Equality. They should take a hint from R# :-)
[not asking for a change here]
| /// to <see cref="ITracer.Inject"/> and/or <see cref="ITracer.Extract"/>. | ||
| /// </summary> | ||
| public MockTracer(IPropagator propagator) | ||
| : this(NoopScopeManager.Instance, propagator) |
There was a problem hiding this comment.
Should we default to AsyncLocalScopeManager here to match the parameterless constructor?
There was a problem hiding this comment.
+1 on this. The one from Java effectively uses ThreadLocalScopeManager, which is the default/defacto manager, and AsyncLocalScopeManager is the equivalent one.
There was a problem hiding this comment.
For this overload, Java uses the NoopScopeManager as well. Changing this would be a functional difference so I'd prefer not to do that for now.
src/OpenTracing/Mock/Propagators.cs
Outdated
| MockSpanContext Extract<TCarrier>(IFormat<TCarrier> format, TCarrier carrier); | ||
| } | ||
|
|
||
| public sealed class PrinterPropagator : IPropagator |
There was a problem hiding this comment.
I think ConsolePropagator would be clearer than Printer.
There was a problem hiding this comment.
It has the current name because it was ported from Java but I definitely agree that ConsolePropagator would be a better name (and more common in .NET).
I think this would be a reasonable deviation as it's not in the core API and as it makes it much easier to understand.
Anyone has an issue with PrinterPropagator being renamed to ConsolePropagator?
There was a problem hiding this comment.
I think that's actually a good deviation, so +1 on that renaming.
There was a problem hiding this comment.
I changed the name to ConsolePropagator
In order to unit test our consumption of the OpenTracing APIs inside our framework, I started designing my own test tracer we could use to carry out assertions and such.
Turns out the Java open tracing project already has a
MockTracerthat does what I needed, so I chose to port that over instead. This should also help provide a basic reference sample that implementors can refer to for building an OpenTracing-compatible client.