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 #2373

Closed
wants to merge 1 commit into from
Closed

Load external .so plugins #2373

wants to merge 1 commit into from

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Feb 7, 2017

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • README.md updated (if adding a new plugin)

@sparrc sparrc force-pushed the telegraf-registry-2 branch from 787218e to 9c9afa1 Compare February 7, 2017 13:30
@sparrc sparrc changed the title WIP Load external .so plugins Feb 7, 2017
@phemmer
Copy link
Contributor

phemmer commented Feb 7, 2017

What if the loadable code wants to provide multiple plugins? Such as multiple inputs, or an input & output, etc.

@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

why not just do it in multiple files?

@phemmer
Copy link
Contributor

phemmer commented Feb 7, 2017

It could be that the multiple plugins share a lot of code.

@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

they can still share code, all you need to do is create a separate file within the same package with something like

package mem

var Plugin telegraf.Input = &memOtherStuff{}

@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

actually I guess it can't be within the same package, you would need to create a separate package for the shared code

@phemmer
Copy link
Contributor

phemmer commented Feb 7, 2017

Correct. We can say this is how it must be done. Just wanted to raise the issue as it does make things a little harder for the plugin developers.

This also made me realize another issue. If 2 different plugins were built with different versions of some 3rd party package, they both won't be able to load. Unfortunately I don't think there's much we can do about this :-(

@phemmer
Copy link
Contributor

phemmer commented Feb 7, 2017

I suspect even loading plugins built with different versions of go will fail if they use a standard lib package that changed between go versions. Ditto if they use a standard lib package telegraf uses.

This seems like a really significant problem to the whole module thing. Will have to poke around on the interwebs to see if the official go devs have any commentary on the matter.

@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

yes, it seems they are setting a rather hard line on that right now in the first release. My hope is that they will loosen up the requirements as it goes, but we'll see.

@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

@phemmer they are actually explicitly blocking this, and doesn't seem like there are any plans to do otherwise: golang/go#17832

@phemmer
Copy link
Contributor

phemmer commented Feb 7, 2017

Yeah, saw that. The design document has more info on the subject. A few bits talk about the future, such as:

all of those plugins must be built with the same version of the Go toolchain, and any shared packages must be identical. It may be possible to weaken this restriction in the future, but we’ll start with this.

In that quote, it's talking about the C-API, but I imagine it applies just as well to native go (otherwise you could just have go use the C-API to load the plugin).

 

There's not a whole lot of discussion on the subject that I could find. But I imagine there will be a LOT more once 1.8 is release, people start trying to write plugins, and find all the versioning compatibility issues.

I know I personally was looking forward to this feature of telegraf, but with this, it's rather dampened my enthusiasm :-(. I can just imagine the royal nightmare its going to be having to ensure every single plugin (and telegraf itself) was built with the same version of go, and any common package.

@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

I think myself and everyone here at Influx would agree with that statement, we're going to think about how we might handle this packaging-wise and whether we want to go down the gRPC route instead...

@sparrc sparrc force-pushed the telegraf-registry-2 branch 2 times, most recently from b9583ff to 4778a38 Compare February 9, 2017 12:53
@sparrc sparrc force-pushed the telegraf-registry-2 branch from 4778a38 to 1df55cb Compare February 16, 2017 22:47
@sparrc sparrc force-pushed the telegraf-registry-2 branch 2 times, most recently from 7971146 to 5da2841 Compare March 9, 2017 11:51
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
@sparrc sparrc force-pushed the telegraf-registry-2 branch from 5da2841 to 92d8a2e Compare March 10, 2017 16:55
@danielnelson danielnelson mentioned this pull request Apr 20, 2017
3 tasks
@phemmer phemmer mentioned this pull request Apr 30, 2017
3 tasks
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017
@russorat
Copy link
Contributor

for now, we are not planning to add this functionality.

@russorat russorat closed this Sep 28, 2018
@danielnelson danielnelson deleted the telegraf-registry-2 branch July 30, 2019 04:36
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.

6 participants