opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Is `final class` necessary? #330

Open safris opened 5 years ago

safris commented 5 years ago

Dear OpenTracing-Mock developers,

I am attempting to wrap the MockTracer with higher-level logic, while preserving the full scope of functionality offered by the MockTracer itself.

I am not able to achieve this, however, due to final class spec on: MockSpan, MockSpan.MockContext, and MockTracer.SpanBuilder.

Before I request to drop the final, I want to make sure I'm not missing something. Is there an underlying reason for the final class spec?

yurishkuro commented 5 years ago

The usual reasons: removing final extends the API surface, making future maintenance and refactoring potentially breaking changes.

Maybe you can explain what kind of higher level logic you want to add. Since it's a mock class, maybe those changes can be upstreamed.

safris commented 5 years ago

Ok, I see. The reason I'd like to inherit from MockTracer is this:

The SpecialAgent project has a JUnit runner named AgentRunner that injects an instance of MockTracer to each test method (example). The MockTracer is the OpenTracing standard for JUnit tests (I believe), so it is reasonable to couple this standard into the JUnit tests that run with AgentRunner (as opposed to creating a replica of MockTracer in the SpecialAgent project itself).

We have a requirement to be able to test the AgentRunner JUnit tests against a real tracer, as opposed to just the MockTracer. Since the AgentRunner JUnit tests are coupled to the MockTracer (as they should be, I believe), it would be necessary to rewrite each AgentRunner JUnit test to accept the Tracer interface instead of the MockTracer implementation specifically. Doing so would allow us to inject real tracers instead of the MockTracer for each test. But, the tests rely on assertions made against the MockTracer's MockTracer#finishedSpans() API, which would then have to be redone in some other way, effectively leading this "other way" to be a reimplementation of the MockTracer to provide an "extensible nature".

To avoid reimplementing the MockTracer, I would like to be able to extend the MockTracer itself. Doing so would allow me to create a "ProxyMockTracer", where I would be able to "filter/pipe/tee" API calls through the MockTracer's API, and to any other Tracer implementation. This would allow a seamless integration of real Tracer providers to AgentRunner JUnit tests.

yurishkuro commented 5 years ago

Read your explanation twice, but still not sure what the issue is. If the tracer API required by your unit tests is more than OT, could you not simply use MockTracer via composition? It sounds like you'd need to do that anyway with a real tracer in order to provide an equivalent of finishedSpans() accessor (since it's not just the method itself, but the data model it returns as well that becomes your dependency).

safris commented 5 years ago

To accomplish my goal, I need to create a “Proxy*” version of every class in the OT API, on top of the MockTracer’s implementation of the API interfaces. Only this way, will a ProxyMockTracer be able to meet the full scope of functionality of the MockTracer as well as the Tracer being “proxied”.

yurishkuro commented 5 years ago

My question is: don't you need to do that anyway? How else can you make a real tracer satisfy the mock tracer API?

Incidentally, this could be helpful https://github.com/opentracing-contrib/java-api-extensions/

safris commented 5 years ago

I'm not quite following what you're referring to with "don't you need to do that anyway". But thank you for the reference to java-api-extensions -- I did not know this existed. Though it's useful to be aware of, it still does not fully meet our requirements.

Apologies if my explanation of my use-case is not great. I'll try to itemize the individual points:

1) As in the example, we want the method signature to require the MockTracer (specifically -- i.e., we want the test classes to refer to MockTracer, and not to APIExtensionTracer, or even a custom tracer that I can create named ProxyTracer, or something like that). 2) Thereafter, we want to allow the MockTracer to do more than what it was designed to do: i.e., we want the AgentRunner runtime to instantiate a MockTracer subclass that: (a) does everything that the MockTracer already does, and (b) also proxies all method calls to another Tracer that is passed to this MockTracer subclass by the AgentRunner during initialization.

As far as I can see, this is not possible with the current class model of the MockTracer, because of final class spec on MockSpan, MockSpan.MockContext, and MockTracer.SpanBuilder.

Please correct my thinking here if I'm overseeing something. I attempted to implement this ProxyMockTracer subclass of MockTracer, and got stuck at the final class spots. Hence this issue.

yurishkuro commented 5 years ago

(b) also proxies all method calls to another Tracer that is passed to this MockTracer subclass by the AgentRunner during initialization.

^ this is the part I was referring to. It sounds like you want MockTracer subclass to act as a wrapper around another real tracer (just like api-extensions thing does). Can you show example of what that looks like? E.g. what happens when the agent/instrumentation calls tracer.startSpan() ?

safris commented 5 years ago

@yurishkuro, I have added a comment for you in the ProxyMockTracer branch of the SpecialAgent project. Looking forward to your reply.

safris commented 5 years ago

@yurishkuro, I had left a comment for you in the ProxyMockTracer branch of the SpecialAgent project. If possible, I'd like to move forward in one way or another, depending on your feedback.