Re: PROTON-772: New proton logging code
On Tue, 2014-12-16 at 10:48 -0500, Andrew Stitcher 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? Done: commit a72202d4f4c02221c6555a9e76480d383c7398c8 Author: Alan Conway acon...@redhat.com Commit: Alan Conway acon...@redhat.com PROTON-772: New proton logging code Based on list discussion: - moved the log statement api into src/log_private - simplified the public API to 2 functions: pn_log_enable and pn_log_logger The minimal public API is required to let applications using proton control where proton log output goes, which is the point of all this. The present state of the logging achieves the following goals: - allow applications to disable or redirect ALL proton library output, nothing is forced to stderr. - leaves the previous behavior of transport tracers unaffected. - introduces as little disturbance as possible (one new env. var PN_TRACE_LOG and 2 new API functions) This is definitely not the final word on logging for proton, but a more sophisticated, unified logging system requires more discussion in the proton community. 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. Proton has no library init function that is required to be called before using proton objects, and there's no root to the proton object tree so I had trouble figuring out where to put such a thing. I agree it would be nicer if we could find a place to attach a logging object. 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. Agree with the sentiment but again I couldn't figure out where to put it. The transport-attached tracer API allows the app to attach arbitrary function calls to each transport, so without changing that behavior what can you centralize? I did make tracer calls with transport=0 go to the global log so that is centralized sort-of. What I've done so far is *definitely* not the last word but it is a step in that it a) stops the barfing on stderr and b) doesn't break anything else. Thanks for making me clean it up so at least there's minimal public API to change when we do come up with something more comprehensive. Cheers, Alan.
Re: PROTON-772: New proton logging code
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. Suggestions to improve this welcome, I blush for the lack of a review request. Andrew
Re: PROTON-772: New proton logging code
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
Re: PROTON-772: New proton logging code
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 request. Cheers, Alan.
