Skip to content

Conversation

@rossdanderson
Copy link
Contributor

@rossdanderson rossdanderson commented Dec 14, 2019

I've added TracingSubscriber (implementing FlowableSubscriber) which covers use cases where you need support for managing back-pressure.
As part of this I have renamed AbstractTracingObserver to RxTracer and made it a field of the implementations rather than inheriting it, which means we can use the same code for the TracingSubscriber as well as the TracingObserver - I've checked that this does not affect the public API as this was already package private.

I have had to make a change to the design for TracingSubscriber in how it is created, by adding static create methods for the various possibilities matching Flowable.subscribe(...). This is mostly because I don't really understand the purpose of TracingConsumer and I wouldn't know what to call a Subscriber equivalent to this (they also use Consumers underneath so it could get confusing!).

An example of wrapping an existing subscriber currently looks like:

        Flowable<Integer> flowable = createSequentialFlowable(mockTracer);
        TestSubscriber<Integer> subscriber = new TestSubscriber<>();
        flowable.subscribe(TracingSubscriber.create(subscriber, "sequential", mockTracer));

and an example of creating a TracingSubscriber directly:

        Flowable<Integer> flowable = createSequentialFlowable(mockTracer).subscribe(
                TracingSubscriber.create(o -> { System.out.println(o); }, "sequential", mockTracer)
        );

Unless I am missing the reason for it's existence, I would consider deprecating/removing TracingConsumer and replacing it with static create methods on TracingObserver.

If you have suggestions, please let me know!

@coveralls
Copy link

Pull Request Test Coverage Report for Build 94

  • 24 of 41 (58.54%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.7%) to 62.824%

Changes Missing Coverage Covered Lines Changed/Added Lines %
opentracing-rxjava-2/src/main/java/io/opentracing/rxjava2/TracingConsumer.java 3 4 75.0%
opentracing-rxjava-2/src/main/java/io/opentracing/rxjava2/TracingObserver.java 3 4 75.0%
opentracing-rxjava-2/src/main/java/io/opentracing/rxjava2/TracingSubscriber.java 16 31 51.61%
Files with Coverage Reduction New Missed Lines %
opentracing-rxjava-2/src/main/java/io/opentracing/rxjava2/TracingConsumer.java 1 73.53%
Totals Coverage Status
Change from base Build 93: -0.7%
Covered Lines: 218
Relevant Lines: 347

💛 - Coveralls

@malafeev malafeev merged commit c740359 into opentracing-contrib:master Dec 16, 2019
@malafeev malafeev mentioned this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants