[PATCH/puppet 1/1] 2842 Format debugging should be removed

[Jesse Wolfe]

There's just something about this message that looks like an error:
debug: Format s not supported for Puppet::FileServing::Metadata; has not implemented method 'from_s'

Not supported! Not implemented! It raises red flags, even though the
message itself is gray.
Generally this message doesn't come up when you're asking for that
specific format, but rather when you're just asking for a list of all
supported formats. The message is particularly spurious in that case.

But, I haven't thought of a way to fix that message, on its own,
without removing data.
So my proposal is to add more context, by reporting which formats *are*
supported. I think that makes the message look more like part of a
discovery process, rather than just a failure.

Here's some example output:
debug: Puppet::FileServing::Metadata does not support format s (has not implemented method 'from_s')
debug: Puppet::FileServing::Metadata supports formats: pson b64_zlib_yaml marshal yaml raw
debug: Using format yaml for Puppet::FileServing::Metadata

It's possible that this is too chatty; I've seen reports on this
mailing list that even the old message alone clogs the debug logs.
So, I'd like to hear some feedback on whether people agree that this is
an improvement.

In addition, do we think it's worthwhile to write unit tests for debug
messages?

Signed-off-by: Jesse Wolfe <jes...@gmail.com>
---
lib/puppet/network/format.rb | 2 +-
lib/puppet/network/format_handler.rb | 5 ++++-
2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/network/format.rb b/lib/puppet/network/format.rb
index a5be3af..3d1d98c 100644
--- a/lib/puppet/network/format.rb
+++ b/lib/puppet/network/format.rb
@@ -117,7 +117,7 @@ class Puppet::Network::Format

return true if has_method

- Puppet.debug "Format %s not supported for %s; %s" % [self.name, klass, message]
+ Puppet.debug "#{klass} does not support format #{self.name} (#{message})"
return false
end
end
diff --git a/lib/puppet/network/format_handler.rb b/lib/puppet/network/format_handler.rb
index e508a02..13f2401 100644
--- a/lib/puppet/network/format_handler.rb
+++ b/lib/puppet/network/format_handler.rb
@@ -119,6 +119,8 @@ module Puppet::Network::FormatHandler
format_handler.format(b).weight <=> format_handler.format(a).weight
end

+ Puppet.debug "#{self} supports formats: #{result.join(' ')}"
+
put_preferred_format_first(result)
end

@@ -129,8 +131,9 @@ module Puppet::Network::FormatHandler
if list.include?(preferred_format)
list.delete(preferred_format)
list.unshift(preferred_format)
+ Puppet.debug "Using format #{preferred_format} for #{self}"
else
- Puppet.warning "Value of 'preferred_serialization_format' ('#{preferred_format}') is invalid, using default ('#{list.first}')"
+ Puppet.warning "Value of 'preferred_serialization_format' (#{preferred_format}) is invalid for #{self}, using default (#{list.first})"
end
list
end
--
1.6.3.3

[Markus Roberts]

+1. 

If it's deemed too chatty I'd suggest a one liner like:

debug: Puppet::FileServing::Metadata supports: {pson b64_zlib_yaml marshal yaml raw}; Using  yaml

but if no one objects to the volume (that is, if the objection was to the misleading semblance of severity sans context) the patch as submitted looks fine.

[Luke Kanies]

I think this single line is much better; three lines for every network
data type is too much. And really, who's the extra information for?
If a given format isn't supported for a given type, developers are the
only ones who can really do anything about that, right? Users only
need to know the what, not the why, IMO.

So I'd modify it to use something like the above line. And I'd be
comfortable using the short indirection name ('file_metadata') rather
than the class name.

--
Zeilinger's Fundamental Law:
There is no Fundamental Law.
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Jesse Wolfe]

Replace this message that looks like an error

debug: Format s not supported for Puppet::FileServing::Metadata; has not implemented method 'from_s'

with
debug: file_metadata supports formats: b64_zlib_yaml marshal pson raw yaml; using pson

Signed-off-by: Jesse Wolfe <jes...@gmail.com>
---

lib/puppet/network/format.rb | 3 ---
lib/puppet/network/format_handler.rb | 8 ++++++--
2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/network/format.rb b/lib/puppet/network/format.rb
index a5be3af..c437639 100644
--- a/lib/puppet/network/format.rb
+++ b/lib/puppet/network/format.rb
@@ -109,15 +109,12 @@ class Puppet::Network::Format

if type == :class
has_method = klass.respond_to?(method)
- message = "has not implemented method '%s'" % method
else
has_method = klass.instance_methods.include?(method)
- message = "has not implemented instance method '%s'" % method
end

return true if has_method

- Puppet.debug "Format %s not supported for %s; %s" % [self.name, klass, message]

return false
end
end
diff --git a/lib/puppet/network/format_handler.rb b/lib/puppet/network/format_handler.rb

index e508a02..544ba82 100644
--- a/lib/puppet/network/format_handler.rb
+++ b/lib/puppet/network/format_handler.rb
@@ -119,7 +119,11 @@ module Puppet::Network::FormatHandler
format_handler.format(b).weight <=> format_handler.format(a).weight
end

- put_preferred_format_first(result)
+ result = put_preferred_format_first(result)
+
+ Puppet.debug "#{indirection.name} supports formats: #{result.sort.join(' ')}; using #{result.first}"
+
+ result
end

private
@@ -130,7 +134,7 @@ module Puppet::Network::FormatHandler
list.delete(preferred_format)
list.unshift(preferred_format)

else
- Puppet.warning "Value of 'preferred_serialization_format' ('#{preferred_format}') is invalid, using default ('#{list.first}')"

+ Puppet.warning "Value of 'preferred_serialization_format' (#{preferred_format}) is invalid for #{indirection.name}, using default (#{list.first})"

[Luke Kanies]

This logical structure becomes obsolete with the rest of your patch,
so it would be better as something like:

return true if type == :class and klass.respond_to?(method)
return true if klass.instance_methods.include?(method)
return false

I don't quite know whether that's the correct idiom - I've heard
others say they prefer ' if ... and ... and return true'. I'd follow
Markus's guidance there.

Otherwise +1.

> --
>
> You received this message because you are subscribed to the Google
> Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com
> .
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en
> .
>
>


--
The easiest way to figure the cost of living is to take your income and
add ten percent.

[Jesse A Wolfe]

Ah, good point.
Maybe something like:

return klass.respond_to?(method) if type == :class
return klass.instance_methods.include?(method)

[Luke Kanies]

Yep, that works even better than what I recommended.

Zeilinger's Fundamental Law:
There is no Fundamental Law.