On Fri, 2014-12-12 at 15:09 -0500, Andrew Stitcher wrote:
> Alan recently landed a change which stops the proton library from
> writing directly to stderr/stdout - this is a good thing (IMO).
> However I question a couple of things:
> 1. This change adds to the external API and so should have been reviewed
> (again IMO). reviews.apache.org now does work with the proton git repo.
Agreed, my bad definitely should have been reviewed.
> 2. I don't think this change should be an external API at all - at least
> not initially, perhaps with some user pressure.
Agreed again. We may want to expose some form of logging as API but this
is probably to rushed. The purpose of this work was to allow users of
the proton library to control where protons logging goes, not to provide
a general purpose logging API for developers using proton.
> 3. There is already logging code triggered by PB_TRACE_FRM,
> PN_TRACE_RAW, PN_TRACE_DRV it would make sense for this to be integrated
> with the new work. Especially as this allows you to specify the tracer
> used (using pn_transport_set_tracer()).
It is sort-of supposed to be as far as it can be. The issue here is that
we already had a tracing mechanism *attached to the transport*. So code
that has access to a transport instance is already using it and didn't
need to be changed. Code that does _not_ have a transport instance was
just dumping on stderr (even code that was pretending to use the
transport tracing mechanism but was passing 0 for the transport pointer,
which just became a dump-to-stderr.) The new env var (PN_TRACE_LOG) was
intended to be "in the spirt" of the existing configurtion, but apply to
code points that don't have a transport and therefore need some "global
log" to dump to that does not reqiure a transport-attached tracer. The
global tracer is pretty much identical to the transport tracer except it
doesn't take a transport pointer.
Suggestions to improve this welcome, I blush for the lack of a review