Merge branch 'master' into add-pprof-endpoint

.github/PULL_REQUEST_TEMPLATE.md

@@ -6,7 +6,7 @@ Before creating a pull request, please make sure:
 - You have read the guide for contributing
   - See https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING.md
 - You signed all your commits (otherwise we won't be able to merge the PR)
-  - See https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING.md#sign-your-work
+  - See https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md#certificate-of-origin---sign-your-work
 - You added unit tests for the new functionality
 - You mention in the PR description which issue it is addressing, e.g. "Resolves #123"
 -->

.travis.yml

@@ -7,30 +7,33 @@ dist: trusty
 
 matrix:
   include:
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - TESTS=true
     - COVERAGE=true
-  - go: "1.11.1"
+  - go: "1.12.1"
+    env:
+    - PROTO_GEN_TEST=true
+  - go: "1.12.1"
     env:
     - ALL_IN_ONE=true
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - CROSSDOCK=true
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - DOCKER=true
     - DEPLOY=true
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - ES_INTEGRATION_TEST=true
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - KAFKA_INTEGRATION_TEST=true
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - CASSANDRA_INTEGRATION_TEST=true
-  - go: "1.11.1"
+  - go: "1.12.1"
     env:
     - HOTROD=true
 
@@ -52,6 +55,12 @@ install:
   - if [ "$ALL_IN_ONE" == true ]; then bash ./scripts/travis/install-ui-deps.sh ; fi
   - if [ "$DOCKER" == true ]; then bash ./scripts/travis/install-ui-deps.sh ; fi
   - if [ "$CROSSDOCK" == true ]; then bash ./scripts/travis/install-crossdock-deps.sh ; fi
+  - |
+    if [ "$PROTO_GEN_TEST" == true ]; then
+        make proto-install
+        curl -L https://github.com/google/protobuf/releases/download/v3.6.1/protoc-3.6.1-linux-x86_64.zip -o /tmp/protoc.zip
+        unzip /tmp/protoc.zip -d "$HOME"/protoc
+    fi
 
 script:
   - export BRANCH=$(if [ "$TRAVIS_PULL_REQUEST" == "false" ]; then echo $TRAVIS_BRANCH; else echo $TRAVIS_PULL_REQUEST_BRANCH; fi)
@@ -63,6 +72,7 @@ script:
   - if [ "$KAFKA_INTEGRATION_TEST" == true ]; then bash ./scripts/travis/kafka-integration-test.sh ; else echo 'skipping kafka integration test'; fi
   - if [ "$CASSANDRA_INTEGRATION_TEST" == true ]; then bash ./scripts/travis/cassandra-integration-test.sh ; else echo 'skipping cassandra integration test'; fi
   - if [ "$HOTROD" == true ]; then bash ./scripts/travis/hotrod-integration-test.sh ; else echo 'skipping hotrod example'; fi
+  - if [ "$PROTO_GEN_TEST" == true ]; then make proto PROTOC=$HOME/protoc/bin/protoc && git diff --name-status --exit-code ; else echo 'skipping protoc validation'; fi
 
 after_success:
   - if [ "$COVERAGE" == true ]; then mv cover.out coverage.txt ; else echo 'skipping coverage'; fi

CHANGELOG.md

@@ -7,6 +7,33 @@ Changes by Version
 #### Backend Changes
 
 ##### Breaking Changes
+- The `kafka` flags were removed in favor of `kafka.producer` and `kafka.consumer` flags ([#1424](https://github.com/jaegertracing/jaeger/pull/1424), [@ledor473](https://github.com/ledor473))
+
+    The following flags have been **removed** in the Collector and the Ingester:
+    ```
+    --kafka.brokers
+    --kafka.encoding
+    --kafka.topic
+    --ingester.brokers
+    --ingester.encoding
+    --ingester.topic
+    --ingester.group-id
+    ``` 
+    
+    In the Collector, they are replaced by:
+    ```
+    --kafka.producer.brokers
+    --kafka.producer.encoding
+    --kafka.producer.topic
+    ```
+    
+    In the Ingester, they are replaced by:
+    ```
+    --kafka.consumer.brokers
+    --kafka.consumer.encoding
+    --kafka.consumer.topic
+    --kafka.consumer.group-id
+    ```
 
 ##### New Features
 
@@ -21,7 +48,7 @@ Changes by Version
 #### Backend Changes
 
 ##### Breaking Changes
-- Introduce `kafka.producer` and `kafka.consumer` flags to replace `kafka` flags ([1360](https://github.com/jaegertracing/jaeger/pull/1360), [@ledor473](https://github.com/ledor473))
+- Introduce `kafka.producer` and `kafka.consumer` flags to replace `kafka` flags ([#1360](https://github.com/jaegertracing/jaeger/pull/1360), [@ledor473](https://github.com/ledor473))
 
     The following flags have been deprecated in the Collector and the Ingester:
     ```

CONTRIBUTING_GUIDELINES.md

@@ -13,7 +13,7 @@ Table of Contents:
 
 * [Making a Change](#making-a-change)
 * [License](#license)
-* [Certificate of Origin - Sign your work](#sign-your-work)
+* [Certificate of Origin - Sign your work](#certificate-of-origin---sign-your-work)
 * [Branches](#branches)
 
 ## Making a Change
@@ -45,7 +45,7 @@ Your pull request is most likely to be accepted if **each commit**:
   * Use the imperative mood in the subject line
   * Wrap the body at 72 characters
   * Use the body to explain _what_ and _why_ instead of _how_
-* Has been signed by the author ([see below](#sign-your-work)).
+* Has been signed by the author ([see below](#certificate-of-origin---sign-your-work)).
 
 ## License
 

Makefile

@@ -332,6 +332,7 @@ generate-mocks: install-mockery
 echo-version:
 	@echo $(GIT_CLOSEST_TAG)
 
+PROTOC := protoc
 PROTO_INCLUDES := \
 	-I model/proto \
 	-I vendor/github.com/grpc-ecosystem/grpc-gateway \
@@ -372,19 +373,19 @@ proto:
 	# TODO use Docker container instead of installed protoc
 	# (https://medium.com/@linchenon/generate-grpc-and-protobuf-libraries-with-containers-c15ba4e4f3ad)
 	#
-	protoc \
+	$(PROTOC) \
 		$(PROTO_INCLUDES) \
 		--gogo_out=plugins=grpc,$(PROTO_GOGO_MAPPINGS):$(PWD)/model/ \
 		model/proto/model.proto
 
-	protoc \
+	$(PROTOC) \
 		$(PROTO_INCLUDES) \
 		--gogo_out=plugins=grpc,$(PROTO_GOGO_MAPPINGS):$(PWD)/proto-gen/api_v2/ \
 		--grpc-gateway_out=$(PROTO_GOGO_MAPPINGS):$(PWD)/proto-gen/api_v2/ \
 		--swagger_out=$(PWD)/proto-gen/openapi/ \
 		model/proto/api_v2.proto
 
-	protoc \
+	$(PROTOC) \
 		-I model/proto \
 		--go_out=$(PWD)/model/prototest/ \
 		model/proto/model_test.proto
@@ -397,5 +398,4 @@ proto-install:
 		./vendor/github.com/gogo/protobuf/protoc-gen-gogo \
 		./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \
 		./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
-		# ./vendor/github.com/mwitkow/go-proto-validators/protoc-gen-govalidators \
-		# ./vendor/github.com/rakyll/statik
+		# ./vendor/github.com/mwitkow/go-proto-validators/protoc-gen-govalidators

cmd/agent/app/builder.go

@@ -16,15 +16,12 @@ package app
 
 import (
 	"fmt"
-	"io/ioutil"
 	"net/http"
-	"os"
 
 	"github.com/apache/thrift/lib/go/thrift"
 	"github.com/pkg/errors"
 	"github.com/uber/jaeger-lib/metrics"
 	"go.uber.org/zap"
-	"google.golang.org/grpc/grpclog"
 
 	"github.com/jaegertracing/jaeger/cmd/agent/app/configmanager"
 	"github.com/jaegertracing/jaeger/cmd/agent/app/httpserver"
@@ -233,8 +230,7 @@ func CreateCollectorProxy(
 	}
 	switch opts.ReporterType {
 	case reporter.GRPC:
-		grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, os.Stderr, os.Stderr))
-		return grpc.NewCollectorProxy(grpcRepOpts, mFactory, logger)
+		return grpc.NewCollectorProxy(grpcRepOpts, opts.AgentTags, mFactory, logger)
 	case reporter.TCHANNEL:
 		return tchannel.NewCollectorProxy(tchanRep, mFactory, logger)
 	default:

cmd/agent/app/reporter/flags.go

@@ -17,12 +17,17 @@ package reporter
 import (
 	"flag"
 	"fmt"
+	"os"
+	"strings"
 
 	"github.com/spf13/viper"
 )
 
 const (
+	// Whether to use grpc or tchannel reporter.
 	reporterType = "reporter.type"
+	// Agent tags
+	agentTags = "jaeger.tags"
 	// TCHANNEL is name of tchannel reporter.
 	TCHANNEL Type = "tchannel"
 	// GRPC is name of gRPC reporter.
@@ -35,15 +40,44 @@ type Type string
 // Options holds generic reporter configuration.
 type Options struct {
 	ReporterType Type
+	AgentTags    map[string]string
 }
 
 // AddFlags adds flags for Options.
 func AddFlags(flags *flag.FlagSet) {
 	flags.String(reporterType, string(GRPC), fmt.Sprintf("Reporter type to use e.g. %s, %s", string(GRPC), string(TCHANNEL)))
+	flags.String(agentTags, "", "One or more tags to be added to the Process tags of all spans passing through this agent. Ex: key1=value1,key2=${envVar:defaultValue}")
 }
 
 // InitFromViper initializes Options with properties retrieved from Viper.
 func (b *Options) InitFromViper(v *viper.Viper) *Options {
 	b.ReporterType = Type(v.GetString(reporterType))
+	b.AgentTags = parseAgentTags(v.GetString(agentTags))
 	return b
 }
+
+// Parsing logic borrowed from jaegertracing/jaeger-client-go
+func parseAgentTags(agentTags string) map[string]string {
+	if agentTags == "" {
+		return nil
+	}
+	tagPairs := strings.Split(string(agentTags), ",")
+	tags := make(map[string]string)
+	for _, p := range tagPairs {
+		kv := strings.SplitN(p, "=", 2)
+		k, v := strings.TrimSpace(kv[0]), strings.TrimSpace(kv[1])
+
+		if strings.HasPrefix(v, "${") && strings.HasSuffix(v, "}") {
+			ed := strings.SplitN(string(v[2:len(v)-1]), ":", 2)
+			e, d := ed[0], ed[1]
+			v = os.Getenv(e)
+			if v == "" && d != "" {
+				v = d
+			}
+		}
+
+		tags[k] = v
+	}
+
+	return tags
+}

cmd/agent/app/reporter/flags_test.go

@@ -16,6 +16,7 @@ package reporter
 
 import (
 	"flag"
+	"os"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -24,6 +25,25 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestBindFlags_NoJaegerTags(t *testing.T) {
+	v := viper.New()
+	command := cobra.Command{}
+	flags := &flag.FlagSet{}
+	AddFlags(flags)
+	command.PersistentFlags().AddGoFlagSet(flags)
+	v.BindPFlags(command.PersistentFlags())
+
+	err := command.ParseFlags([]string{
+		"--reporter.type=grpc",
+	})
+	require.NoError(t, err)
+
+	b := &Options{}
+	b.InitFromViper(v)
+	assert.Equal(t, Type("grpc"), b.ReporterType)
+	assert.Len(t, b.AgentTags, 0)
+}
+
 func TestBindFlags(t *testing.T) {
 	v := viper.New()
 	command := cobra.Command{}
@@ -34,10 +54,21 @@ func TestBindFlags(t *testing.T) {
 
 	err := command.ParseFlags([]string{
 		"--reporter.type=grpc",
+		"--jaeger.tags=key=value,envVar1=${envKey1:defaultVal1},envVar2=${envKey2:defaultVal2}",
 	})
 	require.NoError(t, err)
 
 	b := &Options{}
+	os.Setenv("envKey1", "envVal1")
 	b.InitFromViper(v)
+
+	expectedTags := map[string]string{
+		"key"     : "value",
+		"envVar1" : "envVal1",
+		"envVar2" : "defaultVal2",
+	}
+
 	assert.Equal(t, Type("grpc"), b.ReporterType)
+	assert.Equal(t, expectedTags, b.AgentTags)
+	os.Unsetenv("envKey1")
 }

cmd/agent/app/reporter/grpc/collector_proxy.go

@@ -42,7 +42,7 @@ type ProxyBuilder struct {
 var systemCertPool = x509.SystemCertPool // to allow overriding in unit test
 
 // NewCollectorProxy creates ProxyBuilder
-func NewCollectorProxy(o *Options, mFactory metrics.Factory, logger *zap.Logger) (*ProxyBuilder, error) {
+func NewCollectorProxy(o *Options, agentTags map[string]string, mFactory metrics.Factory, logger *zap.Logger) (*ProxyBuilder, error) {
 	if len(o.CollectorHostPort) == 0 {
 		return nil, errors.New("could not create collector proxy, address is missing")
 	}
@@ -87,7 +87,7 @@ func NewCollectorProxy(o *Options, mFactory metrics.Factory, logger *zap.Logger)
 	grpcMetrics := mFactory.Namespace(metrics.NSOptions{Name: "", Tags: map[string]string{"protocol": "grpc"}})
 	return &ProxyBuilder{
 		conn:     conn,
-		reporter: aReporter.WrapWithMetrics(NewReporter(conn, logger), grpcMetrics),
+		reporter: aReporter.WrapWithMetrics(NewReporter(conn, agentTags, logger), grpcMetrics),
 		manager:  configmanager.WrapWithMetrics(grpcManager.NewConfigManager(conn), grpcMetrics)}, nil
 }
 

cmd/agent/app/reporter/grpc/collector_proxy_test.go

@@ -50,7 +50,7 @@ iPKnCkzNgxMzQtwdgpAOXIAqXyNibvyOAv1C+3QSMLKbuPEHaIxlCuvl1suX/g25
 -----END CERTIFICATE-----`
 
 func TestProxyBuilderMissingAddress(t *testing.T) {
-	proxy, err := NewCollectorProxy(&Options{}, metrics.NullFactory, zap.NewNop())
+	proxy, err := NewCollectorProxy(&Options{}, nil, metrics.NullFactory, zap.NewNop())
 	require.Nil(t, proxy)
 	assert.EqualError(t, err, "could not create collector proxy, address is missing")
 }
@@ -99,7 +99,7 @@ func TestProxyBuilder(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			proxy, err := NewCollectorProxy(test.proxyOptions, metrics.NullFactory, zap.NewNop())
+			proxy, err := NewCollectorProxy(test.proxyOptions, nil, metrics.NullFactory, zap.NewNop())
 			if test.expectError {
 				require.Error(t, err)
 			} else {
@@ -125,7 +125,7 @@ func TestSystemCertPoolError(t *testing.T) {
 	_, err := NewCollectorProxy(&Options{
 		CollectorHostPort: []string{"foo", "bar"},
 		TLS:               true,
-	}, nil, nil)
+	}, nil, nil, nil)
 	assert.Equal(t, fakeErr, err)
 }
 
@@ -142,7 +142,7 @@ func TestMultipleCollectors(t *testing.T) {
 	defer s2.Stop()
 
 	mFactory := metricstest.NewFactory(time.Microsecond)
-	proxy, err := NewCollectorProxy(&Options{CollectorHostPort: []string{addr1.String(), addr2.String()}}, mFactory, zap.NewNop())
+	proxy, err := NewCollectorProxy(&Options{CollectorHostPort: []string{addr1.String(), addr2.String()}}, nil, mFactory, zap.NewNop())
 	require.NoError(t, err)
 	require.NotNil(t, proxy)
 	assert.NotNil(t, proxy.GetReporter())