[collectd] Varnish plugin
octo at verplant.org
Tue Jun 1 15:45:58 CEST 2010
thank you very much for your patch :)
I have a couple of small suggestions / questios regarding the code,
maybe you could help me out:
* You forgot to add yourself as copyright holder at the beginning of the
file. I guess you copied the "apache" plugin and modified it. Since
there's basically nothing of the "apache" plugin left, I'd suggest you
simply remove those other lines.
* The new "types" introduces by the patch, "varnish_cache" and
"varnish_connections", have multiple "data sources" each. This is
deprecated and only used when there's a good reason for it. I think
the "cache_ratio" type could be used instead of "varnish_cache". Many
other plugins define their own "foo_connections" type, for example
"memcached_connections" and "nginx_connections". Feel free to copy one
of those definitions to "varnish_connections". [*]
* The only argument passed to "VSL_OpenStats" is a NULL pointer called
"varnish_instance_name". Would it be reasonable to let the user
configure something else here and use NULL as a default?
* When "VSL_OpenStats" fails, is it possible to get a more detailed
error description or at least an error code?
* The member (value_list_t *)->values_len is of type "int". You assign
to it a "size_t" without cast, which may result in warnings (and hence
problems) with some optimization settings.
[*] In the long run, I'd prefer to have *one* type for this purpose that
is used by all plugins. I might change that in an attempt to unify
the behavior eventually.
Florian octo Forster
Hacker in training
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20100601/e3bab140/attachment.pgp
More information about the collectd