Skip to content
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

Remove process config for Jaeger exporter #1776

Merged
merged 8 commits into from
Apr 6, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,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

Expand Down
33 changes: 0 additions & 33 deletions exporters/trace/jaeger/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"strconv"
"strings"

"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
)

Expand Down Expand Up @@ -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")

Expand Down
121 changes: 0 additions & 121 deletions exporters/trace/jaeger/env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
})
}
}
39 changes: 8 additions & 31 deletions exporters/trace/jaeger/jaeger.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,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
Expand Down Expand Up @@ -103,7 +101,6 @@ func NewRawExporter(endpointOption EndpointOption, opts ...Option) (*Exporter, e
}

o := options{}
opts = append(opts, WithProcessFromEnv())
for _, opt := range opts {
opt(&o)
}
Expand All @@ -119,10 +116,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 {
Expand Down Expand Up @@ -200,8 +196,7 @@ type Exporter struct {
stoppedMu sync.RWMutex
stopped bool

defaultServiceName string
resourceFromProcess *resource.Resource
defaultServiceName string
}

var _ export.SpanExporter = (*Exporter)(nil)
Expand Down Expand Up @@ -424,7 +419,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 {
Expand All @@ -437,7 +432,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
}
Expand All @@ -449,16 +444,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{},
}
}
Expand Down Expand Up @@ -501,16 +491,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...)
}
Loading