Pull in existing code from talsma-ict.#1
Pull in existing code from talsma-ict.#1sjoerdtalsma merged 52 commits intoopentracing-contrib:masterfrom talsma-ict:master
Conversation
|
@adriancole @wu-sheng I can't select you as reviewers, but I know you have a strong opinion about this as well 😄 |
|
first peek is deja-vu from java-globaltracer.. either that's good or not :) I will take a look at this tomorrow after feedback from the former is digested |
codefromthecrypt
left a comment
codefromthecrypt
left a comment
There was a problem hiding this comment.
Similar to the other codebase I think this overshoots in complexity, which is particularly troubling as tests are absent. Please try to pare down to minimum features needed and don't add forward thinking extension points or expose anything public that isn't proven necessary by people besides yourself.
| * | ||
| * @author Sjoerd Talsma | ||
| */ | ||
| public class CallableWithActiveSpan<T> implements Callable<T> { |
There was a problem hiding this comment.
does this need to be a public type? similar for the Runnable variant
| if (span == null) span = NoopSpan.INSTANCE; | ||
| return getInstance().setActiveSpan(span); | ||
| } catch (Exception activationException) { | ||
| LOGGER.log(Level.WARNING, "Could not activate {0}.", new Object[]{span, activationException}); |
There was a problem hiding this comment.
seems string cat is simpler here. there's questionable value in adding a period vs the cruft. ps did you mean to add the exception as a parameter?
| /** | ||
| * Managed object to deactivate an activated {@link Span} with. | ||
| */ | ||
| public interface SpanDeactivator { |
There was a problem hiding this comment.
why the marker interface. what does one expect to do with it?
|
the main thing I think you lose track of is that you are thinking somewhere possibly farther than others. It is possible that the end design will eventually become as complex, but since this is intended to be a shared project, I think it is better to focus code on only what's required and no more. Then, use things like the rule-of-three before making things public etc. Especially when doing fancy code like the lock-free loops, you really must have these things tested as this is the sort of code that breaks and is hard to tell why. Tests are a great way to tell if code should be present or not. For example, if you feel strongly that some hook is necessary, a feature test can explain that better than anyone's conjecture of importance. |
Maybe we should rename the git repository to java-spanmanager ?
|
@bensigelman Should we rename the repo to |
|
just to set expectations, I'm not planning to do further reviews on this, as between the global tracer and here, I've spent dozens of hours which delayed very important work I'm behind on. I hope my help so far has been.. helpful. |
bhs
left a comment
bhs
left a comment
There was a problem hiding this comment.
@sjoerdtalsma I want to make progress on this PR in the coming days if you have cycles for it.
FWIW, I would like OpenTracing APIs to change in 2017 in order to make things like this repo easier/better/cleaner. It's possible that something like this would actually get folded into OpenTracing proper, actually. I figured that getting into the weeds on this PR would be a good way to bring all of these issues to the fore.
I also wanted to mention an alternative: make a version number like 0.0.1, add a giant caveat to the top of the README, and just clean things up a little bit and merge something that's a working prototype more than a fully-debated PR. We would then look at how this works when integrated into real/mature repos and make changes as needed.
Thanks!
| * <li>Consecutive <code>release()</code> calls for already-released spans will be ignored.</li> | ||
| * </ol> | ||
| */ | ||
| final class ThreadLocalSpanManager implements SpanManager { |
There was a problem hiding this comment.
Do you have any practical examples of possible non-TLS SpanManagers that would actually play nice with the static-ness of this API? (I.e., that could retrieve the active span without parameters and without using TLS) Or is the idea just that they'd use some other TLS, like MDC or similar?
There was a problem hiding this comment.
Good question. The idea is that people can keep using their own context propagation mechanism they may already have, as that has impact on the code.
For instance, I created a security context propagation for a client that transfers to new background threads. I can easily create a SpanManager adaptor using that same propagation tool to also propagate/piggyback the spans besides other contextual information.
The impact is on the code scheduling the new threads which needs to be decoupled from whatever context must be propagated.
Whatever works for somebody should be extendable to also propagate spans with, hence the SpanManager interface.
I agree there needs to be cleanup and clarification. What is now the ActiveSpanManager is merely a 'global' placeholder similar to the 'global' tracer.
Should it be named GlobalSpanManager again? similar to GlobalTracer??
The concurrent package is meant to provide an example for propagating spans accross threads, but by no means is anyone obliged to use it.
I personally prefer an existing context propagation solution to which the SpanManager is registered as callback.
I have this concept working in a library, but it is currently not documented.
The tracer package is meant to automatically activate / deactivate spans to be propagated when started / finished.
Again, this is not obligatory, but in my opinion a very useful tool to separate the tracing from the propagation.
|
|
||
| ## Examples | ||
|
|
||
| _TODO create examples_ |
There was a problem hiding this comment.
This is arguably the most important / first thing we should do (even if there's no impl to back things up). It'd be great to talk about how the calling code is going to look and just collectively imagine impls, at least as long as we are able to imagine the same things.
Plus CONTRIBUTING.md and RELEASE.md documents.
|
@bensigelman Could you add me as a Collaborator to this repo? |
Also made the _work in progress_ caveat stand out more.
|
@sjoerdtalsma can you summarize where we are at with this PR? I see that you changed the README to reflect its early stage... are we just waiting for the release artifact machinery to stabilize? Thanks! |
|
|
@sjoerdtalsma I saw your previous note and gave you write access to the repo... |
|
... I bumped you up to Admin, that should hopefully be sufficient!
|
That did it! I enabled travis for this repo, thanks! |
|
The repository rename needs to happen before release, since all credentials are encrypted to a specific org/repo for travis. |
Re-encrypted travis credentials.
|
thanks @sjoerdtalsma!! |
4 participants
The java-globaltracer library has been split up in separate packages for active span management and global tracer access.
This pull request is for the active span management package into an independent separate library.