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

Change NewSplitDriver paramater and initialization #1798

Merged
merged 25 commits into from Apr 23, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
dd93ab7
Change NewSplitDriver paramater and initialization
bryan-aguilar Apr 6, 2021
54f68d1
Update CHANGELOG.md
bryan-aguilar Apr 7, 2021
d47871b
Update CHANGELOG.md
bryan-aguilar Apr 7, 2021
81fcd4e
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 7, 2021
e4bdb6d
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 8, 2021
6743e4d
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 8, 2021
f1c17af
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 8, 2021
e0b1b2b
Move splitdriver option into options.go and rename
bryan-aguilar Apr 8, 2021
06fdd2d
Merge remote-tracking branch 'upstream/main' into NewSplitDriver_Update
bryan-aguilar Apr 8, 2021
dd623a8
Merge branch 'working' into NewSplitDriver_Update
bryan-aguilar Apr 12, 2021
cfa6a91
Update CHANGELOG.md
bryan-aguilar Apr 19, 2021
9493d37
Change option name and nil test to snapshots
bryan-aguilar Apr 19, 2021
8b9bc71
Merge branch 'main' into NewSplitDriver_Update
bryan-aguilar Apr 19, 2021
2694914
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 19, 2021
419e4fc
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 19, 2021
5e6841b
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 19, 2021
06acaa7
Update exporters/otlp/protocoldriver.go
bryan-aguilar Apr 19, 2021
6ff5b28
Update exporters/otlp/options.go
bryan-aguilar Apr 19, 2021
4f5547d
Update exporters/otlp/options.go
bryan-aguilar Apr 19, 2021
845fc07
Update exporters/otlp/options.go
bryan-aguilar Apr 19, 2021
c9d4971
Update exporters/otlp/options.go
bryan-aguilar Apr 22, 2021
2a081bb
Change SplitDriverOption to match spec
bryan-aguilar Apr 22, 2021
2f0aa62
Merge branch 'main' into NewSplitDriver_Update
bryan-aguilar Apr 22, 2021
c9b5d1e
Merge branch 'main' into NewSplitDriver_Update
Aneurysm9 Apr 23, 2021
d6a784d
Update changelog entry
bryan-aguilar Apr 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -68,6 +68,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
It no longer is a conglomerate of itself, events, and link attributes that have been dropped. (#1771)
- Make `ExportSpans` in Jaeger Exporter honor context deadline. (#1773)
- The `go.opentelemetry.io/otel/sdk/export/trace` package is merged into the `go.opentelemetry.io/otel/sdk/trace` package. (#1778)
- Make `NewSplitDriver` from `go.opentelemetry.io/otel/exporters/otlp` take variadic arguments instead of a `SplitConfig` item.
`NewSplitDriver` now automically implements an internal `noopDriver` for `SplitConfig` fields that are not initialized. (#1798)
Aneurysm9 marked this conversation as resolved.
Show resolved Hide resolved

### Removed

Expand Down
6 changes: 1 addition & 5 deletions exporters/otlp/example_test.go
Expand Up @@ -35,11 +35,7 @@ func ExampleNewExporter() {
tracesDriver := otlpgrpc.NewDriver(
// Configure traces driver here
)
config := otlp.SplitConfig{
ForMetrics: metricsDriver,
ForTraces: tracesDriver,
}
driver := otlp.NewSplitDriver(config)
driver := otlp.NewSplitDriver(otlp.WithMetricDriver(metricsDriver), otlp.WithTraceDriver(tracesDriver))
exporter, err := otlp.NewExporter(ctx, driver) // Configure as needed.
if err != nil {
log.Fatalf("failed to create exporter: %v", err)
Expand Down
19 changes: 19 additions & 0 deletions exporters/otlp/options.go
Expand Up @@ -43,3 +43,22 @@ func WithMetricExportKindSelector(selector metricsdk.ExportKindSelector) Exporte
cfg.exportKindSelector = selector
}
}

// ProtocolDriverOption provides options for setting up a split driver
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
type ProtocolDriverOption func(d *splitDriver)
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved

// WithMetricDriver allows one to set the driver used for metrics
// in a SplitDriver
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
func WithMetricDriver(dr ProtocolDriver) ProtocolDriverOption {
return func(d *splitDriver) {
d.metric = dr
}
}

// WithTraceDriver allows one to set the driver used for traces
// in a SplitDriver
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
func WithTraceDriver(dr ProtocolDriver) ProtocolDriverOption {
return func(d *splitDriver) {
d.trace = dr
}
}
149 changes: 104 additions & 45 deletions exporters/otlp/otlp_test.go
Expand Up @@ -283,46 +283,109 @@ func TestNewExportPipeline(t *testing.T) {
}

func TestSplitDriver(t *testing.T) {
driverTraces := &stubProtocolDriver{}
driverMetrics := &stubProtocolDriver{}
config := otlp.SplitConfig{
ForMetrics: driverMetrics,
ForTraces: driverTraces,
}
driver := otlp.NewSplitDriver(config)
ctx := context.Background()
assert.NoError(t, driver.Start(ctx))
assert.Equal(t, 1, driverTraces.started)
assert.Equal(t, 1, driverMetrics.started)
assert.Equal(t, 0, driverTraces.stopped)
assert.Equal(t, 0, driverMetrics.stopped)
assert.Equal(t, 0, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 0, driverMetrics.metricsExported)

recordCount := 5
spanCount := 7
assert.NoError(t, driver.ExportMetrics(ctx, stubCheckpointSet{recordCount}, metricsdk.StatelessExportKindSelector()))
assert.NoError(t, driver.ExportTraces(ctx, stubSpanSnapshot(spanCount)))
assert.Len(t, driverTraces.rm, 0)
assert.Len(t, driverTraces.rs, spanCount)
assert.Len(t, driverMetrics.rm, recordCount)
assert.Len(t, driverMetrics.rs, 0)
assert.Equal(t, 1, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 1, driverMetrics.metricsExported)

assert.NoError(t, driver.Stop(ctx))
assert.Equal(t, 1, driverTraces.started)
assert.Equal(t, 1, driverMetrics.started)
assert.Equal(t, 1, driverTraces.stopped)
assert.Equal(t, 1, driverMetrics.stopped)
assert.Equal(t, 1, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 1, driverMetrics.metricsExported)
t.Run("with metric/trace drivers configured", func(t *testing.T) {
driverTraces := &stubProtocolDriver{}
driverMetrics := &stubProtocolDriver{}

driver := otlp.NewSplitDriver(otlp.WithMetricDriver(driverMetrics), otlp.WithTraceDriver(driverTraces))
ctx := context.Background()
assert.NoError(t, driver.Start(ctx))
assert.Equal(t, 1, driverTraces.started)
assert.Equal(t, 1, driverMetrics.started)
assert.Equal(t, 0, driverTraces.stopped)
assert.Equal(t, 0, driverMetrics.stopped)
assert.Equal(t, 0, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 0, driverMetrics.metricsExported)

recordCount := 5
spanCount := 7
assert.NoError(t, driver.ExportMetrics(ctx, stubCheckpointSet{recordCount}, metricsdk.StatelessExportKindSelector()))
assert.NoError(t, driver.ExportTraces(ctx, stubSpanSnapshot(spanCount)))
assert.Len(t, driverTraces.rm, 0)
assert.Len(t, driverTraces.rs, spanCount)
assert.Len(t, driverMetrics.rm, recordCount)
assert.Len(t, driverMetrics.rs, 0)
assert.Equal(t, 1, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 1, driverMetrics.metricsExported)

assert.NoError(t, driver.Stop(ctx))
assert.Equal(t, 1, driverTraces.started)
assert.Equal(t, 1, driverMetrics.started)
assert.Equal(t, 1, driverTraces.stopped)
assert.Equal(t, 1, driverMetrics.stopped)
assert.Equal(t, 1, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 1, driverMetrics.metricsExported)
})

t.Run("with just metric driver", func(t *testing.T) {
driverMetrics := &stubProtocolDriver{}

driver := otlp.NewSplitDriver(otlp.WithMetricDriver(driverMetrics))
ctx := context.Background()
assert.NoError(t, driver.Start(ctx))

assert.Equal(t, 1, driverMetrics.started)
assert.Equal(t, 0, driverMetrics.stopped)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 0, driverMetrics.metricsExported)

recordCount := 5
assert.NoError(t, driver.ExportMetrics(ctx, stubCheckpointSet{recordCount}, metricsdk.StatelessExportKindSelector()))
assert.NoError(t, driver.ExportTraces(ctx, nil))
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this makes no actual difference in the code, but could we change these nils to actual snapshots like the test above?
This is just to make sure future iterations of this can't shortcut nil or 0 len inputs and still pass but fail with real data.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense to just abstract this part of the repeated code into a separate function that can be called and will ensure this uniform call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense. I will work on refining the test cases, was trying to get the core change out for review. Do you want me to change this to a draft PR @MrAlias ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bryan-aguilar if you think the changes needed will be greater than just the two tests here doing in another PR sounds reasonable, but if not including the changes here seems valid.

assert.Len(t, driverMetrics.rm, recordCount)
assert.Len(t, driverMetrics.rs, 0)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 1, driverMetrics.metricsExported)

assert.NoError(t, driver.Stop(ctx))
assert.Equal(t, 1, driverMetrics.started)
assert.Equal(t, 1, driverMetrics.stopped)
assert.Equal(t, 0, driverMetrics.tracesExported)
assert.Equal(t, 1, driverMetrics.metricsExported)
})

t.Run("with just trace driver", func(t *testing.T) {
driverTraces := &stubProtocolDriver{}

driver := otlp.NewSplitDriver(otlp.WithTraceDriver(driverTraces))
ctx := context.Background()
assert.NoError(t, driver.Start(ctx))
assert.Equal(t, 1, driverTraces.started)
assert.Equal(t, 0, driverTraces.stopped)
assert.Equal(t, 0, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)

spanCount := 7
assert.NoError(t, driver.ExportMetrics(ctx, nil, nil))
assert.NoError(t, driver.ExportTraces(ctx, stubSpanSnapshot(spanCount)))
assert.Len(t, driverTraces.rm, 0)
assert.Len(t, driverTraces.rs, spanCount)
assert.Equal(t, 1, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)

assert.NoError(t, driver.Stop(ctx))
assert.Equal(t, 1, driverTraces.started)
assert.Equal(t, 1, driverTraces.stopped)
assert.Equal(t, 1, driverTraces.tracesExported)
assert.Equal(t, 0, driverTraces.metricsExported)
})

t.Run("with no drivers configured", func(t *testing.T) {

driver := otlp.NewSplitDriver()
ctx := context.Background()
assert.NoError(t, driver.Start(ctx))

assert.NoError(t, driver.ExportMetrics(ctx, nil, nil))
assert.NoError(t, driver.ExportTraces(ctx, nil))
assert.NoError(t, driver.Stop(ctx))
})
}

func TestSplitDriverFail(t *testing.T) {
Expand Down Expand Up @@ -357,11 +420,7 @@ func TestSplitDriverFail(t *testing.T) {
injectedStartError: errStartMetric,
injectedStopError: errStopMetric,
}
config := otlp.SplitConfig{
ForMetrics: driverMetrics,
ForTraces: driverTraces,
}
driver := otlp.NewSplitDriver(config)
driver := otlp.NewSplitDriver(otlp.WithMetricDriver(driverMetrics), otlp.WithTraceDriver(driverTraces))
errStart := driver.Start(ctx)
if shouldStartFail {
assert.Error(t, errStart)
Expand Down
6 changes: 1 addition & 5 deletions exporters/otlp/otlpgrpc/example_test.go
Expand Up @@ -143,11 +143,7 @@ func Example_withDifferentSignalCollectors() {
otlpgrpc.WithInsecure(),
otlpgrpc.WithEndpoint("localhost:30082"),
)
splitCfg := otlp.SplitConfig{
ForMetrics: metricsDriver,
ForTraces: tracesDriver,
}
driver := otlp.NewSplitDriver(splitCfg)
driver := otlp.NewSplitDriver(otlp.WithMetricDriver(metricsDriver), otlp.WithTraceDriver(tracesDriver))
ctx := context.Background()
exp, err := otlp.NewExporter(ctx, driver)
if err != nil {
Expand Down
6 changes: 1 addition & 5 deletions exporters/otlp/otlpgrpc/otlp_integration_test.go
Expand Up @@ -549,11 +549,7 @@ func TestMultiConnectionDriver(t *testing.T) {

tracesDriver := otlpgrpc.NewDriver(optsTraces...)
metricsDriver := otlpgrpc.NewDriver(optsMetrics...)
splitCfg := otlp.SplitConfig{
ForMetrics: metricsDriver,
ForTraces: tracesDriver,
}
driver := otlp.NewSplitDriver(splitCfg)
driver := otlp.NewSplitDriver(otlp.WithMetricDriver(metricsDriver), otlp.WithTraceDriver(tracesDriver))
ctx := context.Background()
exp, err := otlp.NewExporter(ctx, driver)
if err != nil {
Expand Down
39 changes: 35 additions & 4 deletions exporters/otlp/protocoldriver.go
Expand Up @@ -66,16 +66,27 @@ type splitDriver struct {
trace ProtocolDriver
}

// noopDriver implements the ProtocolDriver interface and
// is used internally to implement split drivers that do not have
// all drivers configured.
type noopDriver struct{}

var _ ProtocolDriver = (*noopDriver)(nil)

var _ ProtocolDriver = (*splitDriver)(nil)

// NewSplitDriver creates a protocol driver which contains two other
// protocol drivers and will forward traces to one of them and metrics
// to another.
func NewSplitDriver(cfg SplitConfig) ProtocolDriver {
return &splitDriver{
metric: cfg.ForMetrics,
trace: cfg.ForTraces,
func NewSplitDriver(opts ...ProtocolDriverOption) ProtocolDriver {
driver := splitDriver{
metric: &noopDriver{},
trace: &noopDriver{},
}
for _, opt := range opts {
opt(&driver)
}
return &driver
}

// Start implements ProtocolDriver. It starts both drivers at the same
Expand Down Expand Up @@ -143,3 +154,23 @@ func (d *splitDriver) ExportMetrics(ctx context.Context, cps metricsdk.Checkpoin
func (d *splitDriver) ExportTraces(ctx context.Context, ss []*tracesdk.SpanSnapshot) error {
return d.trace.ExportTraces(ctx, ss)
}

// Start does nothing
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
func (d *noopDriver) Start(ctx context.Context) error {
return nil
}

// Stop does nothing
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
func (d *noopDriver) Stop(ctx context.Context) error {
return nil
}

// ExportMetrics does nothing
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
func (d *noopDriver) ExportMetrics(ctx context.Context, cps metricsdk.CheckpointSet, selector metricsdk.ExportKindSelector) error {
return nil
}

// Export traces does nothing
bryan-aguilar marked this conversation as resolved.
Show resolved Hide resolved
func (d *noopDriver) ExportTraces(ctx context.Context, ss []*tracesdk.SpanSnapshot) error {
return nil
}