On Tue, Dec 16, 2014 at 10:48 AM, Andrew Stitcher <astitc...@redhat.com>
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

Reply via email to