-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
WIP: Add native plugins support to the Collector #1050
Conversation
9ddc40d
to
e59c101
Compare
Great! Looks very promising. Side note: I didn't even know we had presave postsave... Whatever for? A few comments on the design:
You asked for examples, I think the ultimate example would be exposing one of the existing storage impls as a real plugin (despite them being also statically linked), to demo the handling of configuration. |
Stupid question, but doesn't |
Yes afaik |
Yes I think the handling of dependencies in a plugin is not fully working as the issues mentioned here are still open. The main reason I started with the filters/processors is that a storage implementation would likely require additional libraries. With the current bugs, I'm not sure if it's even feasible to implemement a storage plugins without using hashicorp/go-plugin... That being said, I'm not sure neither if using hashicorp/go-plugin for filters/processors would be a good idea performance wise. I could work on the following changes:
But if the dependency issue is a deal breaker for filters/processors (as it is for storage), I would start exploring hashicorp/go-plugins instead. |
I'm not sure it's a deal breaker, but it might change how we support/document this. If we are going to support only the plugins we build as part of the regular binary, then I think it's OK to have it like this. But if we are going to say that we support people building their own plugins, then I think we need to sort out the dependency problem. |
101f524
to
5c4efd1
Compare
It turns out much harder than expected to do the proposed changes but I feel (even if it's not fully done yet) that it's a much cleaner implementation! So here's what I added in the last commits:
Things to do:
|
do we need to? the storage factory facade is already able to pass loggers to the underlying factories. |
So the logger problem is mainly because it's getting created here (in The usage is mainly in that method: https://github.com/jaegertracing/jaeger/pull/1050/files#diff-e6ce03f3742f3a6c8b3e443d8e1a53daR58 A similar thing that would benefit from having a bootstrap logger is this method: https://github.com/jaegertracing/jaeger/pull/1050/files#diff-03e42ee625fab9eaa63d9d9daa235f8eR62 that currently use |
maybe punt on the logger for now, use io.Writer same as that last example. |
I've looked at the other PR for hashicorp/go-plugins and it look great so far, but I still started to implement the storage support to experiement with it as well. Now that this is pushed, I'm trying to implement a working storage plugin with dependencies that are not in the main project. So far I'm having the same issue as we expected:
I'll try to workaround it by compiling both in the same Docker container and report back as soon I have have something working or if I run out of idea to make it work 👍 |
So I was able to make it run. The plugin doesn't do much right now so it's 100% proven but I still wanted to update the PR with more information of what the build looks like. All the commands will be run inside a Docker, here's my exact run command:
Some generic installations/variables to make the build work:
Build Jaeger as we already do:
Now this gets trickier:
With that done, you can run it as follow:
|
Have you tried pulling Jaeger as a vendored dependency of the plugin repo? Because all these machinations remind me of the classic transitive vendor problem caused by relying on a dependency from GOPATH that has its own vendor dir. In our internal builds at Uber we always build against an empty GOPATH, to avoid these issues and have the dep manager fully responsible for deps. |
I am kin to help with this since the gRPC plugin version has shown to have a huge performance impact |
@olivierboucher even though I've worked on this, I think I prefer the gRPC approach anyway. Seems like the recent results you posted are promising too! |
So I've tried it a bit more and it seems that Go Modules (added in Go 1.11) helped with the issue I've mentioned above. Thanks to this comment golang/go#28983 (comment), I was able to make it run successfully!! Here's the recipe so far:
Which produce this:
Note: I run it in Docker to ensure that there's no "hack" on my computer and that it's a repeatable process. |
Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
…t of the Collector code Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
…ush metrics Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
fb6a440
to
52e009e
Compare
@yurishkuro So this is looking pretty good! The only real modification (except from moving to the main package and changing the repo) was to add this file to properly export the plugin: https://github.com/ledor473/jaeger-storage-badger/blob/master/plugin.go With the same recipe as the last comment, I now get the following help:
And it's not just the help that is working:
|
Closing as gRPC plugin is merged now |
Which problem is this PR solving?
This PR provides the foundation to support plugins in Jaeger using native Go plugins (
pkg/plugin
). Resolves #422Supported plugins:
spanstore
+dependencystore
)Sanitizer
,PreProcessor
,PreSave
,SpanFilter
)Short description of the changes
The current PR is working and this comment #1050 (comment) includes an example of a storage plugin that seems to work so far.
What is missing
How to run it
Gotchas