Hi Markus,
On 27/02/11 19:54, Markus Roberts wrote:
> B --
>
> Thoughts:
>
> 1) Wow.
>
>> + def enable
>
> + raise "Probe already enabled" if enabled?
> + method = @method
> + label = @label
> + data = @data
> + klass.class_eval {
> + alias_method("instrumented_#{method}", method)
> + define_method(method) do |*args|
> + begin
> + instrumentation_label = label.respond_to?(:call) ?
> label.call(self, args) : label
> + instrumentation_data = data.respond_to?(:call) ?
> data.call(self, args) : data
> +
> Puppet::Util::Instrumentation.start(instrumentation_label,
> instrumentation_data)
> + send("instrumented_#{method}".to_sym, *args)
> + rescue => e
> + raise
> + ensure
> + Puppet::Util::Instrumentation.stop(instrumentation_label)
> + end
> + end
> + }
> + @enabled = true
> + end
>
>
> 2) Would you get a more useful stack-trace without the rescue?
Actually the first version didn't had the rescue, but I had some issues
and I wanted to print the exception, it apparently escaped the garbage
collection.
Now, I thought that a straight raise would rethrow the exception without
touching it, or am I wrong?
In any case, this is easy to change :)
Now that you talk about, I should begin/rescue the instrumentation code
on front and in the ensure, because the thing we really don't want is to
crash puppet with the instrumentation code :)
> 3) And is there a race condition on @enabled?
Theorically yes, but in this scheme you can only enable all probes or
disable them all at once. And this looks protected.
>
> + def disable
> + raise "Probe is not enabled" unless enabled?
> + method = @method
> + label = @label
> + data = @data
> + klass.class_eval do
> + alias_method("instrumented_#{method}", method)
> + remove_method("instrumented_#{method}".to_sym)
> + end
> + end
>
>
> 4) Should you set @enabled = false somewhere in here?
Yes, I should :)
> + def self.clear_probes
> + INSTRUMENTED_CLASSES.synchronize {
> + INSTRUMENTED_CLASSES.clear
> + }
> + end
>
>
> 5) Do you want a:
>
> nil # do not leak our probes to the exterior world
>
> on clear_probes as well as on each_probes?
Actually, clear_probes is only called in test, but you're right.
And there's some other place (like the listeners) that also leak some
internal instances to the indirector.
Thanks for your comments!
--
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.
