On Tue, Dec 16, 2014 at 10:48 AM, Andrew Stitcher <[email protected]> wrote: > > On Mon, 2014-12-15 at 11:51 -0500, Alan Conway wrote: > > 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. > > Woudl you like to move it or shall I? > > > > > 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. > > I think the issue of a new logging level (not that I'm against new > levels) and code without access to the transport object is unconnected. > I agree that we need some way to log from all parts of the code - I > would tend to introduce a log object though rather than make the > facility global. > > Also I really want there to be only one logging service in the library > so the previous transport attached logger should share the tracer with > the new facility.
I'm not sure if you're suggesting otherwise, but I do think it is valuable to be able to configure the logging (both the level and possibly the tracer also) on a per transport basis. I don't think this precludes sharing though and I agree that would be a good thing. --Rafael
