Configuration and status of reporterImpl via the Tracer object

[MarkKMueller]

The reportSpan member function of reporterImpl will drop spans if the span buffer is full yet there currently is no way for a client to know when this happens. Additionally, there is currently no way to change the max_buffered_spans or reporting_period parameters, yet some implementations may need to adjust these for performance and reliability reasons.

I'd like to propose the following changes as a means to address these issues and can submit a PRs if this proposal is acceptable (maybe 3 PRs, 1 for configuration, 1 for status, and 1 for the change to makeZipkinOtTracer?):

1. Add member functions to the reporterImpl class, and pass through functions to Tracer, to get and set the max_buffered_spans and reporting_period parameters and change them to be members of the reporterImpl class
2. Change the reportSpan member function to maintain a count of the number of dropped spans
3. Add a member function to the reporterImpl class, and a pass through function to Tracer, which will return the number of dropped spans and the length of time over which the count is applicable.
4. Add an optional third parameter to the makeZipkinOtTracer function which, if provided, will return a pointer to a shared pointer of the Zipkin Tracer object that will provide external access to these new status and configuration functions.

I've left out many of the implementation details, but do have a working model of the above which I have also done testing on. Does this sound reasonable?

[rnburn]

Great! I'd welcome PRs to support customizing the configuration more and adding dropped span metrics.

A couple of thoughts on the proposal:

1. Add member functions to the reporterImpl class, and pass through functions to Tracer, to get and set the max_buffered_spans and reporting_period parameters and change them to be members of the reporterImpl class

Could we instead pass in a configuration object and have the options specified there. This would be more consistent with how zipkin-go-opentracing does things.

4. Add an optional third parameter to the makeZipkinOtTracer function which, if provided, will return a pointer to a shared pointer of the Zipkin Tracer object that will provide external access to these new status and configuration functions.

If the ZipkinTracer has additional functionality, can we just return std::shared_ptr<ZipkinTracer> instead of adding an optional argument. You can always cast it to an std::shared_ptr<opentracing::Tracer>.

[MarkKMueller]

Could we instead pass in a configuration object and have the options specified there. This would be more consistent with how zipkin-go-opentracing does things.

I chose this method to allow for dynamic run time adjustment of the configuration. In some scenarios these values are unknown at initialization time and need to be adjusted when things reach a steady state, or when some part of the system changes. I see the merit and simplicity in passing parameters via ZipkinOtTracerOptions. Is it reasonable to support both?

If the ZipkinTracer has additional functionality, can we just return std::shared_ptr instead of adding an optional argument. You can always cast it to an std::shared_ptr.

The optional argument was for backwards compatibility. Returning a ZipkinTracer is OK by me. I don't see how an opentracing::Tracer can be cast to a ZipkinTracer though. ZipkinTracer is not derived from an opentracing::Tracer. OtTracer is derived from opentracing::Tracer, but it's not visible anywhere. I could add a pointer to opentracing::Tracer in the ZipkinTracer and a function to retrieve it. Should I do that or can you provide more info on how to cast from a ZipkinTracer to an ot::Tracer?

[rnburn]

I chose this method to allow for dynamic run time adjustment of the configuration. In some scenarios these values are unknown at initialization time and need to be adjusted when things reach a steady state, or when some part of the system changes. I see the merit and simplicity in passing parameters via ZipkinOtTracerOptions. Is it reasonable to support both?

Fair enough. We did something similar with the LightStep tracer where we allowed configuration values to also be functions that could be changed at runtime (example, max_buffered_spans). But since there's no precedent for runtime-adjusted configuration values in zipkin tracers AFAIK, I'm ok if you want to go with a different approach.

The optional argument was for backwards compatibility. Returning a ZipkinTracer is OK by me. I don't see how an opentracing::Tracer can be cast to a ZipkinTracer though. ZipkinTracer is not derived from an opentracing::Tracer. OtTracer is derived from opentracing::Tracer, but it's not visible anywhere. I could add a pointer to opentracing::Tracer in the ZipkinTracer and a function to retrieve it. Should I do that or can you provide more info on how to cast from a ZipkinTracer to an ot::Tracer?

Yeah, sorry. I meant return std::shared_ptr and make it visible if it has extensions to the standard opentracing api. I'd prefer the minor break in backwards compatibility to the weirdness of an optional third parameter.

[MarkKMueller]

OK. I'm just about ready to move forward. Everything is currently one commit but I can rebase -i that however needed, and do multiple PRs. Are there any recommendations for how to break this up? I could do the instantiation config parameters as one change, expose the OtTracer as a second, and everything else as the third. If you've got some guidelines, I can do my best to meet those

[rnburn]

I could do the instantiation config parameters as one change, expose the OtTracer as a second, and everything else as the third.

That sounds good.