-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
How to support plugins #422
Comments
I think Kubernetes is also planning on supporting plugins for external storage. I'm not familiar with how it works, but it would perhaps be worth checking if it would make sense for us to follow the same path. |
I think K8s plugins will be runnable containers, i.e. integration is out of process. A lot of our use cases require in-process integration for performance reasons, e.g. span sanitizers, filters. |
I believe out of process integration (effectively middleware/brokers) would be practical for storage, of course, for things like sanitizers, filters, et al. that is a different story. |
What I found developing Kanali is that plugins are amazing for allowing users to add their own custom functionality. For example, if people design plugins for Kanali, the only requirement is that they implement a certain interface. One side affect is that the I think I agree with @omeid that storage should be out of process. For things like these, I'd propose using gRPC to communicate with out of process integration. For in-process items, such as encrypting tag values, +1 for golang plugins. |
I've been thinking this past week about how to support plugins in Jaeger. Some considerations:
|
Addressing your last point, when apiVersion: extensions/v1beta1
kind: Deployment
metadata:
name: kanali
namespace: default
spec:
template:
metadata:
labels:
app: kanali
spec:
serviceAccountName: kanali
containers:
- name: kanali
image: northwesternmutual/kanali
command: ["/kanali", "start"]
volumeMounts:
- name: plugins
mountPath: /plugins
readOnly: true
initContainers:
- name: kanali-plugin-apikey
image: fbgrecojr/kanali-plugin-apikey:local
imagePullPolicy: IfNotPresent
command: ["cp", "/apiKey_v2.0.0-rc.1.so", "/plugins/"]
volumeMounts:
- name: plugins
mountPath: /plugins
volumes:
- name: plugins
emptyDir: {} Addressing your first point, making these plugins configurable is tricky. ENV variables will work but I haven't found a solution for dynamically changing the cli interface. The problems lies in the fact that a golang plugin's init function is called when they are opened. This of course would happened after the init functions for the code doing the opening is executed. |
I was thinking of something very similar, thanks for posting! Re dynamic CLI, I have a proposal in #608. |
Food for thought - hashicorp terraform also does pluggable backends. AFAIR, they use protobuf for intra-process communication. Might be worth having a look at how they do it. |
@yurishkuro Ran into a nasty issue relating to plugins with Kanali that I thought I'd share. In theory, plugins should be able to be dynamically loaded without the need to modify Jaeger's container image. In the context of Kubernetes, the recommended approach would be to use init containers. The init container would move the compiled plugin into the location where the Jaeger binary is configured to look for them. However, there is a nasty bug that prevents this from being the case until at least Go v1.11. Evidence of this can be tracked in the following issues:
The root cause of the issue stems from the fact that unless a plugin is compiled using the exact same |
The current workaround that I use entails building your custom plugin within the same |
Thanks for pointing those out, @frankgreco. It seems like a complete show stopper to me until there's a better fix from Go. Even if the plugin is build with the same vendor as Jaeger core, there's going to be tight coupling between Jaeger version and plugin build, since the next Jaeger version might have upgrades to the deps. I actually want to give the hashicorp gRPC based plugin framework a try. |
@yurishkuro I agree! I'm going to checkout their gRPC framework as well. |
@yurishkuro Curious if there's been any new developments here as I am interested in working on a storage plugin. Is harshicorp gRPC still being evaluated? |
Now that the Protobuf model has been merged, the development of gRPC-based plugins can go ahead without blocking on the rest of #773. We have not evaluated harshicorp plugins system, however. I am not sure what it provides that's not achieved by plain gRPC. |
I just had a look at harshicorp/go-plugin. If I understand it correctly, even though it uses gRPC (can also use net/rpc), it spawns the plugin as a child process and uses Unix domain socket as the comms channel. This sounds good for implementing plugins for real storage. It somewhat derails another plan I had to build in-memory storage as a shared gRPC service, which would be useful for our crossdock integration tests (instead of always spawning Cassandra), but it won't work with hashicorp/go-plugin since we need a single shared storage process, not two separate child processes for collector and query. However, hashicorp/go-plugin requires us to define a gRPC service and implement client and server anyway, so maybe we can reuse those. I will update the top of the ticket with what I think is an actionable and realistic plan. |
Hi @yurishkuro, is it possible that your in-memory storage issue could be worked around with some atomic handling of plugin start? It is possible to define a grpc server with a custom listen address, If plugins were responsible for their own configuration, they could be given an explicit listen address for the grpc server, if they were not able to bind, they would simply exit. What do you think? We at https://github.com/Klarrio use Jaeger for some of the interesting bits on our platform. We are interested in having storage plugins implemented and we would happily contribute. |
I don't think lock files are relevant here. If I understand correctly, @yurishkuro means that Hashicorp plugins are always spawned as a subprocess, so cannot be shared among multiple services. Collector and query need to access the same database in order for the tests to succeed. However, I don't see any problem with implementing a storage plugin interface in plain grpc without Hashicorp plugins. That should resolve everything. |
@isaachier one could write this from scratch but Hashicorp plugins support reattaching, the only inconvenience is that there is a plugin instance starting and stopping immediately due to not being able to bind. Why writing everything from scratch? |
@olivierboucher cool, hashicorp plugins appear to work! |
Still a work in progress. Related to issue jaegertracing#422. Leverages Hashicorp's go-plugin. New set of protos for the use of plugin maintainers. Missing tests. Still need to create a plugin binary for the memory backend. Signed-off-by: Olivier Boucher <info@olivierboucher.com>
Renamed packages from "external" to "grpc". Changed storage type to "grpc-plugin". Added cmd/memory-grpc-plugin that uses the memory backend as a grpc plugin. Added go-plugin dependency to Glide. Added task to generate plugin protos to Makefile. Signed-off-by: Olivier Boucher <info@olivierboucher.com>
Curious alternative approach (https://github.com/mholt/caddy/wiki): adding the plugins to the source code and building a custom binary, i.e. does not involve distributing pre-built binaries for core server + plugins. |
A company that needs a custom plugin would then have to build Jaeger from the sources themselves? |
Yes, that's why I don't like that solution. They actually have a website where you can select the plugins in the UI and it builds the binary for you and allows to download it, which is ever more odd for automatic deployment & raises security concerns. |
It sounds cool, but I probably wouldn't want to use it as a customer, nor support it as a vendor :-) |
Indeed, hence I labeled it curious rather that useful. |
Relating to issue jaegertracing#422 and review jaegertracing#1214, add proto changes to later allow for gRPC plugins to be used. Signed-off-by: Charles Dixon <chvckd@gmail.com>
@yurishkuro It would be tremendous help if this feature be finished and merged, to let end user to plugin their own storage without pulling down the binary for other backends; and easier to add additional backends easily without tying to to this repo. Do you have a time line for this feature? |
@etsangsplk we discussed it last Friday and I think the consensus is to move forward with https://github.com/hashicorp/go-plugin, with InfluxDB being the first implementation (#272). No promises about the timeline, but we are closer to getting it done now than we were one week ago :-) |
Hello, we'd (github.com/exaring) like to provide plugin support for storage backend with Go's pkg/plugin support. Later on we'd like to build a storage plugin for DynamoDB. What is the current state of the discussion/progress here? |
The problem with Go's pkg/plugin approach is that it (currently) requires your plugins to be built against a very specific revision/tag of Jaeger. Whenever you update your Jaeger instance, you'll need to recompile your plugins, or you risk introducing some bugs that are nasty and hard to detect/diagnose. |
@jpkrohling Is there is weekly meeting group for jaegertracing backend (or a page that record the meeting notes)? |
@yurishkuro @jpkrohling |
@jacobmarble we usually don't use feature branches. The work is happening on some PRs linked to this issue, e.g. #1323, #1214. |
Relating to issue jaegertracing#422 and review jaegertracing#1214, add proto changes to later allow for gRPC plugins to be used. Signed-off-by: Charles Dixon <chvckd@gmail.com>
* Add gRPC plugin proto changes Relating to issue #422 and review #1214, add proto changes to later allow for gRPC plugins to be used. Signed-off-by: Charles Dixon <chvckd@gmail.com> * Change FindTraces signature to return a stream. FindTraces can hit grpc message size limits if a large number of spans are requested, using a stream mitigates this issue. Signed-off-by: Charles Dixon <chvckd@gmail.com> * Satisfy gofmt tool Signed-off-by: Charles Dixon <chvckd@gmail.com> * Change proto package and service names Signed-off-by: Charles Dixon <chvckd@gmail.com> * Delete commented out spanstorage Signed-off-by: Charles Dixon <chvckd@gmail.com> * Change FindTraces response to be a stream of spans Signed-off-by: Charles Dixon <chvckd@gmail.com> * Change from EmptyMessage to google.protobuf.Empty Signed-off-by: Charles Dixon <chvckd@gmail.com> * Move from using StoragePluginError to google.rpc.Status Signed-off-by: Charles Dixon <chvckd@gmail.com> * Remove commented code and clean up proto formatting Signed-off-by: Charles Dixon <chvckd@gmail.com> * Remove protobuf responses and only return successes, rely on Status for errors Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update Gopkg lockfile Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update Span type to come from model.proto Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update storage proto file Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add generated storage plugin file to lint ignores Signed-off-by: Charles Dixon <chvckd@gmail.com> * Lint ignore grpc plugin generated code by name not path Signed-off-by: Charles Dixon <chvckd@gmail.com> * Rename FindTracesResponseChunk to SpansResponseChunk Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add marshal/unmarshal tests for DependencyLink Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add tests for storage protos with custom types Signed-off-by: Charles Dixon <chvckd@gmail.com> * Run fmt and ignore storage_test for linting Signed-off-by: Charles Dixon <chvckd@gmail.com> * Remove DependencyLinkSource and use string Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update headers Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add SpansChunkResponse test Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update makefile protoc calls Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update proto generated files and update license script Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update generated storage file to new proto layout Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add storage generated files to import order cleanup ignores Signed-off-by: Charles Dixon <chvckd@gmail.com> * Move storage generated file to proto-gen dir Signed-off-by: Charles Dixon <chvckd@gmail.com> * Remove generated plugin storage file from script ignores Signed-off-by: Charles Dixon <chvckd@gmail.com> * Fix copyright headers Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update storage_test generated file Signed-off-by: Charles Dixon <chvckd@gmail.com>
Still a work in progress. Related to issue jaegertracing#422. Leverages Hashicorp's go-plugin. New set of protos for the use of plugin maintainers. Missing tests. Still need to create a plugin binary for the memory backend. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added Memory GRPC Plugin. Renamed packages (jaegertracing#422) Renamed packages from "external" to "grpc". Changed storage type to "grpc-plugin". Added cmd/memory-grpc-plugin that uses the memory backend as a grpc plugin. Added go-plugin dependency to Glide. Added task to generate plugin protos to Makefile. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Refactored gRPC storage plugin protos Now makes use of protos from model/proto/model.proto and removed mapping functions. Plugin invoker calls the plugin binary directly instead of relying on sh. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Refactored gRPC plugin protos to use defined models Removed mapping code. Removed used of sh, calling plugin binary directly. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Removed .editorconfig file Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added support for gRPC plugin configuration Configuration file is passed down to the plugin through the --config argument. It is the plugin's responsability to parse the file and initialize itself. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added benchmark for noop SpanWriter vs gRPC based noop plugin Fixed issues related to proto changes. Deleted memory-grpc-plugin since it was no longer relevant. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Updated gRPC plugin & protos to implement latest SpanReader Added dependencies to Gopkg.lock since Glide -> dep transition happened recently. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Fixed proto generation introduced when switching to Dep Changed json tags to match when TraceID was not a proto. Signed-off-by: Olivier Boucher <info@olivierboucher.com> proto fix Fixed issue introduced moving TraceID to model.proto Added proper annotations to storage.proto instead of exposing TraceID in model.proto. Tests are back to green. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added tests for gRPC storage factory and options Moved DependencyLink to model.proto since it had to be used and the existing struct did not implement any Marshaler/Unmarshaler methods. Changed storage configuration to allow testing via mocks. Added DependencyReader interface to the StoragePlugin interface. Signed-off-by: Olivier Boucher <info@olivierboucher.com>
* Add New Storage Plugin Framework Still a work in progress. Related to issue #422. Leverages Hashicorp's go-plugin. New set of protos for the use of plugin maintainers. Missing tests. Still need to create a plugin binary for the memory backend. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added Memory GRPC Plugin. Renamed packages (#422) Renamed packages from "external" to "grpc". Changed storage type to "grpc-plugin". Added cmd/memory-grpc-plugin that uses the memory backend as a grpc plugin. Added go-plugin dependency to Glide. Added task to generate plugin protos to Makefile. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Refactored gRPC storage plugin protos Now makes use of protos from model/proto/model.proto and removed mapping functions. Plugin invoker calls the plugin binary directly instead of relying on sh. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Refactored gRPC plugin protos to use defined models Removed mapping code. Removed used of sh, calling plugin binary directly. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Removed .editorconfig file Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added support for gRPC plugin configuration Configuration file is passed down to the plugin through the --config argument. It is the plugin's responsability to parse the file and initialize itself. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added benchmark for noop SpanWriter vs gRPC based noop plugin Fixed issues related to proto changes. Deleted memory-grpc-plugin since it was no longer relevant. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Updated gRPC plugin & protos to implement latest SpanReader Added dependencies to Gopkg.lock since Glide -> dep transition happened recently. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Fixed proto generation introduced when switching to Dep Changed json tags to match when TraceID was not a proto. Signed-off-by: Olivier Boucher <info@olivierboucher.com> proto fix Fixed issue introduced moving TraceID to model.proto Added proper annotations to storage.proto instead of exposing TraceID in model.proto. Tests are back to green. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added tests for gRPC storage factory and options Moved DependencyLink to model.proto since it had to be used and the existing struct did not implement any Marshaler/Unmarshaler methods. Changed storage configuration to allow testing via mocks. Added DependencyReader interface to the StoragePlugin interface. Signed-off-by: Olivier Boucher <info@olivierboucher.com> * Remove old generated storage file Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update plugin grpc interface/implementation Signed-off-by: Charles Dixon <chvckd@gmail.com> * Fix gosec by adding an exclude comment Signed-off-by: Charles Dixon <chvckd@gmail.com> * Run make fmt Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add comments to uncommented functions Signed-off-by: Charles Dixon <chvckd@gmail.com> * Split grpc server and client into separate files Signed-off-by: Charles Dixon <chvckd@gmail.com> * Use errors.wrap rather than fmt for errors Signed-off-by: Charles Dixon <chvckd@gmail.com> * Remove unused plugin file Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add copyright header to grpc server file Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update headers to updated license on uncommitted files Signed-off-by: Charles Dixon <chvckd@gmail.com> * Move grpc config to plugin/storage Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add empty test files to fix build Signed-off-by: Yuri Shkuro <ys@uber.com> * Move grpc config empty_test file Signed-off-by: Charles Dixon <chvckd@gmail.com> * Change fmt for error to errors.Wrap Signed-off-by: Charles Dixon <chvckd@gmail.com> * Use single-line for comprehensions where possible Signed-off-by: Charles Dixon <chvckd@gmail.com> * Don't accumulate spans before sending Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add function to serve the plugin Signed-off-by: Charles Dixon <chvckd@gmail.com> * Small grpc client refactor Signed-off-by: Charles Dixon <chvckd@gmail.com> * Update grpc interface and move noop plugin to examples Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add missed fix for factory Signed-off-by: Charles Dixon <chvckd@gmail.com> * Rename memory-store example to memstore-plugin Signed-off-by: Charles Dixon <chvckd@gmail.com> * Refactor Serve to call ServeWithGRPCServer Signed-off-by: Charles Dixon <chvckd@gmail.com> * Refactor to make some structs private Signed-off-by: Charles Dixon <chvckd@gmail.com> * Change factory_test to use mocks Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add storage grpc integration test Signed-off-by: Charles Dixon <chvckd@gmail.com> * Create mocks for storage_v1 and add grpc client tests Signed-off-by: Charles Dixon <chvckd@gmail.com> * Add grpc server tests Signed-off-by: Charles Dixon <chvckd@gmail.com> * Run make fmt Signed-off-by: Charles Dixon <chvckd@gmail.com> * Strip unnecessary code from test Signed-off-by: Charles Dixon <chvckd@gmail.com>
#1461 is merged 🎉 🎉 🎉 🎈 🎈 🎈 Many thanks to @olivierboucher and @chvck 👏 👏 👏 Remaining task: add documentation #1518. |
@yurishkuro can we close this one? |
We continue to be asked if we can "support X as storage backend" (e.g. #331, #421). Provided that the authors are willing to contribute, maintain, and support such backend implementations, we still have an open question of whether we want to accept those contributions into the main
jaeger
repository. I could be wrong, by my initial reaction is that we should keep them in separate "contrib" style repositories, for the following reasons:If we do keep them in the contrib repos, however, we need an approach to allow end users to use those implementations without having to rebuild the backend from source.
One such approach is using Go plugins (https://golang.org/pkg/plugin/), for example as done in Kanali. I think it is feasible to package plugins as individual containers and mount them into a shared volume where the main Jaeger binaries can locate and load them.
cc @pavolloffay @black-adder - any thoughts?
Update 12/28/2017
An alternative approach mentioned in the comments below is the sidecar plugin model (e.g. https://github.com/hashicorp/go-plugin) where the plugin runs as a separate process and the main binary communicates with it via RPC, e.g. gRPC streams. It's worth noting, however, that this approach is still a special case of in-process plugin model, so we need to start there and answer the questions below. For each type of plugin we can support a built-in "sidecar" variant.
Update 8/2/2018
Per #422 (comment), I think the following is a reasonable, actionable, and realistic plan. If someone wants to volunteer, ping me.
h-plugin
(h for harshicorp) andg-plugin
(plain gRPC). Theh-plugin
should support a cli flag for the name of the plugin executable. Theg-plugin
should support a cli flag for grpc server host:port.g-plugin
using gRPC client/server defined aboveh-plugin
as a template.h-plugin
.Update 9/4/2018
Someone pointed out that Go's pkg/plugin now supports MacOS and Linux. This removes a significant development hurdle with using the native plugins, and makes it a viable option which is probably simpler to implement than the gRPC-based harshicorp model.
The text was updated successfully, but these errors were encountered: