From 3472d0d25409740ebc54127f64e19215bb21a578 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Tue, 26 Feb 2019 15:54:57 +0100 Subject: [PATCH 1/6] Make grpc reporter default and add retry Signed-off-by: Pavol Loffay --- Gopkg.lock | 13 ++++++++++++ Gopkg.toml | 4 ++++ cmd/agent/app/reporter/flags.go | 2 +- .../app/reporter/grpc/collector_proxy.go | 7 +++++-- cmd/agent/app/reporter/grpc/flags.go | 6 +++++- cmd/agent/app/reporter/grpc/flags_test.go | 20 +++++++++---------- cmd/all-in-one/main.go | 1 + cmd/collector/app/builder/builder_flags.go | 2 +- crossdock/jaeger-docker-compose.yml | 4 +--- docker-compose/jaeger-docker-compose.yml | 2 +- 10 files changed, 42 insertions(+), 19 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index c4ac2a204d6..db54b701d8b 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -367,6 +367,18 @@ revision = "a7962380ca08b5a188038c69871b8d3fbdf31e89" version = "v1.7.0" +[[projects]] + digest = "1:d84c274ffdfd7ce6b4efd64ee776648459009d1f542440b1faa32cb7ed9aba84" + name = "github.com/grpc-ecosystem/go-grpc-middleware" + packages = [ + "retry", + "util/backoffutils", + "util/metautils", + ] + pruneopts = "UT" + revision = "c250d6563d4d4c20252cd865923440e829844f4e" + version = "v1.0.0" + [[projects]] digest = "1:e4f2416beca662090d5cfc9489208e9d7e3c17dee87e4fe2c5f2c8fe3cb71aa8" name = "github.com/grpc-ecosystem/grpc-gateway" @@ -926,6 +938,7 @@ "github.com/golang/protobuf/protoc-gen-go", "github.com/gorilla/handlers", "github.com/gorilla/mux", + "github.com/grpc-ecosystem/go-grpc-middleware/retry", "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway", "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger", "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options", diff --git a/Gopkg.toml b/Gopkg.toml index 6a8c7de35cd..42f843f2b00 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -122,3 +122,7 @@ required = [ [[constraint]] name = "github.com/Shopify/sarama" version = "1.20.1" + +[[constraint]] + name = "github.com/grpc-ecosystem/go-grpc-middleware" + version = "1.0.0" diff --git a/cmd/agent/app/reporter/flags.go b/cmd/agent/app/reporter/flags.go index fa6aa44e788..8db51bf9388 100644 --- a/cmd/agent/app/reporter/flags.go +++ b/cmd/agent/app/reporter/flags.go @@ -39,7 +39,7 @@ type Options struct { // AddFlags adds flags for Options. func AddFlags(flags *flag.FlagSet) { - flags.String(reporterType, string(TCHANNEL), fmt.Sprintf("Reporter type to use e.g. %s, %s", string(TCHANNEL), string(GRPC))) + flags.String(reporterType, string(GRPC), fmt.Sprintf("Reporter type to use e.g. %s, %s", string(GRPC), string(TCHANNEL))) } // InitFromViper initializes Options with properties retrieved from Viper. diff --git a/cmd/agent/app/reporter/grpc/collector_proxy.go b/cmd/agent/app/reporter/grpc/collector_proxy.go index 9cfe69648f4..05e6f1ff95f 100644 --- a/cmd/agent/app/reporter/grpc/collector_proxy.go +++ b/cmd/agent/app/reporter/grpc/collector_proxy.go @@ -17,6 +17,7 @@ package grpc import ( "errors" + "github.com/grpc-ecosystem/go-grpc-middleware/retry" "github.com/uber/jaeger-lib/metrics" "go.uber.org/zap" "google.golang.org/grpc" @@ -49,10 +50,12 @@ func NewCollectorProxy(o *Options, mFactory metrics.Factory, logger *zap.Logger) resolvedAddrs = append(resolvedAddrs, resolver.Address{Addr: addr}) } r.InitialAddrs(resolvedAddrs) - conn, _ = grpc.Dial(r.Scheme()+":///round_robin", grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name)) + conn, _ = grpc.Dial(r.Scheme()+":///round_robin", grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name), + grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor(grpc_retry.WithMax(o.MaxRetry)))) } else { // It does not return error if the collector is not running - conn, _ = grpc.Dial(o.CollectorHostPort[0], grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name)) + conn, _ = grpc.Dial(o.CollectorHostPort[0], grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name), + grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor(grpc_retry.WithMax(o.MaxRetry)))) } grpcMetrics := mFactory.Namespace(metrics.NSOptions{Name: "", Tags: map[string]string{"protocol": "grpc"}}) return &ProxyBuilder{ diff --git a/cmd/agent/app/reporter/grpc/flags.go b/cmd/agent/app/reporter/grpc/flags.go index 3fb69ff8ce5..f3055696f68 100644 --- a/cmd/agent/app/reporter/grpc/flags.go +++ b/cmd/agent/app/reporter/grpc/flags.go @@ -24,17 +24,20 @@ import ( const ( gRPCPrefix = "reporter.grpc." collectorHostPort = gRPCPrefix + "host-port" + retry = gRPCPrefix + "retry.max" ) // Options Struct to hold configurations type Options struct { // CollectorHostPort is list of host:port Jaeger Collectors. CollectorHostPort []string + MaxRetry uint } // AddFlags adds flags for Options. func AddFlags(flags *flag.FlagSet) { - flags.String(collectorHostPort, "", "(experimental) Comma-separated string representing host:port of a static list of collectors to connect to directly.") + flags.String(collectorHostPort, "", "Comma-separated string representing host:port of a static list of collectors to connect to directly.") + flags.Uint(retry, 3, "Sets the maximum number of retries for a call.") } // InitFromViper initializes Options with properties retrieved from Viper. @@ -43,5 +46,6 @@ func (o *Options) InitFromViper(v *viper.Viper) *Options { if hostPorts != "" { o.CollectorHostPort = strings.Split(hostPorts, ",") } + o.MaxRetry = uint(v.GetInt(retry)) return o } diff --git a/cmd/agent/app/reporter/grpc/flags_test.go b/cmd/agent/app/reporter/grpc/flags_test.go index 2285b0d2681..da727dfd192 100644 --- a/cmd/agent/app/reporter/grpc/flags_test.go +++ b/cmd/agent/app/reporter/grpc/flags_test.go @@ -25,23 +25,23 @@ import ( ) func TestBingFlags(t *testing.T) { - v := viper.New() - command := cobra.Command{} - flags := &flag.FlagSet{} - AddFlags(flags) - command.PersistentFlags().AddGoFlagSet(flags) - v.BindPFlags(command.PersistentFlags()) - tests := []struct { cOpts []string expected *Options }{ - {cOpts: []string{"--reporter.grpc.host-port=localhost:1111"}, - expected: &Options{CollectorHostPort: []string{"localhost:1111"}}}, + {cOpts: []string{"--reporter.grpc.host-port=localhost:1111", "--reporter.grpc.retry.max=15"}, + expected: &Options{CollectorHostPort: []string{"localhost:1111"}, MaxRetry:15}}, {cOpts: []string{"--reporter.grpc.host-port=localhost:1111,localhost:2222"}, - expected: &Options{CollectorHostPort: []string{"localhost:1111", "localhost:2222"}}}, + expected: &Options{CollectorHostPort: []string{"localhost:1111", "localhost:2222"}, MaxRetry:3}}, } for _, test := range tests { + v := viper.New() + command := cobra.Command{} + flags := &flag.FlagSet{} + AddFlags(flags) + command.PersistentFlags().AddGoFlagSet(flags) + v.BindPFlags(command.PersistentFlags()) + err := command.ParseFlags(test.cOpts) require.NoError(t, err) b := new(Options).InitFromViper(v) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index a0c0e87ff95..b8111eb3c8a 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -223,6 +223,7 @@ func createCollectorProxy( ) (agentApp.CollectorProxy, error) { switch repOpts.ReporterType { case agentRep.GRPC: + fmt.Println("AAAAAA") grpcRepOpts.CollectorHostPort = append(grpcRepOpts.CollectorHostPort, fmt.Sprintf("127.0.0.1:%d", cOpts.CollectorGRPCPort)) return agentGrpcRep.NewCollectorProxy(grpcRepOpts, mFactory, logger) case agentRep.TCHANNEL: diff --git a/cmd/collector/app/builder/builder_flags.go b/cmd/collector/app/builder/builder_flags.go index b33337c155d..1adf49df701 100644 --- a/cmd/collector/app/builder/builder_flags.go +++ b/cmd/collector/app/builder/builder_flags.go @@ -59,7 +59,7 @@ func AddFlags(flags *flag.FlagSet) { flags.Int(collectorNumWorkers, app.DefaultNumWorkers, "The number of workers pulling items from the queue") flags.Int(collectorPort, defaultTChannelPort, "The TChannel port for the collector service") flags.Int(collectorHTTPPort, defaultHTTPPort, "The HTTP port for the collector service") - flags.Int(collectorGRPCPort, defaultGRPCPort, "(experimental) The gRPC port for the collector service") + flags.Int(collectorGRPCPort, defaultGRPCPort, "The gRPC port for the collector service") flags.Int(collectorZipkinHTTPort, 0, "The HTTP port for the Zipkin collector service e.g. 9411") } diff --git a/crossdock/jaeger-docker-compose.yml b/crossdock/jaeger-docker-compose.yml index f6a5d9dd1fb..1ce1e33aaea 100644 --- a/crossdock/jaeger-docker-compose.yml +++ b/crossdock/jaeger-docker-compose.yml @@ -30,9 +30,7 @@ services: jaeger-agent: image: jaegertracing/jaeger-agent - # FIXME temporarily switching back to tchannel - # https://github.com/jaegertracing/jaeger/issues/1229 - command: ["--reporter.tchannel.host-port=jaeger-collector:14267"] + command: ["--reporter.grpc.host-port=jaeger-collector:14267"] ports: - "5775:5775/udp" - "6831:6831/udp" diff --git a/docker-compose/jaeger-docker-compose.yml b/docker-compose/jaeger-docker-compose.yml index 02a40336ad3..c10b55b393d 100644 --- a/docker-compose/jaeger-docker-compose.yml +++ b/docker-compose/jaeger-docker-compose.yml @@ -26,7 +26,7 @@ services: jaeger-agent: image: jaegertracing/jaeger-agent - command: ["--reporter.type=grpc", "--reporter.grpc.host-port=jaeger-collector:14250"] + command: ["--reporter.grpc.host-port=jaeger-collector:14250"] ports: - "5775:5775/udp" - "6831:6831/udp" From 5669ad3646235aa3ffe561af3af60a9368d5b609 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Tue, 26 Feb 2019 16:08:01 +0100 Subject: [PATCH 2/6] Polish Signed-off-by: Pavol Loffay --- cmd/agent/app/reporter/grpc/flags.go | 2 +- cmd/agent/app/reporter/grpc/flags_test.go | 2 +- cmd/agent/app/reporter/grpc/reporter.go | 4 ++++ cmd/all-in-one/main.go | 1 - 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/agent/app/reporter/grpc/flags.go b/cmd/agent/app/reporter/grpc/flags.go index f3055696f68..b27ecd95db0 100644 --- a/cmd/agent/app/reporter/grpc/flags.go +++ b/cmd/agent/app/reporter/grpc/flags.go @@ -37,7 +37,7 @@ type Options struct { // AddFlags adds flags for Options. func AddFlags(flags *flag.FlagSet) { flags.String(collectorHostPort, "", "Comma-separated string representing host:port of a static list of collectors to connect to directly.") - flags.Uint(retry, 3, "Sets the maximum number of retries for a call.") + flags.Uint(retry, defaultMaxRetry, "Sets the maximum number of retries for a call.") } // InitFromViper initializes Options with properties retrieved from Viper. diff --git a/cmd/agent/app/reporter/grpc/flags_test.go b/cmd/agent/app/reporter/grpc/flags_test.go index da727dfd192..1cba9c3ee27 100644 --- a/cmd/agent/app/reporter/grpc/flags_test.go +++ b/cmd/agent/app/reporter/grpc/flags_test.go @@ -32,7 +32,7 @@ func TestBingFlags(t *testing.T) { {cOpts: []string{"--reporter.grpc.host-port=localhost:1111", "--reporter.grpc.retry.max=15"}, expected: &Options{CollectorHostPort: []string{"localhost:1111"}, MaxRetry:15}}, {cOpts: []string{"--reporter.grpc.host-port=localhost:1111,localhost:2222"}, - expected: &Options{CollectorHostPort: []string{"localhost:1111", "localhost:2222"}, MaxRetry:3}}, + expected: &Options{CollectorHostPort: []string{"localhost:1111", "localhost:2222"}, MaxRetry:defaultMaxRetry}}, } for _, test := range tests { v := viper.New() diff --git a/cmd/agent/app/reporter/grpc/reporter.go b/cmd/agent/app/reporter/grpc/reporter.go index 38f3425dee9..c1cceeb49c2 100644 --- a/cmd/agent/app/reporter/grpc/reporter.go +++ b/cmd/agent/app/reporter/grpc/reporter.go @@ -29,6 +29,10 @@ import ( "github.com/jaegertracing/jaeger/thrift-gen/zipkincore" ) +const ( + defaultMaxRetry = 3 +) + // Reporter reports data to collector over gRPC. type Reporter struct { collector api_v2.CollectorServiceClient diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index b8111eb3c8a..a0c0e87ff95 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -223,7 +223,6 @@ func createCollectorProxy( ) (agentApp.CollectorProxy, error) { switch repOpts.ReporterType { case agentRep.GRPC: - fmt.Println("AAAAAA") grpcRepOpts.CollectorHostPort = append(grpcRepOpts.CollectorHostPort, fmt.Sprintf("127.0.0.1:%d", cOpts.CollectorGRPCPort)) return agentGrpcRep.NewCollectorProxy(grpcRepOpts, mFactory, logger) case agentRep.TCHANNEL: From bc8f7120aea1c8828516ce2cf67651ccdd8bc89d Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Tue, 26 Feb 2019 16:39:57 +0100 Subject: [PATCH 3/6] Fix port Signed-off-by: Pavol Loffay --- crossdock/jaeger-docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crossdock/jaeger-docker-compose.yml b/crossdock/jaeger-docker-compose.yml index 1ce1e33aaea..0b2f7714d9b 100644 --- a/crossdock/jaeger-docker-compose.yml +++ b/crossdock/jaeger-docker-compose.yml @@ -30,7 +30,7 @@ services: jaeger-agent: image: jaegertracing/jaeger-agent - command: ["--reporter.grpc.host-port=jaeger-collector:14267"] + command: ["--reporter.grpc.host-port=jaeger-collector:14250"] ports: - "5775:5775/udp" - "6831:6831/udp" From bfbf3bcac88fede11f64a2980b50b2038b1e525b Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Tue, 26 Feb 2019 16:46:15 +0100 Subject: [PATCH 4/6] Polish Signed-off-by: Pavol Loffay --- cmd/agent/app/reporter/grpc/collector_proxy.go | 14 ++++++++------ cmd/agent/app/reporter/grpc/flags.go | 1 + cmd/agent/app/reporter/grpc/reporter.go | 4 ---- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/cmd/agent/app/reporter/grpc/collector_proxy.go b/cmd/agent/app/reporter/grpc/collector_proxy.go index 05e6f1ff95f..f8b0eb97ccf 100644 --- a/cmd/agent/app/reporter/grpc/collector_proxy.go +++ b/cmd/agent/app/reporter/grpc/collector_proxy.go @@ -42,7 +42,7 @@ func NewCollectorProxy(o *Options, mFactory metrics.Factory, logger *zap.Logger) if len(o.CollectorHostPort) == 0 { return nil, errors.New("could not create collector proxy, address is missing") } - var conn *grpc.ClientConn + var target string if len(o.CollectorHostPort) > 1 { r, _ := manual.GenerateAndRegisterManualResolver() var resolvedAddrs []resolver.Address @@ -50,13 +50,15 @@ func NewCollectorProxy(o *Options, mFactory metrics.Factory, logger *zap.Logger) resolvedAddrs = append(resolvedAddrs, resolver.Address{Addr: addr}) } r.InitialAddrs(resolvedAddrs) - conn, _ = grpc.Dial(r.Scheme()+":///round_robin", grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name), - grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor(grpc_retry.WithMax(o.MaxRetry)))) + target = r.Scheme() + ":///round_robin" } else { - // It does not return error if the collector is not running - conn, _ = grpc.Dial(o.CollectorHostPort[0], grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name), - grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor(grpc_retry.WithMax(o.MaxRetry)))) + target = o.CollectorHostPort[0] } + // It does not return error if the collector is not running + conn, _ := grpc.Dial(target, + grpc.WithInsecure(), + grpc.WithBalancerName(roundrobin.Name), + grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor(grpc_retry.WithMax(o.MaxRetry)))) grpcMetrics := mFactory.Namespace(metrics.NSOptions{Name: "", Tags: map[string]string{"protocol": "grpc"}}) return &ProxyBuilder{ conn: conn, diff --git a/cmd/agent/app/reporter/grpc/flags.go b/cmd/agent/app/reporter/grpc/flags.go index b27ecd95db0..487c2c587b1 100644 --- a/cmd/agent/app/reporter/grpc/flags.go +++ b/cmd/agent/app/reporter/grpc/flags.go @@ -25,6 +25,7 @@ const ( gRPCPrefix = "reporter.grpc." collectorHostPort = gRPCPrefix + "host-port" retry = gRPCPrefix + "retry.max" + defaultMaxRetry = 3 ) // Options Struct to hold configurations diff --git a/cmd/agent/app/reporter/grpc/reporter.go b/cmd/agent/app/reporter/grpc/reporter.go index c1cceeb49c2..38f3425dee9 100644 --- a/cmd/agent/app/reporter/grpc/reporter.go +++ b/cmd/agent/app/reporter/grpc/reporter.go @@ -29,10 +29,6 @@ import ( "github.com/jaegertracing/jaeger/thrift-gen/zipkincore" ) -const ( - defaultMaxRetry = 3 -) - // Reporter reports data to collector over gRPC. type Reporter struct { collector api_v2.CollectorServiceClient From 4860295b3ed23b152eb0e147efc2bcc4a1bb250f Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Tue, 26 Feb 2019 17:08:30 +0100 Subject: [PATCH 5/6] Use higher retry Signed-off-by: Pavol Loffay --- crossdock/jaeger-docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crossdock/jaeger-docker-compose.yml b/crossdock/jaeger-docker-compose.yml index 0b2f7714d9b..2df94d2f5ef 100644 --- a/crossdock/jaeger-docker-compose.yml +++ b/crossdock/jaeger-docker-compose.yml @@ -30,7 +30,7 @@ services: jaeger-agent: image: jaegertracing/jaeger-agent - command: ["--reporter.grpc.host-port=jaeger-collector:14250"] + command: ["--reporter.grpc.host-port=jaeger-collector:14250", "--reporter.grpc.retry.max=20"] ports: - "5775:5775/udp" - "6831:6831/udp" From f9a890b770812adea3cd159fc5aed4dcafa84fbe Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Tue, 26 Feb 2019 17:34:07 +0100 Subject: [PATCH 6/6] Increase retry to 100 Signed-off-by: Pavol Loffay --- crossdock/jaeger-docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crossdock/jaeger-docker-compose.yml b/crossdock/jaeger-docker-compose.yml index 2df94d2f5ef..630b82f3f1b 100644 --- a/crossdock/jaeger-docker-compose.yml +++ b/crossdock/jaeger-docker-compose.yml @@ -30,7 +30,7 @@ services: jaeger-agent: image: jaegertracing/jaeger-agent - command: ["--reporter.grpc.host-port=jaeger-collector:14250", "--reporter.grpc.retry.max=20"] + command: ["--reporter.grpc.host-port=jaeger-collector:14250", "--reporter.grpc.retry.max=100"] ports: - "5775:5775/udp" - "6831:6831/udp"