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 [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
