Hi Brice, Thanks for resubmitting. We are wrapping up Windows support this week, and will be back reviewing patches soon.
Thanks again, Josh -- Join us for PuppetConf, September 22nd and 23rd in Portland, OR: http://bit.ly/puppetconfsig On Sun, Sep 18, 2011 at 6:55 AM, Brice Figureau < brice-pup...@daysofwonder.com> wrote: > Hi Josh, > > I know this was long overdue, but my own schedule couldn't fit any > puppet development until now. > I'm aware this will take you or the dev team a looong time to get back > into context :) > > Anyway, I started working on modifying the PIF to take into account your > comments, and my answers are interspersed into yours below. > > I'm also sending an official pull request at the same time. > Thanks for your time, > > On 17/06/11 01:10, Josh Cooper wrote: > > Hi Brice, > > > > Thanks for sending these patches, it's great stuff. I have some > > general comments first, and then comments about each commit below: > > > > GENERAL > > > > * I noticed places where a monitor was being created lazily, e.g. > > @last_logs ||= {}.extend(MonitorMixin). This is not thread safe, as > > two threads could create and synchronize on different objects. To be > > safe, I would create the last_logs hash in the initialize method or > > include the MonitorMixin in your class. > > Those were found most in the "example" code than in the framework > itself. I've taken care of it. > > > * There are a couple of places where access to a hash is > > synchronized, e.g. Instrumentation.listeners_of, but the hash is > > returned from the method call. As a result a caller may be iterating > > over it while another thread modifies it, resulting in undefined > > behavior. It would be safer to return a dup'ed hash. > > I changed the ones I could find to use a block (ie kind of visitor > pattern) instead of returning internal values. > > > * Have you considered how inheritance will affect instrumented > > methods? I wasn't able to produce a failure condition, but I'm > > concerned about an infinite loop if a super and sub class instrument > > the same method due to them both creating an "instrumented_method" > > alias. You may want to make the instrumented method name unique, e.g > > adding the klass object id to the method name? > > No, I didn't try anything specific in this respect. I'll consider using > the klass object id to decorate the instrumented method name, though the > current pull request don't have it. > > > COMMIT 1/8 > > > > lib/puppet/util/instrumentation.rb > > > > * The unsubscribe method is never called as far as I can tell. > > ... which doesn't mean it won't ever :) > > > * The listeners_of method should return a dup of the @listeners_of > > hash, otherwise, in the publish method, one thread could be iterating > > though the hash, while another thread, e.g. rest, attempts to > > concurrently modify it, e.g. saving a listener through the rest > > interface causes the rehash method to be called. > > I changed the semantic to a visitor pattern. > > > * Related to this, if a listener has been notified, there's nothing > > to prevent it from unsubscribing, or instrumenting additional methods > > (from within its notify). I'm pretty sure bad-things-would-happen. > > That's correct. I don't think it adds anything to sandbox the listeners. > Let's trust people writing listeners :) > > > * The subscribe method registers listener.name, but the unsubscribe > > uses listener.name.to_s > > Fixed. > > > * rehash assumes that the caller is holding the monitor while it > > clears the listeners_of hash. I'd recommend making it private. > > Fixed. > > > lib/puppet/util/instrumentation/listener.rb > > > > * Puppet.warnonce "Error during instrumentation notification: #{e}" > > > > This should just be: warnonce(...) > > Fixed. > > > spec/unit/util/instrumentation/listener_spec.rb > > > > * This could use more tests, e.g. calling unsubscribe, without first > > calling subscribe, calling subscribe multiple times, etc.. > > > > spec/unit/util/instrumentation_spec.rb > > > > * This should also test that listeners are not notified when literal > > and regex patterns don't match > > Both fixed (note your comments were interverted). > > > ----------------------------------------------------- > > > > COMMIT 2/8 > > > > lib/puppet/indirector/instrumentation_data.rb > > lib/puppet/indirector/instrumentation_data/local.rb > > lib/puppet/indirector/instrumentation_data/rest.rb > > lib/puppet/indirector/instrumentation_listener.rb > > lib/puppet/indirector/instrumentation_listener/local.rb > > lib/puppet/indirector/instrumentation_listener/rest.rb > > lib/puppet/util/instrumentation/data.rb > > > > * I had a lot of trouble getting this to work, partly because it's > > new to me and also because of the whole plurality business. But we > > could use better error messages. For example, when trying to search > > for instrumentation data I tried the following > > > > curl -k -H 'Accept: pson' -X GET "https://localhost:8140/production/ > > instrumentation_data/all" > > Could not render to pson: undefined method `name' for nil:NilClass > > > > I instead had to do ".../instrumentation_data_search/all" > > The commit message might have been wrong. I think you need to > "pluralize" instrumentation_data to instrumentation_datas. > Reading the API code, we apparently also support _search now for just > the cases where pluralization by adding 's' doesn't work :) > > > * Also, if the listener does not provide data, e.g. process_name: > > > > curl -k -H 'Accept: pson' -X GET "https://localhost:8140/production/ > > instrumentation_data/process_name" > > Could not render to pson: undefined method `data' for > > #<Puppet::Util::Instrumentation::Process_name:0x1024bca28> > > Fixed > > > * I'm seeing data leakage when "save"ing the listener > > > > curl -k -H 'Accept: pson' -X PUT -H "Content-Type: text/pson" -d > > "{\"document_type\":\"Puppet::Util::Instrumentation::Listener\",\"data > > \":{\"enabled\":true,\"name\":\"log\"}}" "https://localhost:8140/ > > production/instrumentation_listener/log" > > --- !ruby/object:Puppet::Util::Instrumentation::Listener > > enabled: true > > listener: !ruby/object:Puppet::Util::Instrumentation::Log > > last_logs: > > save_instrumentation_listener_local: > > - save_instrumentation_listener_local took 0.000217 > > Fixed. > > > ----------------------------------------------------- > > > > COMMIT 3/8 > > > > lib/puppet/util/instrumentation/Instrumentable.rb > > > > * The disable method does not restore the original method. The order > > of the parameters to the alias_method needs to be reversed. > > Oops :) > Fixed > > > * Need to lowercase the filename > > Fixed > > > * Do the enable and disable methods really need to create local > > variables, e.g. method = @method? We definitely don't need label and > > data variables in the disable method. > > It simpler to use method in the class_eval block than the outlining > Instrumentable object instance variable @method. > You're correct data and label are unneeded. > > > spec/unit/util/instrumentation/instrumentable_spec.rb > > > > * The spec should really call the various instrumented methods (as > > opposed to seeing if they respond to) after the probe is enabled and > > after it has been disabled, to ensure that the "disable" method really > > works. > > Fixed. > > > ----------------------------------------------------- > > > > COMMIT 4/8 > > > > lib/puppet/util/instrumentation/listeners/log.rb > > > > * There's a race condition creating the @last_log object, which is > > later used to synchronize on. > > > > * The data method returns the actual last_logs array, not a dup, but > > as the caller is iterating the array, another event could be > > delivered. Note the :performance listener does perform a dup within a > > synchronized block when returning its data. > > Both are fixed. > > > spec/unit/util/instrumentation/listeners/log_spec.rb > > > > * Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist? > > (f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/ > > spec_helper.rb") } > > > > can be changed to just: > > > > require 'spec/spec_helper' > > Fixed. > > > ----------------------------------------------------- > > > > COMMIT 5/8 > > > > lib/puppet/util/instrumentation/listeners/performance.rb > > > > * Same comment about synchronizing on a mutable samples hash. > > Fixed > > > * If the duration is always greater than 32768, then min will > > incorrectly be 32768 > > Fixed > > > spec/unit/util/instrumentation/listeners/performance_spec.rb > > > > * Same comment about require 'spec/spec_helper' > > Fixed > > > ----------------------------------------------------- > > > > COMMIT 6/8 > > > > lib/puppet/util/instrumentation/listeners/process_name.rb > > > > * @oldname is set, but never used > > > > * Same comment about synchronizing on a mutable @mutex > > > > * The scroller thread should be stopped if the listener is disabled > > > > spec/unit/util/instrumentation/listeners/process_name_spec.rb > > > > * require 'spec/spec_helper' > > I reworked the process name listener and this should fix all these > concerns. > > > ----------------------------------------------------- > > > > COMMIT 7/8 > > > > lib/puppet/indirector/instrumentation_probe.rb > > lib/puppet/indirector/instrumentation_probe/local.rb > > lib/puppet/indirector/instrumentation_probe/rest.rb > > > > * Getting a list of "probes" doesn't exactly work, due to rest > > interface pluralizing (something to just document when telling people > > how to use this feature): > > > > curl -k -H 'Accept: pson' -X GET -H "Content-Type: text/pson" -d > > "{}" "https://localhost:8140/production/instrumentation_probes/all"; > > Forbidden request: localhost(127.0.0.1) access to / > > instrumentation_prob/all [search] at line 105 > > > > Have to say .../instrumentation_probe_search/all instead. > > I'll update the commit message. > > > lib/puppet/util/instrumentation/indirection_probe.rb > > > > * :docuemnt_type misspelled > > Fixed. > > > spec/unit/indirector/instrumentation_probe/local_spec.rb > > spec/unit/indirector/instrumentation_probe/rest_spec.rb > > spec/unit/util/instrumentation/indirection_probe_spec.rb > > No comments for those? > > > ----------------------------------------------------- > > > > COMMIT 8/8 > > > > lib/puppet/indirector/indirection.rb > > > > * no comments > > Hey, that's cool! > -- > Brice Figureau > My Blog: http://www.masterzen.fr/ > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to puppet-dev@googlegroups.com. > To unsubscribe from this group, send email to > puppet-dev+unsubscr...@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to puppet-dev@googlegroups.com. To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
