From 23422c56df5578ae4f70f8bd5c0225dbad6be388 Mon Sep 17 00:00:00 2001 From: IrisTuntun <73505006+IrisTuntun@users.noreply.github.com> Date: Tue, 6 Apr 2021 15:03:32 -0700 Subject: [PATCH] Remove process config for Jaeger exporter (#1776) * remove process config for Jaeger exporter * remove process config for Jaeger exporter * Add CHANGELOG.md back * Add CHANGLOG.md changes back * fill in PR number in CHANGELOG * Update CHANGELOG.md Co-authored-by: Tyler Yahn Co-authored-by: Tyler Yahn --- CHANGELOG.md | 2 + exporters/trace/jaeger/env.go | 33 ------- exporters/trace/jaeger/env_test.go | 121 -------------------------- exporters/trace/jaeger/jaeger.go | 39 ++------- exporters/trace/jaeger/jaeger_test.go | 88 ++----------------- 5 files changed, 16 insertions(+), 267 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ecd8d49719..4b01183b4f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `HasRemoteParent` field of the `"go.opentelemetry.io/otel/sdk/trace".SamplingParameters` is removed. This field is redundant to the information returned from the `Remote` method of the `SpanContext` held in the `ParentContext` field. (#1749) - The `trace.FlagsDebug` and `trace.FlagsDeferred` constants have been removed and will be localized to the B3 propagator. (#1770) +- Remove `Process` configuration, `WithProcessFromEnv` and `ProcessFromEnv`, from the Jaeger exporter package. + The information that could be configured in the `Process` struct should be configured in a `Resource` instead. (#1776) ## [0.19.0] - 2021-03-18 diff --git a/exporters/trace/jaeger/env.go b/exporters/trace/jaeger/env.go index b9f53c367ee..52fd4168f6d 100644 --- a/exporters/trace/jaeger/env.go +++ b/exporters/trace/jaeger/env.go @@ -20,7 +20,6 @@ import ( "strconv" "strings" - "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" ) @@ -71,38 +70,6 @@ func WithDisabledFromEnv() Option { } } -// ProcessFromEnv parse environment variables into jaeger exporter's Process. -// It will return a nil tag slice if the environment variable JAEGER_TAGS is malformed. -func ProcessFromEnv() Process { - var p Process - if e := os.Getenv(envServiceName); e != "" { - p.ServiceName = e - } - if e := os.Getenv(envTags); e != "" { - tags, err := parseTags(e) - if err != nil { - otel.Handle(err) - } else { - p.Tags = tags - } - } - - return p -} - -// WithProcessFromEnv uses environment variables and overrides jaeger exporter's Process. -func WithProcessFromEnv() Option { - return func(o *options) { - p := ProcessFromEnv() - if p.ServiceName != "" { - o.Process.ServiceName = p.ServiceName - } - if len(p.Tags) != 0 { - o.Process.Tags = p.Tags - } - } -} - var errTagValueNotFound = errors.New("missing tag value") var errTagEnvironmentDefaultValueNotFound = errors.New("missing default value for tag environment value") diff --git a/exporters/trace/jaeger/env_test.go b/exporters/trace/jaeger/env_test.go index f55dcf15362..5a30d6abbda 100644 --- a/exporters/trace/jaeger/env_test.go +++ b/exporters/trace/jaeger/env_test.go @@ -229,13 +229,10 @@ func TestNewRawExporterWithEnv(t *testing.T) { WithCollectorEndpoint(CollectorEndpointFromEnv(), WithCollectorEndpointOptionFromEnv()), WithDisabled(true), WithDisabledFromEnv(), - WithProcessFromEnv(), ) assert.NoError(t, err) assert.Equal(t, false, exp.o.Disabled) - assert.EqualValues(t, serviceName, exp.o.Process.ServiceName) - assert.Len(t, exp.o.Process.Tags, 1) require.IsType(t, &collectorUploader{}, exp.uploader) uploader := exp.uploader.(*collectorUploader) @@ -276,8 +273,6 @@ func TestNewRawExporterWithEnvImplicitly(t *testing.T) { assert.NoError(t, err) // NewRawExporter will ignore Disabled env assert.Equal(t, true, exp.o.Disabled) - assert.EqualValues(t, serviceName, exp.o.Process.ServiceName) - assert.Len(t, exp.o.Process.Tags, 1) require.IsType(t, &collectorUploader{}, exp.uploader) uploader := exp.uploader.(*collectorUploader) @@ -394,119 +389,3 @@ func TestWithDisabledFromEnv(t *testing.T) { }) } } - -func TestProcessFromEnv(t *testing.T) { - testCases := []struct { - name string - serviceName string - tags string - expectedProcess Process - }{ - { - name: "set process", - serviceName: "test-service", - tags: "key=value,key2=123", - expectedProcess: Process{ - ServiceName: "test-service", - Tags: []attribute.KeyValue{ - attribute.String("key", "value"), - attribute.Int64("key2", 123), - }, - }, - }, - { - name: "malformed tags", - serviceName: "test-service", - tags: "key", - expectedProcess: Process{ - ServiceName: "test-service", - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - envStore, err := ottest.SetEnvVariables(map[string]string{ - envServiceName: tc.serviceName, - envTags: tc.tags, - }) - require.NoError(t, err) - - p := ProcessFromEnv() - assert.Equal(t, tc.expectedProcess, p) - - require.NoError(t, envStore.Restore()) - }) - } -} - -func TestWithProcessFromEnv(t *testing.T) { - testCases := []struct { - name string - envServiceName string - envTags string - options options - expectedOptions options - }{ - { - name: "overwriting", - envServiceName: "service-name", - envTags: "key=value", - options: options{ - Process: Process{ - ServiceName: "old-name", - Tags: []attribute.KeyValue{ - attribute.String("old-key", "old-value"), - }, - }, - }, - expectedOptions: options{ - Process: Process{ - ServiceName: "service-name", - Tags: []attribute.KeyValue{ - attribute.String("key", "value"), - }, - }, - }, - }, - { - name: "no overwriting", - envServiceName: "", - envTags: "", - options: options{ - Process: Process{ - ServiceName: "old-name", - Tags: []attribute.KeyValue{ - attribute.String("old-key", "old-value"), - }, - }, - }, - expectedOptions: options{ - Process: Process{ - ServiceName: "old-name", - Tags: []attribute.KeyValue{ - attribute.String("old-key", "old-value"), - }, - }, - }, - }, - } - - envStore := ottest.NewEnvStore() - envStore.Record(envServiceName) - envStore.Record(envTags) - defer func() { - require.NoError(t, envStore.Restore()) - }() - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - require.NoError(t, os.Setenv(envServiceName, tc.envServiceName)) - require.NoError(t, os.Setenv(envTags, tc.envTags)) - - f := WithProcessFromEnv() - f(&tc.options) - - assert.Equal(t, tc.expectedOptions, tc.options) - }) - } -} diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go index 82327452f74..a7aa73c03aa 100644 --- a/exporters/trace/jaeger/jaeger.go +++ b/exporters/trace/jaeger/jaeger.go @@ -49,8 +49,6 @@ type Option func(*options) // options are the options to be used when initializing a Jaeger export. type options struct { - // Process contains the information about the exporting process. - Process Process // BufferMaxCount defines the total number of traces that can be buffered in memory BufferMaxCount int @@ -104,7 +102,6 @@ func NewRawExporter(endpointOption EndpointOption, opts ...Option) (*Exporter, e } o := options{} - opts = append(opts, WithProcessFromEnv()) for _, opt := range opts { opt(&o) } @@ -120,10 +117,9 @@ func NewRawExporter(endpointOption EndpointOption, opts ...Option) (*Exporter, e } e := &Exporter{ - uploader: uploader, - o: o, - defaultServiceName: defaultServiceName, - resourceFromProcess: processToResource(o.Process), + uploader: uploader, + o: o, + defaultServiceName: defaultServiceName, } bundler := bundler.NewBundler((*export.SpanSnapshot)(nil), func(bundle interface{}) { if err := e.upload(bundle.([]*export.SpanSnapshot)); err != nil { @@ -201,8 +197,7 @@ type Exporter struct { stoppedMu sync.RWMutex stopped bool - defaultServiceName string - resourceFromProcess *resource.Resource + defaultServiceName string } var _ export.SpanExporter = (*Exporter)(nil) @@ -431,7 +426,7 @@ func (e *Exporter) Flush() { } func (e *Exporter) upload(spans []*export.SpanSnapshot) error { - batchList := jaegerBatchList(spans, e.defaultServiceName, e.resourceFromProcess) + batchList := jaegerBatchList(spans, e.defaultServiceName) for _, batch := range batchList { err := e.uploader.upload(batch) if err != nil { @@ -444,7 +439,7 @@ func (e *Exporter) upload(spans []*export.SpanSnapshot) error { // jaegerBatchList transforms a slice of SpanSnapshot into a slice of jaeger // Batch. -func jaegerBatchList(ssl []*export.SpanSnapshot, defaultServiceName string, resourceFromProcess *resource.Resource) []*gen.Batch { +func jaegerBatchList(ssl []*export.SpanSnapshot, defaultServiceName string) []*gen.Batch { if len(ssl) == 0 { return nil } @@ -456,16 +451,11 @@ func jaegerBatchList(ssl []*export.SpanSnapshot, defaultServiceName string, reso continue } - newResource := ss.Resource - if resourceFromProcess != nil { - // The value from process will overwrite the value from span's resources - newResource = resource.Merge(ss.Resource, resourceFromProcess) - } - resourceKey := newResource.Equivalent() + resourceKey := ss.Resource.Equivalent() batch, bOK := batchDict[resourceKey] if !bOK { batch = &gen.Batch{ - Process: process(newResource, defaultServiceName), + Process: process(ss.Resource, defaultServiceName), Spans: []*gen.Span{}, } } @@ -508,16 +498,3 @@ func process(res *resource.Resource, defaultServiceName string) *gen.Process { return &process } - -func processToResource(process Process) *resource.Resource { - var attrs []attribute.KeyValue - if process.ServiceName != "" { - attrs = append(attrs, semconv.ServiceNameKey.String(process.ServiceName)) - } - attrs = append(attrs, process.Tags...) - - if len(attrs) == 0 { - return nil - } - return resource.NewWithAttributes(attrs...) -} diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index aad2053d364..dc7f2764038 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -729,11 +729,10 @@ func TestJaegerBatchList(t *testing.T) { now := time.Now() testCases := []struct { - name string - spanSnapshotList []*export.SpanSnapshot - defaultServiceName string - resourceFromProcess *resource.Resource - expectedBatchList []*gen.Batch + name string + spanSnapshotList []*export.SpanSnapshot + defaultServiceName string + expectedBatchList []*gen.Batch }{ { name: "no span shots", @@ -850,82 +849,7 @@ func TestJaegerBatchList(t *testing.T) { }, }, { - name: "merge resources that come from process", - spanSnapshotList: []*export.SpanSnapshot{ - { - Name: "s1", - Resource: resource.NewWithAttributes( - semconv.ServiceNameKey.String("name"), - attribute.Key("r1").String("v1"), - attribute.Key("r2").String("v2"), - ), - StartTime: now, - EndTime: now, - }, - }, - resourceFromProcess: resource.NewWithAttributes( - semconv.ServiceNameKey.String("new-name"), - attribute.Key("r1").String("v2"), - ), - expectedBatchList: []*gen.Batch{ - { - Process: &gen.Process{ - ServiceName: "new-name", - Tags: []*gen.Tag{ - {Key: "r1", VType: gen.TagType_STRING, VStr: newString("v2")}, - {Key: "r2", VType: gen.TagType_STRING, VStr: newString("v2")}, - }, - }, - Spans: []*gen.Span{ - { - OperationName: "s1", - Tags: []*gen.Tag{ - {Key: "span.kind", VType: gen.TagType_STRING, VStr: &spanKind}, - }, - StartTime: now.UnixNano() / 1000, - }, - }, - }, - }, - }, - { - name: "span's snapshot contains no service name but resourceFromProcess does", - spanSnapshotList: []*export.SpanSnapshot{ - { - Name: "s1", - Resource: resource.NewWithAttributes( - attribute.Key("r1").String("v1"), - ), - StartTime: now, - EndTime: now, - }, - nil, - }, - resourceFromProcess: resource.NewWithAttributes( - semconv.ServiceNameKey.String("new-name"), - ), - expectedBatchList: []*gen.Batch{ - { - Process: &gen.Process{ - ServiceName: "new-name", - Tags: []*gen.Tag{ - {Key: "r1", VType: gen.TagType_STRING, VStr: newString("v1")}, - }, - }, - Spans: []*gen.Span{ - { - OperationName: "s1", - Tags: []*gen.Tag{ - {Key: "span.kind", VType: gen.TagType_STRING, VStr: &spanKind}, - }, - StartTime: now.UnixNano() / 1000, - }, - }, - }, - }, - }, - { - name: "no service name in spans and resourceFromProcess", + name: "no service name in spans", spanSnapshotList: []*export.SpanSnapshot{ { Name: "s1", @@ -962,7 +886,7 @@ func TestJaegerBatchList(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - batchList := jaegerBatchList(tc.spanSnapshotList, tc.defaultServiceName, tc.resourceFromProcess) + batchList := jaegerBatchList(tc.spanSnapshotList, tc.defaultServiceName) assert.ElementsMatch(t, tc.expectedBatchList, batchList) })