Initial version of the OpenTracing Metrics Java project

[objectiser]

@pavolloffay @jpkrohling Updated the previous prototype with additional config for metric type names - add headers, class/method comments. Would be good if you could take another quick look.

cc @yurishkuro @bhs @tedsuo

[pavolloffay]

We should really start thinking about observable for span. It would make it much cleaner and simpler.

[objectiser]

@pavolloffay Thanks for the comments, will have a look soon. There is an issue already underway to create an oberver https://github.com/opentracing/specification/issues/76 - once that is completed then the metrics mechanism will switch over to use it. The current wrapper is internal so not exposed to users, to hide this transition.

[pavolloffay]

I'm also wondering whether is it necessary to wrap all active span stuff. If you wrap finish you should perhaps know what span has been finished.

[objectiser]

@pavolloffay Most of the changes have been applied, apart from the ones I've commented on.

Regarding the comment about wrapping finish - the problem is that some Tracer methods only return the ActiveSpan so there is no way to get access to the underlying Span that may be internally managed by the tracer. Same with the ActiveSpan returns by the Continuation.

[pavolloffay]

I think you could do something like:

        @Override
        public ActiveSpan startActive() {
            Span span = startManual(); //wrapped metrics span
            return wrappedTracer.makeActive(span);
        }

[objectiser]

@pavolloffay That would only work for the startActive - there is also Tracer.activeSpan, Continuation.activate where we only have access to the ActiveSpan returned by the tracer.

[pavolloffay]

@pavolloffay That would only work for the startActive - there is also Tracer.activeSpan, Continuation.activate where we only have access to the ActiveSpan returned by the tracer.

In this cases, there is already span associated with mentioned invocation so it should be fine.

[objectiser]

Not sure if I am understanding - possibly once this PR has been merged you could try to simplify it, but don't really want to spend much time on the wrapper as it will be replaced hopefully in the near future.

[objectiser]

@pavolloffay Have used your suggestion for just wrapping the span, because the previous approach assumed the SpanContext impl provided equals/hashCode - which is not the case with many/all of the tracers (including mock tracer). However this new approach has the downside that it expects the ActiveSpanSource.makeActive to accept any span - which is fine for any tracer using the ThreadLocalActiveSpanSource, but won't work with tracers that expect their own span impl (e.g. Brave).

As this wrapper approach is only temporary, until an observer solution is available, this approach is better as it does not impose any requirements on many existing tracers - but unfortunately won't work with zipkin/brave currently.

[pavolloffay]

However this new approach has the downside that it expects the ActiveSpanSource.makeActive to accept any span - which is fine for any tracer using the ThreadLocalActiveSpanSource, but won't work with tracers that expect their own span impl (e.g. Brave).

I don't get it why it shouldn't work, ActiveSpanSource accepts span https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/ActiveSpanSource.java#L49 and Tracer#makeActive too... Brave is zipkin specific, are you talking about brave-opentracing?

[objectiser]

Yes, brave-opentracing.

[pavolloffay]

I don't see the problem there as the proposed approach calls public OT APIs. Could you please explain more?

[objectiser]

Easier to show: https://github.com/jakerobb/brave-opentracing/blob/ec1c3814104ddb03b9f75ac7b9d526b010dd6288/src/main/java/brave/opentracing/BraveActiveSpanSource.java#L56

If the opentracing API is a wrapper around a non-OT tracer, and that tracer impl uses its own span management mechanism, then it would also wrap that span management mechanism with the ActiveSpanSource API. Not saying this approach is right or wrong, but just highlighting the issue.

As said though, this should only be a short term workaround, so not sure it is a big deal.

[pavolloffay]

Easier to show: https://github.com/jakerobb/brave-opentracing/blob/ec1c3814104ddb03b9f75ac7b9d526b010dd6288/src/main/java/brave/opentracing/BraveActiveSpanSource.java#L56

I think the problem might be with their code. We might introduce NoSpan or something similar and it would break..

[objectiser]

@pavolloffay @jpkrohling Is it good enough to merge now? Would like to get an initial release out soon.

[objectiser]

@pavolloffay @jpkrohling Thanks!

[bhs]

FWIW, I am excited about this! Thanks @objectiser!