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

Bump opentelemetry-collector to v0.14.0 #2617

Merged
merged 5 commits into from
Nov 9, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 4 additions & 4 deletions cmd/opentelemetry/app/exporter/badgerexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,19 +89,19 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {
}
}

// CreateTraceExporter creates Jaeger Cassandra trace exporter.
// CreateTracesExporter creates Jaeger Cassandra trace exporter.
// This function implements OTEL component.ExporterFactory interface.
func (f Factory) CreateTraceExporter(
func (f Factory) CreateTracesExporter(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe, what happens when so many people contributing are not native speakers. "trace exporter" was grammatically correct name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a jab at you, @Vemmy124 , but on the upstream change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confess it did sound strange to my ears, but I'm not a native speaker myself, so, thought it was a problem with my ears :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that was bogdandrutu's idea open-telemetry/opentelemetry-collector#1974

Copy link
Contributor Author

@pkositsyn pkositsyn Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe it's really the problem of me being not native speaker, but I agree with him, because the component consumes the structure named pdata.Traces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter what exact data type is passed, a component that exports traces (plural) is "trace exporter" (noun used as adjective in singular form).

_ context.Context,
params component.ExporterCreateParams,
cfg configmodels.Exporter,
) (component.TraceExporter, error) {
) (component.TracesExporter, error) {
config := cfg.(*Config)
factory, err := f.createStorageFactory(params, config)
if err != nil {
return nil, err
}
return exporter.NewSpanWriterExporter(cfg, factory,
return exporter.NewSpanWriterExporter(cfg, params, factory,
exporterhelper.WithTimeout(config.TimeoutSettings),
exporterhelper.WithQueue(config.QueueSettings),
exporterhelper.WithRetry(config.RetrySettings))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestCreateTraceExporter(t *testing.T) {
factory := NewFactory(func() *badger.Options {
return opts
})
exporter, err := factory.CreateTraceExporter(context.Background(), component.ExporterCreateParams{Logger: zap.NewNop()}, factory.CreateDefaultConfig())
exporter, err := factory.CreateTracesExporter(context.Background(), component.ExporterCreateParams{Logger: zap.NewNop()}, factory.CreateDefaultConfig())
require.NoError(t, err)
assert.NotNil(t, exporter)
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/opentelemetry/app/exporter/cassandraexporter/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ import (
)

// new creates Cassandra exporter/storage
func new(config *Config, params component.ExporterCreateParams) (component.TraceExporter, error) {
func new(config *Config, params component.ExporterCreateParams) (component.TracesExporter, error) {
f := cassandra.NewFactory()
f.InitFromOptions(&config.Options)

err := f.Initialize(metrics.NullFactory, params.Logger)
if err != nil {
return nil, err
}
return exporter.NewSpanWriterExporter(config, f,
return exporter.NewSpanWriterExporter(config, params, f,
exporterhelper.WithTimeout(config.TimeoutSettings),
exporterhelper.WithQueue(config.QueueSettings),
exporterhelper.WithRetry(config.RetrySettings))
Expand Down
6 changes: 3 additions & 3 deletions cmd/opentelemetry/app/exporter/cassandraexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,13 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {
}
}

// CreateTraceExporter creates Jaeger Cassandra trace exporter.
// CreateTracesExporter creates Jaeger Cassandra trace exporter.
// This function implements OTEL component.ExporterFactory interface.
func (f Factory) CreateTraceExporter(
func (f Factory) CreateTracesExporter(
_ context.Context,
params component.ExporterCreateParams,
cfg configmodels.Exporter,
) (component.TraceExporter, error) {
) (component.TracesExporter, error) {
config := cfg.(*Config)
return new(config, params)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestCreateTraceExporter(t *testing.T) {
factory := Factory{OptionsFactory: func() *cassandra.Options {
return opts
}}
exporter, err := factory.CreateTraceExporter(context.Background(), component.ExporterCreateParams{}, factory.CreateDefaultConfig())
exporter, err := factory.CreateTracesExporter(context.Background(), component.ExporterCreateParams{}, factory.CreateDefaultConfig())
require.Nil(t, exporter)
assert.Contains(t, err.Error(), "gocql: unable to create session")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package esmodeltranslator

import (
"encoding/hex"
"errors"
"fmt"
"strconv"
Expand Down Expand Up @@ -167,7 +168,7 @@ func toTime(nano pdata.TimestampUnixNano) time.Time {
}

func references(links pdata.SpanLinkSlice, parentSpanID pdata.SpanID, traceID dbmodel.TraceID) ([]dbmodel.Reference, error) {
parentSpanIDSet := len(parentSpanID.Bytes()) != 0
parentSpanIDSet := parentSpanID.IsValid()
if !parentSpanIDSet && links.Len() == 0 {
return emptyReferenceList, nil
}
Expand Down Expand Up @@ -222,24 +223,20 @@ func references(links pdata.SpanLinkSlice, parentSpanID pdata.SpanID, traceID db
}

func convertSpanID(spanID pdata.SpanID) (dbmodel.SpanID, error) {
spanIDInt, err := tracetranslator.BytesToUInt64SpanID(spanID.Bytes())
if err != nil {
return "", err
}
if spanIDInt == 0 {
if !spanID.IsValid() {
return "", errZeroSpanID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels misplaced to me. This kind of validation should've been done way earlier in the pipeline. The ES backend itself doesn't actually care if IDs are all zeroes, it would still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're definitely right, but practically having this error handled here can prevent some time of unnecessary debugging in case of zero IDs and missing validation earlier. I believe this error arises systematically, so anyway there will be collisions by ID and this will only implicitly indicate zero ID error.

}
return dbmodel.SpanID(fmt.Sprintf("%016x", spanIDInt)), nil
src := spanID.Bytes()
dst := make([]byte, hex.EncodedLen(len(src)))
hex.Encode(dst, src[:])
return dbmodel.SpanID(dst), nil
}

func convertTraceID(traceID pdata.TraceID) (dbmodel.TraceID, error) {
high, low, err := tracetranslator.BytesToUInt64TraceID(traceID.Bytes())
if err != nil {
return "", err
}
if low == 0 && high == 0 {
if !traceID.IsValid() {
return "", errZeroTraceID
}
high, low := tracetranslator.BytesToUInt64TraceID(traceID.Bytes())
return dbmodel.TraceID(traceIDToString(high, low)), nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package esmodeltranslator

import (
"encoding/binary"
"fmt"
"testing"
"time"
Expand All @@ -30,8 +29,9 @@ import (
)

var (
traceID = pdata.NewTraceID([]byte("0123456789abcdef"))
spanID = pdata.NewSpanID([]byte("01234567"))
traceID = pdata.NewTraceID([16]byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F})
spanID = pdata.NewSpanID([8]byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07})
)

func TestAttributeToKeyValue(t *testing.T) {
Expand Down Expand Up @@ -129,7 +129,9 @@ func TestConvertSpan(t *testing.T) {
span.Links().Resize(1)
span.Links().At(0).InitEmpty()
span.Links().At(0).SetSpanID(spanID)
span.Links().At(0).SetTraceID(traceID)
traceIDZeroHigh := pdata.NewTraceID([16]byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07})
span.Links().At(0).SetTraceID(traceIDZeroHigh)

c := &Translator{
tagKeysAsFields: map[string]bool{"toTagMap": true},
Expand All @@ -143,15 +145,15 @@ func TestConvertSpan(t *testing.T) {
Resource: resource,
InstrumentationLibrary: traces.ResourceSpans().At(0).InstrumentationLibrarySpans().At(0).InstrumentationLibrary(),
DBSpan: &dbmodel.Span{
TraceID: "30313233343536373839616263646566",
SpanID: "3031323334353637",
TraceID: "000102030405060708090a0b0c0d0e0f",
SpanID: "0001020304050607",
StartTime: 1000,
Duration: 1000,
OperationName: "root",
StartTimeMillis: 1,
Tags: []dbmodel.KeyValue{
{Key: "span.kind", Type: dbmodel.StringType, Value: "client"},
{Key: "status.code", Type: dbmodel.StringType, Value: "STATUS_CODE_CANCELLED"},
{Key: "status.code", Type: dbmodel.StringType, Value: "STATUS_CODE_OK"},
{Key: "error", Type: dbmodel.BoolType, Value: "true"},
{Key: "status.message", Type: dbmodel.StringType, Value: "messagetext"},
{Key: "foo", Type: dbmodel.BoolType, Value: "true"},
Expand All @@ -163,8 +165,8 @@ func TestConvertSpan(t *testing.T) {
{Key: "event", Value: "eventName", Type: dbmodel.StringType},
{Key: "foo", Value: "bar", Type: dbmodel.StringType}}, Timestamp: 500}},
References: []dbmodel.Reference{
{SpanID: "3031323334353637", TraceID: "30313233343536373839616263646566", RefType: dbmodel.ChildOf},
{SpanID: "3031323334353637", TraceID: "30313233343536373839616263646566", RefType: dbmodel.FollowsFrom}},
{SpanID: "0001020304050607", TraceID: "000102030405060708090a0b0c0d0e0f", RefType: dbmodel.ChildOf},
{SpanID: "0001020304050607", TraceID: "0001020304050607", RefType: dbmodel.FollowsFrom}},
Process: dbmodel.Process{
ServiceName: "myservice",
Tags: []dbmodel.KeyValue{{Key: "num", Value: "16.66", Type: dbmodel.Float64Type}},
Expand All @@ -173,6 +175,12 @@ func TestConvertSpan(t *testing.T) {
}, spansData[0])
}

func BenchmarkConvertSpanID(b *testing.B) {
for i := 0; i < b.N; i++ {
_, _ = convertSpanID(spanID)
}
}

func TestSpanEmptyRef(t *testing.T) {
traces := traces("myservice")
span := addSpan(traces, "root", traceID, spanID)
Expand All @@ -190,8 +198,8 @@ func TestSpanEmptyRef(t *testing.T) {
Resource: traces.ResourceSpans().At(0).Resource(),
InstrumentationLibrary: traces.ResourceSpans().At(0).InstrumentationLibrarySpans().At(0).InstrumentationLibrary(),
DBSpan: &dbmodel.Span{
TraceID: "30313233343536373839616263646566",
SpanID: "3031323334353637",
TraceID: "000102030405060708090a0b0c0d0e0f",
SpanID: "0001020304050607",
StartTime: 1000,
Duration: 1000,
OperationName: "root",
Expand All @@ -215,26 +223,16 @@ func TestEmpty(t *testing.T) {
}

func TestErrorIDs(t *testing.T) {
zero64Bytes := make([]byte, 16)
binary.LittleEndian.PutUint64(zero64Bytes, 0)
binary.LittleEndian.PutUint64(zero64Bytes, 0)
var zero64Bytes [16]byte
var zero32Bytes [8]byte
tests := []struct {
spanID pdata.SpanID
traceID pdata.TraceID
err string
}{
{
traceID: pdata.NewTraceID([]byte("invalid-%")),
err: "TraceID does not have 16 bytes",
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why these tests are removed - give the current code there would still be some kind of error returned from converters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But TraceID is now exactly [16]byte. Why would the error be there even if only several first bytes are set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are silently truncated according to this PR conversations open-telemetry/opentelemetry-collector#2001

{
traceID: traceID,
spanID: pdata.NewSpanID([]byte("invalid-%")),
err: "SpanID does not have 8 bytes",
},
{
traceID: traceID,
spanID: pdata.NewSpanID(zero64Bytes[:8]),
spanID: pdata.NewSpanID(zero32Bytes),
err: errZeroSpanID.Error(),
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
)

// newExporter creates Elasticsearch exporter/storage.
func newExporter(ctx context.Context, config *Config, params component.ExporterCreateParams) (component.TraceExporter, error) {
func newExporter(ctx context.Context, config *Config, params component.ExporterCreateParams) (component.TracesExporter, error) {
esCfg := config.GetPrimary()
w, err := newEsSpanWriter(*esCfg, params.Logger, false, config.Name())
if err != nil {
Expand All @@ -38,6 +38,7 @@ func newExporter(ctx context.Context, config *Config, params component.ExporterC
}
return exporterhelper.NewTraceExporter(
config,
params.Logger,
w.WriteTraces,
exporterhelper.WithTimeout(config.TimeoutSettings),
exporterhelper.WithQueue(config.QueueSettings),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {
}
}

// CreateTraceExporter creates Jaeger Elasticsearch trace exporter.
// CreateTracesExporter creates Jaeger Elasticsearch trace exporter.
// This function implements OTEL component.ExporterFactory interface.
func (Factory) CreateTraceExporter(
func (Factory) CreateTracesExporter(
ctx context.Context,
params component.ExporterCreateParams,
cfg configmodels.Exporter,
) (component.TraceExporter, error) {
) (component.TracesExporter, error) {
esCfg, ok := cfg.(*Config)
if !ok {
return nil, fmt.Errorf("could not cast configuration to %s", TypeStr)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ func TestCreateTraceExporter(t *testing.T) {
}}
config := factory.CreateDefaultConfig().(*Config)
config.Primary.Servers = []string{"http://foobardoesnotexists.test"}
exporter, err := factory.CreateTraceExporter(context.Background(), component.ExporterCreateParams{Logger: zap.NewNop()}, config)
exporter, err := factory.CreateTracesExporter(context.Background(), component.ExporterCreateParams{Logger: zap.NewNop()}, config)
require.Nil(t, exporter)
assert.Contains(t, err.Error(), "no such host")
}

func TestCreateTraceExporter_nilConfig(t *testing.T) {
factory := &Factory{}
exporter, err := factory.CreateTraceExporter(context.Background(), component.ExporterCreateParams{}, nil)
exporter, err := factory.CreateTracesExporter(context.Background(), component.ExporterCreateParams{}, nil)
require.Nil(t, exporter)
assert.Contains(t, err.Error(), "could not cast configuration to jaeger_elasticsearch")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ const (
serviceIndexBaseName = "jaeger-service"
spanTypeName = "span"
serviceTypeName = "service"
indexDateFormat = "2006-01-02" // date format for index e.g. 2020-01-20
)

// esSpanWriter holds components required for ES span writer
Expand Down Expand Up @@ -245,7 +244,7 @@ func bulkItemsToTraces(bulkItems []bulkItem) pdata.Traces {
rss := traces.ResourceSpans().At(i)
if !spanData.Resource.IsNil() {
rss.Resource().InitEmpty()
rss.Resource().Attributes().InitFromAttributeMap(spanData.Resource.Attributes())
spanData.Resource.Attributes().CopyTo(rss.Resource().Attributes())
}
rss.InstrumentationLibrarySpans().Resize(1)
ispans := rss.InstrumentationLibrarySpans().At(0)
Expand Down
4 changes: 2 additions & 2 deletions cmd/opentelemetry/app/exporter/grpcpluginexporter/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ import (
)

// new creates gRPC exporter/storage.
func new(config *Config, params component.ExporterCreateParams) (component.TraceExporter, error) {
func new(config *Config, params component.ExporterCreateParams) (component.TracesExporter, error) {
factory := storageGrpc.NewFactory()
factory.InitFromOptions(config.Options)
err := factory.Initialize(metrics.NullFactory, params.Logger)
if err != nil {
return nil, err
}
return storageOtelExporter.NewSpanWriterExporter(&config.ExporterSettings, factory,
return storageOtelExporter.NewSpanWriterExporter(&config.ExporterSettings, params, factory,
exporterhelper.WithTimeout(config.TimeoutSettings),
exporterhelper.WithQueue(config.QueueSettings),
exporterhelper.WithRetry(config.RetrySettings))
Expand Down
6 changes: 3 additions & 3 deletions cmd/opentelemetry/app/exporter/grpcpluginexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {
}
}

// CreateTraceExporter creates Jaeger gRPC trace exporter.
// CreateTracesExporter creates Jaeger gRPC trace exporter.
// This function implements OTEL component.ExporterFactory interface.
func (f Factory) CreateTraceExporter(
func (f Factory) CreateTracesExporter(
_ context.Context,
params component.ExporterCreateParams,
cfg configmodels.Exporter,
) (component.TraceExporter, error) {
) (component.TracesExporter, error) {
grpcCfg := cfg.(*Config)
return new(grpcCfg, params)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestCreateTraceExporter(t *testing.T) {
factory := &Factory{OptionsFactory: func() *storageGrpc.Options {
return opts
}}
exporter, err := factory.CreateTraceExporter(context.Background(), component.ExporterCreateParams{}, factory.CreateDefaultConfig())
exporter, err := factory.CreateTracesExporter(context.Background(), component.ExporterCreateParams{}, factory.CreateDefaultConfig())
require.Nil(t, exporter)
assert.Contains(t, err.Error(), "error attempting to connect to plugin rpc client: fork/exec : no such file or directory")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {
return cfg
}

// CreateTraceExporter creates Jaeger trace exporter.
// CreateTracesExporter creates Jaeger trace exporter.
// This function implements OTEL component.ExporterFactory interface.
func (f Factory) CreateTraceExporter(
func (f Factory) CreateTracesExporter(
ctx context.Context,
params component.ExporterCreateParams,
cfg configmodels.Exporter,
) (component.TraceExporter, error) {
return f.Wrapped.CreateTraceExporter(ctx, params, cfg)
) (component.TracesExporter, error) {
return f.Wrapped.CreateTracesExporter(ctx, params, cfg)
}

// CreateMetricsExporter creates a metrics exporter based on provided config.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ func TestDefaultValueFromViper(t *testing.T) {
cfg := f.CreateDefaultConfig().(*jaegerexporter.Config)

qs := exporterhelper.CreateDefaultQueueSettings()
qs.Enabled = false
assert.Equal(t, &jaegerexporter.Config{
ExporterSettings: configmodels.ExporterSettings{
TypeVal: "jaeger",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,14 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {
return cfg
}

// CreateTraceExporter creates Jaeger trace exporter.
// CreateTracesExporter creates Jaeger trace exporter.
// This function implements OTEL component.ExporterFactory interface.
func (f Factory) CreateTraceExporter(
func (f Factory) CreateTracesExporter(
ctx context.Context,
params component.ExporterCreateParams,
cfg configmodels.Exporter,
) (component.TraceExporter, error) {
return f.Wrapped.CreateTraceExporter(ctx, params, cfg)
) (component.TracesExporter, error) {
return f.Wrapped.CreateTracesExporter(ctx, params, cfg)
}

// CreateMetricsExporter creates a metrics exporter based on provided config.
Expand Down
Loading