Add Opentracing plugin support #4506

frrist · 2017-12-19T19:02:50Z

define an interface for creating and configuring tracers
add the Jaeger tracer as a plugin

License: MIT
Signed-off-by: ForrestWeston forrest@protocol.ai

frrist · 2017-12-19T19:05:35Z

Points worth noting:

Plugins do not work on windows.
~~- Jaeger should be version controlled (currently its not), not sure how to accomplish this yet.~~
To start the Jaeger UI (lets you view traces, spans, and their metadata)

docker run -p5775:5775/udp -p6831:6831/udp -p6832:6832/udp \                                                                                                     Tue Dec 19 18:57:00 UTC 2017
    -p5778:5778 -p16686:16686 -p14268:14268 -p9411:9411 jaegertracing/all-in-one:0.8.0

~~- To actually use this you need to have a few different branches checked out and packages linked, will add a how-to for that shortly.~~

frrist · 2017-12-19T19:13:29Z

Not sure what's going wrong with Jenkins here, the plugin isn't enabled in the PR, and it looks like all tests pass (not sure on the exit code though):

# passed all 243 test(s)
1..243
script returned exit code 2

whyrusleeping · 2017-12-19T22:24:36Z

plugin/loader/initializer.go

@@ -41,3 +52,15 @@ func runIPLDPlugin(pl plugin.Plugin) error {

 	return ipldpl.RegisterInputEncParsers(coredag.DefaultInputEncParsers)
 }
+
+func runTracerPlugin(pl plugin.Plugin) error {


since we're doing a type cast above, we can make this function just accept a PluginTracer and skip the cast here. Should do the same for the ipld one too

whyrusleeping · 2017-12-19T22:25:18Z

plugin/plugins/jaeger/jaeger.go

+}
+
+//Initalize a Jaeger tracer and set it as the global tracer in opentracing api
+func (*jaegerPlugin) InitGlobalTracer(cfg map[string]interface{}) error {


we should probably use values from the cfg input in the configuration we do here.

Good point. I think instead of using a cfg we should define tracer specific options in the plugins themselves, since config values can differ across different tracer implementations.

whyrusleeping · 2018-01-04T01:59:42Z

plugin/loader/initializer.go

-			return err
+		switch pl.(type) {
+		case plugin.PluginIPLD:
+			err := runIPLDPlugin(pl.(plugin.PluginIPLD))


no need to do the cast here, the type switch does some magic for you.

Are we sure about this? building with err := runIPLDPlugin(pl) instead of err := runIPLDPlugin(pl.(plugin.PluginIPLD)) fails

plugin/loader/initializer.go:26:24: cannot use pl (type "github.com/ipfs/go-ipfs/plugin".Plugin) as type "github.com/ipfs/go-ipfs/plugin".PluginIPLD in argument to runIPLDPlugin: "github.com/ipfs/go-ipfs/plugin".Plugin does not implement "github.com/ipfs/go-ipfs/plugin".PluginIPLD (missing RegisterBlockDecoders method)

Ah, i forgot one bit, it needs to be:

switch pl := pl.(type) {

whyrusleeping · 2018-01-04T02:01:37Z

plugin/loader/initializer.go

+}
+
+func runTracerPlugin(pl plugin.PluginTracer) error {
+	err := pl.InitGlobalTracer()


To make type checks easier, I would do:

tr, err := pl.GetTracer() if err != nil { return err } opentrace.SetGlobalTracer(tr)

That way if the types in your plugin are wrong, this won't fail silently, it will hit type assertion issues and throw an error.

whyrusleeping · 2018-01-30T23:01:48Z

cmd/ipfs/main.go


 		// check if repo is accessible before loading plugins
 		ok, err := checkPermissions(cctx.ConfigRoot)
 		if err != nil {
 			return nil, err
 		}
 		if ok {
-			if _, err := loader.LoadPlugins(pluginpath); err != nil {
+			if _, err := loader.LoadPlugins(cctx.ConfigRoot); err != nil {


Hrm... making that code open up and parse the config on its own breaks some abstractions. The fact that the config is a file on disk is an implementation detail, We might need to rethink some things here.

cc @Stebalien @magik6k We need to get some configuration, and the peer ID to the tracing plugin. I'm not entirely sure yet how that should get fed through, ideas?

For some context, here is how it may be used with the tracing plugin
https://github.com/ipfs/go-jaeger-plugin/pull/1/files#diff-e92ca16875e3c00b8a36338613029bf9R35

This will also break changes in https://github.com/ipfs/go-ipfs/pull/4244/files#diff-153cb71153ee178fe2869148600ce8d5R197.

The problem is that at this stage we may not even have a peerID/config (i.e. user calling ipfs init), we may add some sort of late init for plugins that want it and call that above in buildEnv, either before or after calling core.NewNode

For now setting the tracer name could be a manual process - maybe we don't want it to be the nodeID, I was doing this for convenience.

This would still be done via a configuration file (where the tracer name is a field), but that file would reside in the plugin path - possibly under a directory based on the plugin name (e.g. pluginPath/pluginName/config.cfg).

I imagine we will want the ability to pass plugins config files in the future?

Stebalien

LGTM. This interface may need to change while implementing/reviewing the plugin itself but this looks like a good baseline.

Kubuxu

LGTM

magik6k

1 nitpick, rest LGTM

magik6k · 2018-03-02T00:40:35Z

plugin/loader/initializer.go

@@ -3,6 +3,7 @@ package loader
 import (
 	"github.com/ipfs/go-ipfs/core/coredag"
 	"github.com/ipfs/go-ipfs/plugin"
+	"gx/ipfs/QmWLWmRVSiagqP15jczsGME1qpob6HDbtbHAY2he9W5iUo/opentracing-go"


Import grouping

Kubuxu · 2018-03-20T12:22:04Z

I've rebased it on top of the master to allow #4806 to base off it.

Kubuxu

I would say: let's merge it and allow #4806 to improve it in future.

define interface for creating tracers for use with opentracing-api License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

frrist requested review from Stebalien, whyrusleeping and Kubuxu December 19, 2017 19:02

ghost assigned frrist Dec 19, 2017

ghost added the status/in-progress In progress label Dec 19, 2017

whyrusleeping reviewed Dec 19, 2017

View reviewed changes

frrist force-pushed the feat/opentrace branch from 933eccf to 9e55c95 Compare December 19, 2017 23:35

frrist force-pushed the feat/opentrace branch from 38cdfd3 to 8e2dcf8 Compare December 27, 2017 20:51

whyrusleeping reviewed Jan 4, 2018

View reviewed changes

frrist force-pushed the feat/opentrace branch from 38d1140 to 256b50b Compare January 4, 2018 19:37

frrist changed the title ~~WIP - Add tracer plugin~~ Add Opentracing plugin support Jan 4, 2018

whyrusleeping mentioned this pull request Jan 6, 2018

KCP transport (reliable UDP) libp2p/go-libp2p#193

Closed

frrist force-pushed the feat/opentrace branch 2 times, most recently from ce55e7f to 47e4b4a Compare January 8, 2018 20:56

hsanjuan mentioned this pull request Jan 9, 2018

Use OpenTracing to gather valuable execution information and metrics ipfs-cluster/ipfs-cluster#287

Closed

frrist force-pushed the feat/opentrace branch from 47e4b4a to 4b5fd5f Compare January 30, 2018 20:04

whyrusleeping reviewed Jan 30, 2018

View reviewed changes

frrist force-pushed the feat/opentrace branch 2 times, most recently from 1003957 to 179e64a Compare February 28, 2018 20:54

Stebalien approved these changes Mar 1, 2018

View reviewed changes

Kubuxu approved these changes Mar 1, 2018

View reviewed changes

magik6k reviewed Mar 2, 2018

View reviewed changes

frrist force-pushed the feat/opentrace branch from 179e64a to eebfa07 Compare March 2, 2018 20:57

Kubuxu added RFM and removed status/in-progress In progress labels Mar 10, 2018

Kubuxu added this to the go-ipfs 0.4.15 milestone Mar 10, 2018

Kubuxu force-pushed the feat/opentrace branch from eebfa07 to 5ad5e4f Compare March 20, 2018 12:20

ghost assigned Kubuxu Mar 20, 2018

ghost added the status/in-progress In progress label Mar 20, 2018

Kubuxu mentioned this pull request Mar 20, 2018

Stateful plugin loading #4806

Merged

Kubuxu removed the RFM label Mar 20, 2018

Kubuxu approved these changes Mar 23, 2018

View reviewed changes

Kubuxu added RFM and removed status/in-progress In progress labels Mar 23, 2018

magik6k approved these changes Mar 23, 2018

View reviewed changes

Stebalien mentioned this pull request Mar 24, 2018

discovery: swap mdns lib for grandcat/zeroconf libp2p/go-libp2p#285

Closed

2 tasks

Add tracer plugin support

46722af

define interface for creating tracers for use with opentracing-api License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

frrist force-pushed the feat/opentrace branch from 5ad5e4f to 46722af Compare March 25, 2018 19:58

ghost added the status/in-progress In progress label Mar 25, 2018

whyrusleeping merged commit 7badbeb into master Mar 25, 2018

whyrusleeping deleted the feat/opentrace branch March 25, 2018 21:04

ghost removed the status/in-progress In progress label Mar 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Opentracing plugin support #4506

Add Opentracing plugin support #4506

frrist commented Dec 19, 2017

frrist commented Dec 19, 2017 •

edited

Loading

frrist commented Dec 19, 2017

whyrusleeping Dec 19, 2017

whyrusleeping Dec 19, 2017

frrist Dec 19, 2017

whyrusleeping Jan 4, 2018

frrist Jan 4, 2018

frrist Jan 4, 2018

whyrusleeping Jan 4, 2018

whyrusleeping Jan 4, 2018

whyrusleeping Jan 30, 2018

frrist Jan 30, 2018

magik6k Jan 30, 2018

frrist Jan 31, 2018

Stebalien left a comment

Kubuxu left a comment

magik6k left a comment

magik6k Mar 2, 2018

Kubuxu commented Mar 20, 2018

Kubuxu left a comment

Add Opentracing plugin support #4506

Add Opentracing plugin support #4506

Conversation

frrist commented Dec 19, 2017

frrist commented Dec 19, 2017 • edited Loading

frrist commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Kubuxu left a comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Mar 20, 2018

Kubuxu left a comment

Choose a reason for hiding this comment

frrist commented Dec 19, 2017 •

edited

Loading