opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
505 stars 56 forks source link

[#21] Adds mock tracer. #46

Closed jcchavezs closed 6 years ago

jcchavezs commented 6 years ago

Short description of what this PR does:

Checklist

Ping @yurishkuro @tedsuo @felixfbecker @beberlei

Closes #21

beberlei commented 6 years ago

Why is it named mock tracer? it doesnt really mock or? it really works.

jcchavezs commented 6 years ago

Basically following the naming from other language's libraries. The main advantage of this tracer is that it is inspectable, you can access to all the attributes of a span.

beberlei commented 6 years ago

@jcchavezs ok that makes sense then.

A few subjective technical nitpickeries:

  1. I would prefer a namespace OpenTracking\MockTracer instead of OpenTracingMock. The first namespace is usually the vendor and adding another namespace entry to the composer list has a small perf penalty for something that nobody will use in their production code.

  2. Naming the classes Span and implenents OTSpan with use statement rename is a bad practice imho, because it creates a second item in PHPStorm called Span and people sometimes make mistakes importing the wrong ones. If the class would be called MockSpan this wouldn't happen.

  3. you mentioned this is needed for testing, but you have added no tests using the mock tracer, is this intentional and only needed for #45 after merge?

Otherwise +1

jcchavezs commented 6 years ago

Thanks for the great feedback, Ben.

  1. I am OK to move to the OpenTracing\MockTracer namespace but I think the current setup having OpenTracingMock in require-dev avoid the performance penalty. Am I missing something?
  2. I think that is the whole idea about using namespaces, to me having OpenTracing\MockTracer\MockSpan looks too much mock. Is there any other approach we could sort this out by?
  3. My bad, there are some stuff missing here: tests for tracer and also injection and extraction. I added the WIP in the title so it is clear.

Again, great feedback!

beberlei commented 6 years ago

@jcchavezs you are right about require-dev, sorry i didnt see that.

I don't agree with "too much mock", you must see it in context of being use'd once at the top so one "Mock" in the namespace name is already not visible anymore. and then if you are using Span in your testcode, its not clear if its the interface or the mock span. From a user perspective of this class i prefer to be clearer.

ellisv commented 6 years ago

@jcchavezs But if you leave this under autoload-dev other libraries that require this package and want to use those mocks on their test suite will not be able to do so as they will not be autoloadable from their point of view. Or that is not a goal of those mocks?

jcchavezs commented 6 years ago

Right @ellisv I will change that.

jcchavezs commented 6 years ago

I just uploaded all the code and removed the wip tag. Please have a look.

Ping @beberlei @StymiedSloth @masterada @ellisv @yurishkuro

jcchavezs commented 6 years ago

Please final review @beberlei @StymiedSloth @masterada @ellisv @yurishkuro