-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Load external .so plugins, fixes #1717 #6024
Conversation
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
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
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
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
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
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
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
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
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
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
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
ping? Does anyone know a good way to test this or refactor this so it can be tested and merged? |
@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. |
@danielnelson I think supporting some sort DSO based interface like what Is there some existing preferred direction for an extensions API that looks |
@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. |
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 |
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. |
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. |
There was a problem hiding this 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.
55af621
to
500ef08
Compare
Addressed your comments. Just to double check we should still be trying to load the |
For now let's only |
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
500ef08
to
2cf73a1
Compare
Ok, the plugin is now simply loaded and it is expected for the |
// 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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"]]
There was a problem hiding this comment.
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
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:
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 😄