AsyncLocal based scope manager (ported from Java) #63
Conversation
|
fyi: I'll probably merge this on Tuesday morning (UTC) if there's no contrary comments. |
| private readonly bool _finishOnDispose; | ||
| private readonly IScope _scopeToRestore; | ||
|
|
||
| public AsyncLocalScope(AsyncLocalScopeManager scopeManager, ISpan wrappedSpan, bool finishOnDispose) |
There was a problem hiding this comment.
[nit] it looks like the only place wrappedSpan is supplied here it's not wrapped in anything?
There was a problem hiding this comment.
Oooh it's the span that is BEING wrapped, not a span that IS wrapped. Obviously your naming is fine now that I realize this, but given I was confused for a bit suggests possibility for better naming (subjective, feel free to leave if you disagree)
There was a problem hiding this comment.
Naming is from Java, except that I named it wrappedSpan instead of just wrapped.
| { | ||
| ISpan span = Substitute.For<ISpan>(); | ||
|
|
||
| using (IScope scope = _source.Activate(span, false)) |
There was a problem hiding this comment.
[nit] I generally prefer named args for booleans (or nulls) because it makes the call site make a lot more sense.
E.g. in this place you'd be able to see we expect no Finish() because we said finishOnDispose: false
Without it you require more contextual brain-knowledge to be able to understand the test
| /// that relies on C#'s AsyncLocal/CallContext storage primitive. | ||
| /// </summary> | ||
| /// <seealso cref="IScopeManager"/> | ||
| public class AsyncLocalScope : IScope |
There was a problem hiding this comment.
This relates to the other review comment I made regarding the APIs. Using this structure removes the ability for any implementation to expect that the Scope they get back will be the one they pushed out of their APIs, which can cause some weird issues.
Do not have a good suggestion at this time, so this is not a comment to request change but just to start your brain pondering over solutions.
There was a problem hiding this comment.
Could you clarify this? I do not understand what you mean.
| { | ||
| if (_scopeManager.Active != this) | ||
| { | ||
| // This shouldn't happen if users call methods in the expected order. Bail out. |
There was a problem hiding this comment.
Throw InvalidOperationException?
The exception that is thrown when a method call is invalid for the object's current state
There was a problem hiding this comment.
Depends on your views... Throw from Dispose is generally frowned upon, but which is worse:
- Failing silently leaving a bug to go potentially unnoticed for quite some time and hard to debug
- Throwing from Dispose on code errors
The reason Throw on Dispose is frowned upon is because the following
try
{
using (var span = /* ... */)
{
// Do stuff
throw new IOException();
}
}
catch (IOException)
{
// Not hit if (non-IOException) thrown from Dispose, and the exception being thrown is not available to `Dispose` so it can't even be wrapped
}But I generally feel that's okay because you only loose an exception if you're 1) already in an exceptional state and 2) have done something exceptional with the library that suggests a developer bug (I avoid it when it's a state-based/concurrency-based bug but when it's something that's likely just a coding error it seems better to fail-fast to me)
There was a problem hiding this comment.
AFAIK not throwing an exception here has been a deliberate decision in Java as tracers are not meant to break the application (in the same way that loggers are not meant to break it). So if you want to have this changed we should discuss it in a separate issue.
| _wrappedSpan = wrappedSpan; | ||
| _finishOnDispose = finishOnDispose; | ||
|
|
||
| _scopeToRestore = scopeManager.Active; |
There was a problem hiding this comment.
Do we need to explicitly set the Parent here? I notice my InMemory implementation also does not (at this location) but I'm curious if we both missed part of the spec
There was a problem hiding this comment.
Could you clarify this? I do not understand what you mean.
|
Personally, I think wrapped may be a little less misleading :) but
non-issue, it didn’t waste too much time reparsing.
…On Sun, Feb 11, 2018 at 1:38 AM Christian Weiss ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/OpenTracing/Util/AsyncLocalScope.cs
<#63 (comment)>
:
> @@ -0,0 +1,43 @@
+namespace OpenTracing.Util
+{
+ /// <summary>
+ /// The <see cref="AsyncLocalScope"/> is a simple <see cref="IScope"/> implementation
+ /// that relies on C#'s AsyncLocal/CallContext storage primitive.
+ /// </summary>
+ /// <seealso cref="IScopeManager"/>
+ public class AsyncLocalScope : IScope
+ {
+ private readonly AsyncLocalScopeManager _scopeManager;
+ private readonly ISpan _wrappedSpan;
+ private readonly bool _finishOnDispose;
+ private readonly IScope _scopeToRestore;
+
+ public AsyncLocalScope(AsyncLocalScopeManager scopeManager, ISpan wrappedSpan, bool finishOnDispose)
Naming is from Java, except that I named it wrappedSpan instead of just
wrapped.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#63 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKRxOgK-qExdwCVVDuIm5vuGKynXmdZ8ks5tTrT4gaJpZM4SA9aT>
.
|
This is a port from opentracing-java's ThreadLocalScopeManager.
AsyncLocal(.NET 4.6+, .NET Core) or theCallContext(.NET 4.5). This code is based on ASP.NET Core's HttpContextAccessor.netstandard1.3andnet452as described in Target netstandard1.3 and net45 #62 - I'll change the targets here should that issue decide differently.AutoFinishScopeManageras I'm not sure if that's required in .NET (due to async/await). I'd say we port that later if someone really needs it.