Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom plugin name for 'curl', 'memcachec', 'tail' and 'tail_csv' plugins. #1681

Merged
merged 5 commits into from Sep 29, 2017

Conversation

rpv-tomsk
Copy link
Contributor

Like added in #1558 and #1563 , added option 'PluginName' to other Collectd plugins:

tail plugin: Allow custom plugin name for file instances
tail_csv: Allow custom plugin name for file instances
curl plugin: Allow custom plugin name for each Page block
memcachec: Allow custom plugin name for each Page block

@rpv-tomsk
Copy link
Contributor Author

Hi @toni-moreno ,

Could be a great idea recover the this change to use in all general purpose pugins.

Stay tuned.


If someone knows about other general purpose plugins I missed in this work, please inform me, I will update this PR.

@mfournier
Copy link

I'm a bit concerned about adding a per-plugin config option to change the plugin name. With the filter chains system, we already have a generic way to rename the host, plugin, plugin_instance and type_instance on the fly.

Agreed it's not as simple as a one-line configuration option, but it works for all current and future plugins, without adding any code.

Am I missing something ? What's the motivation behing this change ?

@rpv-tomsk
Copy link
Contributor Author

Existence of generic way for renaming is good, but for 'general purpose' plugins configuration by per-plugin option is much simplier.

As for me, as sysadm, I never touched Collectd 'filters' configuration at all. Its usage (for renaming), as for me (again), is like using iptables to redirect HTTP port 80 when 'webserver' listens 65079 and that value is unchangeable.

Currently, other general-purpose plugins allows to specify PluginName: 'exec'; 'java' ('GenericJMX'), 'python', 'perl' bindings. And there is no requirement to rename fixed 'exec' / 'java' / 'python' / 'perl' plugin name to required value via filters - that plugins just works. That is simple and simply works - that is what expected by people.

I see no problems about adding PluginName option to curl_, tail_ and memcachec plugins (and to possible future general-purpose plugins too).

@toni-moreno
Copy link
Contributor

Hi @mfournier I agree with @rpv-tomsk .

Anyway 2 years ago I was playing with filters and I was not able to configure filters to change plugin name for combined plugins and instanceid. how can be done now?

I think also this code could be easily maintainable , since there is no much generic purpose plugins, and there is not much code to change.

I can help if you need.

Thank you very much.

@tokkee
Copy link
Member

tokkee commented Jun 5, 2016

I agree that a config option for generic plugins is better than using filter-chains for the purpose. In fact, it's more powerful because filter-chains only work if there are no duplicate names before renaming (i.e. the config option can be used to avoid conflicts).

That said, instead of (or in addition to) a static name, I wonder if it would be better to fetch the name from a query result. In SQL plugins (e.g., #1707), this would be easy, because a static name could also be provided by the query. For the plugins in this PR, this may not always be feasible, though.

@tokkee
Copy link
Member

tokkee commented Jun 5, 2016

On another note, there are now a bunch of PRs all related to this topic. @rpv-tomsk could you open a separate issue as a feature request for this and collecting all information (and conversations) in one place?

@octo
Copy link
Member

octo commented Aug 6, 2016

I tend to agree with @mfournier here: changing the plugin name is not essential, since in all the rather generic plugins you can control large parts of the identifier already. If you have a good use case, you can change the plugin with generic tools (i.e. filter chains) that work for all metrics.

@rpv-tomsk
Copy link
Contributor Author

rpv-tomsk commented Aug 7, 2016

changing the plugin name is not essential

Completely disagreed. Did you look into #1558 and #1563, especially into 0dcb6a6 ?

If you have a good use case, you can change the plugin with generic tools (i.e. filter chains) that
work for all metrics.

  • There is MANY good use-cases. These (with patch proposed) plugins are 'generic', so people can use them for gathering metrics from different part of their systems. That is their common use, they are implemented for this, so there is many use cases exists. For example, some metrics from curl_json can be sent as 'phpfpm' plugin name and some as another plugin name. I use 'Tail' plugin for gathering metrics which are sent as 'postfix' and 'apache' plugin names.
  • That looks too complex to use filters for these simple tasks. It is much easier to put one directive to set 'PluginName' than write complex filter with matches and sets. Filters is not convenient for this as configuration split is inconvenient in practice.

//As for me, I do not use 'filters' in none of my servers, and I have 50+ of them.
//I think many peoples does not use that too.

  • The possibility to set 'plugin instance' value from the values obtained by plugin is required.
    And the possibility to set 'plugin name' makes things (configuration) consistent.

So, proposed possibility to set 'PluginName' is very useful, powerful and easy to use.

Also this feature is easily implemented and does not require efforts to support it in future.

@octo
Copy link
Member

octo commented Aug 8, 2016

You're right in that there is no "service" label in collectd and plugin can kind of fill that position.

Please use the config option Plugin for this purpose everywhere. Open an umbrella issue, like @tokkee suggested, to keep track of all ongoing work in this direction and then create one PR per plugin, referencing that tracking issue.

Thanks and best regards,
—octo

@rpv-tomsk
Copy link
Contributor Author

and then create one PR per plugin, referencing that tracking issue.

Hi! I'm going to continue this work.
Which branch should I use as PR target?

@octo
Copy link
Member

octo commented Sep 8, 2016

Hi @rpv-tomsk, thanks for looking into this! Please open PRs implementing new features against the master branch.

@rpv-tomsk
Copy link
Contributor Author

Renamed 'PluginName' option to unified 'Plugin' form.

@rpv-tomsk
Copy link
Contributor Author

Rebased to current master branch.

@rpv-tomsk
Copy link
Contributor Author

ping

@rpv-tomsk
Copy link
Contributor Author

Ping me if you want this PR to be merged, I will rebase it to current master.

@rpv-tomsk
Copy link
Contributor Author

The PR rebased to current master.

Please take a look and merge it.

@octo octo added the Feature label Sep 29, 2017
Copy link
Member

@octo octo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @rpv-tomsk!

@octo octo merged commit 6fe9925 into collectd:master Sep 29, 2017
@octo octo added this to the 5.8 milestone Oct 6, 2017
@rpv-tomsk rpv-tomsk deleted the tail-plugins-pluginname branch July 6, 2018 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants