-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: demonstrate some concepts #5
base: master
Are you sure you want to change the base?
Conversation
... while also moving the refcounting to the util package and creating a cleaner path to something different should we want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(self-comments as a sort of guided tour)
* RPC will be unfinished but blocked on IO while the RPC is still outstanding. A {@link Scope} defines when a given | ||
* {@link Span} <em>is</em> scheduled and on the critical path. | ||
*/ | ||
public interface Scope extends Closeable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda/sorta replaces ActiveSpan
* Defined as equivalent to calling {@code tracer.scopeManager().activate(this)} on the {@link Tracer} instance | ||
* that started this {@link Span} instance. | ||
*/ | ||
Scope activate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only meaningful change to the Span interface... this is just sugar and doesn't need to be here (but it does save a lot of typing and complexity).
public interface Tracer { | ||
|
||
ScopeManager scopeManager(); | ||
void setScopeManager(ScopeManager scopeManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this can be set – def different than inheriting as we did before
* // (Do work) | ||
* span.setTag( ... ); // etc, etc | ||
* } // Span finishes automatically unless deferred via {@link ActiveSpan#capture} | ||
* } // XXX Span finishes automatically unless deferred via {@link ActiveSpan#capture} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: I did not update most comments
/** | ||
* Created by bhs on 8/5/17. | ||
*/ | ||
public class AutoFinishScopeManager implements ScopeManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class demonstrates that the refcounting can be built entirely outside of the core OT API
* or implied. See the License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
package io.opentracing.usecases; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(misc: decided to get started with a "usecases" package/module)
/** | ||
* Created by bhs on 8/1/17. | ||
*/ | ||
public class ThreadLocalScopeManager implements ScopeManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the simple case (note that this does not need to be part of the core OT API!)
// Nothing to do. | ||
} | ||
|
||
// Make sure the Span got finish()ed this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e., the refcounting can still work.
@@ -186,7 +198,7 @@ | |||
* @see ActiveSpanSource | |||
* @see ActiveSpan | |||
*/ | |||
ActiveSpan startActive(); | |||
Scope startActive(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking; maybe with a decent observer definition, could it be feasible to let the ScopeManager implementation decide whether to auto-activate started spans?
If it can register itself as observer to started spans, it can make the decision to auto-activate or not (maybe even based on some inspection of the started span).
That way for the instrumentation the tracer API basically becomes simple again (pre-0.30), while affording adding such complexity if desired in the scope manager as implementation decision.
Just thoughts basically, would have to play with things to test feasibility...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose activation might be a decision of specific framework instrumentation that will not be aware if auto-activation is enabled or not.
// We can't use 1.7 features like try-with-resources in this repo without meddling with pom details for tests. | ||
try (AutoFinishScopeManager.AutoFinishScope scope = autoFinishActivator.activate(span)) { | ||
// Take a reference... | ||
continuation = scope.defer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having the defer
and Continuation
as part of the API will be a problem, as it means any async based framework instrumentations will be bound to the AutoFinishScopeManager
implementation.
If we want to reduce the number of components, how about having Scope.defer()
return a deferred scope which can then be activated in the new context? So the Scope
is either activated immediately on startActive()
or explicitly via its activate()
method - but can only be activated once.
Summarized the changes and testing results of this PR here: https://gist.github.com/carlosalberto/28ba04ae3201d87f91eff924e7d427da That will get an overall idea for those who haven't been lately following the discussion, as well as for those interested in any further changes to this design - specially the And hopefully this will get things rolling again (feedback, for a start) :) |
Anyone interested in proposing changes to Java API, please use https://github.com/opentracing-contrib/java-examples to ensure all identified use cases are being addressed. BTW I recommend forking Carlos's branch if you are working on the current PR: https://github.com/carlosalberto/java-examples/tree/bhs/scopes-update |
* {@link Scope} instance after this is only defined to do no harm to the process (i.e., not-crash). Again, this is | ||
* only intended for use during initialization. | ||
*/ | ||
void setScopeManager(ScopeManager scopeManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice not to require that in the API, since it can always be a part of implementation-specific initialization sequence. Do we know of a use case where this method is absolutely required in the Tracer API?
(deliberately not opening up a PR against the OT version of this repo... for casual inspection only)
This change is