Provide a tracer wrapper that can be used as basis for adding ex… #9

objectiser · 2017-07-20T07:45:18Z

…tension APIs (e.g. Observer) implementations

objectiser · 2017-07-20T07:46:54Z

@jpkrohling @pavolloffay Can you review please?

…dual tests

jpkrohling · 2017-07-21T08:41:11Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+    public Span setOperationName(String operationName) {
+        wrappedSpan.setOperationName(operationName);
+        this.operationName = operationName;
+        if (!observers.isEmpty()) {


Is this check needed? Won't the for loop just be skipped anyway?

Was to avoid creating the Iterator - although the only purpose of this wrapper (currently) is to manage observers - so redundant at the moment. So will remove.

jpkrohling · 2017-07-21T08:42:06Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+     */
+    public APIExtensionsSpan(Span span, String operationName,
+            long startTimestampMicro, long startTimeNano, Map<String,Object> tags) {
+        this.wrappedSpan = span;


Would be nice to add some sanity checks here, like a null check for the span. Otherwise, it might fail later on with a NPE.

I'll change it to package private as should only be used from the APIExtensionsSpanBuilder, which uses startManual which must return a Span.

jpkrohling · 2017-07-21T08:42:41Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+    @Override
+    public Span setBaggageItem(String name, String value) {
+        wrappedSpan.setBaggageItem(name, value);
+        if (!observers.isEmpty()) {


Same as before: is this check needed?

(there are other places like this, so, if you do change this, the other places might need changing as well)

jpkrohling · 2017-07-21T08:46:19Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+    @Override
+    public Span log(Map<String, ?> fields) {
+        wrappedSpan.log(fields);
+        return handleLog(TimeUnit.MILLISECONDS.toMicros(System.currentTimeMillis()), fields);


Could you replace the System.currentTimeMillis with a Clock? This way, you don't lose the micro precision (not to mention that it's far easier to test with Clock)

If you do change this to use a Clock, there are other places that would need to be changed as well

Unfortunately Clock is 1.8, and we are trying to remain 1.6 compatible for now. Hopefully that decision won't last forever.

jpkrohling · 2017-07-21T08:48:50Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+    @Override
+    public String getStringTag(String key) {
+        Object value = tags.get(key);
+        if (value instanceof String) {


I'd rather to a toString() instead.

Was discussed in the PR related to the API, and decided that non-string tags should not be returned as strings, as null is used as a way to understand it is not a string type.

jpkrohling · 2017-07-21T08:51:46Z

...ensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpanBuilder.java

+    private final String operationName;
+    private final SpanBuilder wrappedBuilder;
+    private long startTimestampMicro = TimeUnit.MILLISECONDS.toMicros(System.currentTimeMillis());
+    private long startTimeNano = System.nanoTime();


Why do you have a nano and a micro?

Micro is for the timestamp - nano is for the elapsed time, to provide more accurate result.

jpkrohling · 2017-07-21T08:53:49Z

...ensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpanBuilder.java

+    public APIExtensionsSpanBuilder(Tracer tracer, List<TracerObserver> observers,
+            String operationName, SpanBuilder builder) {
+        this.tracer = tracer;
+        this.observers = observers;


You are letting the caller retain access to this list. What would happen if the caller changes this list down the road? Is that OK, or would it be better to clone the list?

Also, it might be worth doing some sanity checks (null checks) as well.

As with previous comment - I've now made this package private and is only used by the APIExtensionsTracer which owns the list.

I suppose that still leaves the potential for the app to add/remove an observer which would impact inflight spans. So probably better to clone to make sure the observer sees the full lifecycle of the span being observed.

Sorry, actually it won't be a problem - the list is only used at the point where the span is started, at which point it obtain a SpanObserver which will monitor the complete lifecycle of the span. As said before the list is maintained by the extension tracer - the app could add/remove observers at any point - but essentially at the point the span is started, it has a snapshot of the observer list at that point in time.

jpkrohling · 2017-07-21T08:54:48Z

...ensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpanBuilder.java

+    }
+
+    Map<String, Object> tags() {
+        return tags;


You are giving the caller access to your internal map. Is that intentional?

This is for test access only.

@objectiser Can't you fine another way to test, without exposing internals ?
I think the question is justified why this method exists.

jpkrohling · 2017-07-21T08:55:22Z

...i-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsTracer.java

+    private final Tracer wrappedTracer;
+    private final List<TracerObserver> observers = new CopyOnWriteArrayList<TracerObserver>();
+
+    public APIExtensionsTracer(Tracer tracer) {


How about providing a constructor that tries to get a Tracer from the GlobalTracer?

Same as above - prefer to leave to app for now.

@objectiser I agree, I think most usecases for wrapping with API extensions will be from application initialization code just before setting the GlobalTracer.

Also, you're not preventing the caller from supplying the GlobalTracer anyway if he/she does have such a usecase.

jpkrohling · 2017-07-21T08:58:12Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+    @Override
+    public Number getNumberTag(String key) {
+        Object value = tags.get(key);
+        if (value instanceof Number) {


If I try to get a "int" from a string value, I would expect an exception, not null (at least, that's what happen in other APIs out there). If returning null is indeed what you think your callers would expect (or want), then please add a javadoc about it (sorry if it's added somewhere already)

https://github.com/opentracing-contrib/java-api-extensions/blob/master/opentracing-api-extensions/src/main/java/io/opentracing/contrib/api/SpanData.java#L87
If you think the API should be different, then best to raise an issue so can be discussed.

… remove check for empty observer list

objectiser · 2017-07-21T12:58:33Z

@jpkrohling Thanks!

pavolloffay · 2017-07-27T12:16:47Z

...api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpan.java

+    @Override
+    public long getDuration() {
+        // If start or finish nano times are not available, then use timestamps
+        if (startTimeNano == 0 || finishTimeNano == 0) {


When this can be 0?

https://github.com/opentracing-contrib/java-api-extensions/blob/master/opentracing-api-extensions-tracer/src/main/java/io/opentracing/contrib/api/tracer/APIExtensionsSpanBuilder.java#L96-L98

objectiser · 2017-08-10T15:37:04Z

closes #5

WIP: Provide a tracer wrapper that can be used as basis for adding ex…

8a17e75

…tension APIs (e.g. Observer) implementations

objectiser added 2 commits July 20, 2017 08:49

Fix version

d5f1093

Factored out creation of mocks and extensions SpanBuilder from indivi…

af30b2d

…dual tests

objectiser changed the title ~~WIP: Provide a tracer wrapper that can be used as basis for adding ex…~~ Provide a tracer wrapper that can be used as basis for adding ex… Jul 21, 2017

jpkrohling reviewed Jul 21, 2017

View reviewed changes

Update following review comments - make constructors package private,…

f03c2ef

… remove check for empty observer list

jpkrohling approved these changes Jul 21, 2017

View reviewed changes

objectiser merged commit b580eb7 into opentracing-contrib:master Jul 21, 2017

objectiser deleted the tracer branch July 21, 2017 12:59

pavolloffay reviewed Jul 27, 2017

View reviewed changes

objectiser mentioned this pull request Aug 11, 2017

Change access to tags from package private to public with readonly ac… #13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a tracer wrapper that can be used as basis for adding ex… #9

Provide a tracer wrapper that can be used as basis for adding ex… #9

objectiser commented Jul 20, 2017

objectiser commented Jul 20, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

jpkrohling Jul 21, 2017

jpkrohling Jul 21, 2017

jpkrohling Jul 21, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

objectiser Jul 21, 2017

objectiser Jul 21, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

sjoerdtalsma Aug 10, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

sjoerdtalsma Aug 10, 2017

jpkrohling Jul 21, 2017

objectiser Jul 21, 2017

objectiser commented Jul 21, 2017

pavolloffay Jul 27, 2017

objectiser Jul 27, 2017

objectiser commented Aug 10, 2017

Provide a tracer wrapper that can be used as basis for adding ex… #9

Provide a tracer wrapper that can be used as basis for adding ex… #9

Conversation

objectiser commented Jul 20, 2017

objectiser commented Jul 20, 2017

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

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

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

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

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

objectiser commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Aug 10, 2017