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

Load external .so plugins, fixes #1717 #6024

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

Timidger
Copy link
Contributor

@Timidger Timidger commented Jun 20, 2019

Original patch written by sparrc, brought up to date by timidger.
Original from 92d8a2e message follows:

support for the Go 1.8 shared object feature of loading external
plugins.

this support relies on the developer defining a Plugin symbol in their
.so file that is a telegraf plugin interface.

So instead of the plugin developer "Adding" their own plugin to the
telegraf registry, telegraf loads the .so, looks up the Plugin symbol,
and then adds it if it finds it.

The name of the plugin is determined by telegraf, and is namespaced
based on the filename and path.

see #1717

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Since this adding a new capability to inject plugins I wasn't sure where to document this or how to test this. So looking for feed back on how to do those things 😄

Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
Timidger pushed a commit to StarryInternet/telegraf-plugin-rs that referenced this pull request Jun 20, 2019
This is not a complete solution but has the base form for how the
project is laid out and has an example showing how it can be used in a
project.

To test with telegraf you'll need to use the changes in this PR:
influxdata/telegraf#6024
@Timidger Timidger changed the title Load external .so plugins Load external .so plugins, fixes #1717 Jun 24, 2019
@Timidger
Copy link
Contributor Author

ping? Does anyone know a good way to test this or refactor this so it can be tested and merged?

@danielnelson
Copy link
Contributor

@Timidger I'm not ready to give this change a thumbs up or down yet, we are still trying to decide in which ways we want to support these types of extensions, and don't want to provide any methods officially that we aren't prepared to support.

@mtp401
Copy link

mtp401 commented Jul 16, 2019

@danielnelson I think supporting some sort DSO based interface like what
@Timidger has implemented here is incredibly important for most IoT/embedded
scenarios for a couple of reasons. Firstly, in these types of resource
constrained environments the cost of everything really counts. The existing
generic methods for integrating with telegraf (exec, socket input, etc) are all
relatively expensive compared to an in memory solution. In addition to
performance concerns, the state of the art in most of these environments is
either C or C++. It would be much easier to adopt Telegraf into these
environments there was a straightforward way of integrating plugins written
in one of these languages. For most other projects, this integration point is
usually a C FFI.

Is there some existing preferred direction for an extensions API that looks
like this?

@danielnelson
Copy link
Contributor

@mtp401 Thanks, I appreciate you taking the time to provide this feedback. To be sure, I agree about the value of calling into native code and that it is a big limitation right now.

When this code was originally written we ran into some issues that required Telegraf and all plugins to be compiled together; it didn't seem possible to compile ones own plugin and then use it with official builds or use a compiled plugin on multiple Telegraf versions. I don't think we looked at plugins using CGO, I imagine they would require both Telegraf and the plugin to be compiled with CGO_ENABLED. There were other shortcomings when using libraries from pure Go plugins and the lack of Windows support meant we couldn't move anything standard into plugins. Given these limitations, at the time I didn't feel like the solution was a large enough improvement over creating a new plugin in a fork to be included.

I do encourage you to compile with this code report back with any feedback you have, it is always good to gather experience with using a plugin.

@Timidger
Copy link
Contributor Author

I do encourage you to compile with this code report back with any feedback you have, it is always good to gather experience with using a plugin.

There's an example of how I plan to use it here, which is from this pr using this glue code that relies on these changes.

For the IoT/embedded usecase that @mtp401 mentioned generally some sort of cross build tool is used that compiles from source the entire target image. Thus the versioning limitations you mentioned wouldn't affect this usecase since telegraf would essentially be vendored (and thus always at one version).

For other projects that want to use this feature they can weight the limitations vs the benefits. If being tied to one version and compiling with CGo is not an option then there's always the exec functionality. However, if they are working in a limited environment the ability to avoid expensive logging by doing everything in a shared library is a huge boon.

@danielnelson
Copy link
Contributor

If you are building the plugin and Telegraf together it works quite well, however I think it also reduces the need for the plugin code quite a bit. For example, I think this would work and only requires a single line change to Telegraf itself, give it a try if you have time:

Add a init function to your glue code:

func init() {
    inputs.Add("rust_plugin", func() telegraf.Input { return &RustPlugin{} })
}

Then add the plugin import path to Telegraf's input all.go:

import (
	_ "github.com/StarryInternet/telegraf-plugin-go-glue"

Compile Telegraf and no need to compile a go-plugin.

One thing I'm considering is just to always load all go-plugins from a directory, this could allow you to skip the one line change and also allow third party sql packages to be loaded as described here, without committing to a particular API on the Telegraf side.

@Timidger
Copy link
Contributor Author

I tried your suggestion and that does seem much simpler. I don't know about the SQL use case that well, but I know it definitely solves our use case with minimal support needed from telegraf itself.

If that is a solution you are willing to purse I can close this PR and implement that instead. Let me know what you decide.

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Yeah let's do it, we will have the plugin directory option and just try to Open any plugins in there. We won't do any special handling of the plugin, leaving them totally free form for now.

internal/usage.go Outdated Show resolved Hide resolved
cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved
cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved
@Timidger
Copy link
Contributor Author

Addressed your comments. Just to double check we should still be trying to load the Plugin structure from the go plugin, like I had it before?

@danielnelson
Copy link
Contributor

For now let's only Open the plugin, and plugins can use the init() function to register new plugins.

@Timidger
Copy link
Contributor Author

Got it, will fix this up on Monday

Original patch written by sparrc, brought up to date by timidger.
Original from 92d8a2e message follows:

support for the Go 1.8 shared object feature of loading external
plugins.

this support relies on the developer defining a `Plugin` symbol in their
.so file that is a telegraf plugin interface.

So instead of the plugin developer "Adding" their own plugin to the
telegraf registry, telegraf loads the .so, looks up the Plugin symbol,
and then adds it if it finds it.

The name of the plugin is determined by telegraf, and is namespaced
based on the filename and path.

see influxdata#1717
@Timidger
Copy link
Contributor Author

Ok, the plugin is now simply loaded and it is expected for the init method to handle all injection into the input plugin list.

@danielnelson danielnelson added this to the 1.12.0 milestone Jul 30, 2019
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 30, 2019
@danielnelson danielnelson merged commit 5c7c9e4 into influxdata:master Jul 30, 2019
@Timidger Timidger deleted the external-go-plugins branch July 30, 2019 14:00
// ie, if the plugin file is ./telegraf-plugins/foo.so, name
// will be "telegraf-plugins/foo"
name := strings.TrimPrefix(strings.TrimPrefix(pth, rootDir), string(os.PathSeparator))
name = strings.TrimSuffix(name, filepath.Ext(pth))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the var name defined?
Looks like is not being used.

Maybe it was thought to register the plugin with that name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, TOML doesn't like names with slasesh:

[[inputs.extplugins/ext-kernel]]
2019-08-05T11:01:53Z E! [telegraf] Error running agent: Error parsing prueba.conf, line 5: invalid TOML 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name is leftover of the previous implementation, you're right that it should be removed.

Also, TOML doesn't like names with slasesh

You can include a slash using ""

[[inputs."extplugins/ext-kernel"]]

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably already have this figured out, but I removed the name code when merging.
Plugins can be loaded using the name specified in the registration function called from init().

I also have an example that I was using for testing over at https://github.com/danielnelson/telegraf-plugins

@danielnelson danielnelson mentioned this pull request Aug 7, 2019
3 tasks
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants