Export package #162

dmathieu · 2019-10-03T14:59:16Z

This is an attempt at implementing #77.
There are still unresolved questions for which I'm going to leave inline comments.

dmathieu · 2019-10-03T15:00:28Z

sdk/exporter/exporter.go

+
+// SyncExporter is a synchronous exporter
+type SyncExporter interface {
+	ExportSpan(context.Context, interface{})


I've added a context here, as was suggested in the examples on #77. There is never a context available when we call this method however.
This might come later, but it still seems a bit weird.

keep the context for now.

dmathieu · 2019-10-03T15:01:32Z

sdk/exporter/exporter.go

+//
+// The SpanData should not be modified.
+type BatchExporter interface {
+	ExportSpans(context.Context, []interface{})


In both ExportSpan(s) methods, I've currently set an interface, so the exporter can take multiple span structs (depending whether we're exporting to metrics or traces). This is causing a lot of weird things when having to turn SpanData back and forth to interfaces.
Maybe the SpanData struct should be moved to this package?

Thinking this through, the interface definitely seems wrong, and a metrics exporter won't be sending SpanData anyway. So I believe the struct can be moved into this package.
I'll be doing that very soon.

I agree on moving SpanData to this package and removing interface.

dmathieu · 2019-10-03T15:01:51Z

sdk/exporter/exporter.go

+}
+
+// Load loads all the registered exporters matching a specific type
+func Load(t Type) []interface{} {


Note sure Load is the best name here. Get might be better?

rghetia · 2019-10-03T17:45:51Z

sdk/exporter/exporter.go

+//
+// The SpanData should not be modified.
+type BatchExporter interface {
+	ExportSpans(context.Context, []interface{})


I agree on moving SpanData to this package and removing interface.

rghetia · 2019-10-03T17:46:09Z

sdk/exporter/exporter.go

+
+// SyncExporter is a synchronous exporter
+type SyncExporter interface {
+	ExportSpan(context.Context, interface{})


keep the context for now.

rghetia · 2019-10-03T17:47:57Z

sdk/exporter/exporter.go

+
+// Register adds to the list of Exporters that will receive sampled trace
+// spans.
+func Register(t Type, e interface{}) {


You can remove Register/Unregister/Load. As exporter will be used via SpanProcessor (Batched or Simple or something else)

freeformz · 2019-10-03T19:40:26Z

sdk/exporter/exporter.go

+)
+
+// SyncExporter is a synchronous exporter
+type SyncExporter interface {


Prefer Syncer or Sync. Otherwise when used outside of this package it's exporter.SyncExporter, which stuttering, which should generally be avoided as it's just annoying to read and write (at least iMO).

freeformz · 2019-10-03T19:41:04Z

sdk/exporter/exporter.go

+
+// All the exporter types
+const (
+	ExporterTypeSync = iota


Prefer TypeSync and TypeBatch because stuttering when used outside of the package (exporter.ExpoterTypeSync).

freeformz · 2019-10-03T19:41:49Z

sdk/exporter/exporter.go

+// not take forever to process the spans.
+//
+// The SpanData should not be modified.
+type BatchExporter interface {


Prefer Batcher over BatchExporter because stutter.

freeformz · 2019-10-03T19:42:47Z

sdk/exporter/exporter.go

+	exporters.Store(nm)
+}
+
+// Load loads all the registered exporters matching a specific type


If you keep it please consider removing the additional "loads", godoc comments should read cleanly.

rghetia · 2019-10-04T15:57:34Z

sdk/exporter/exporter.go

+	ExportSpan(context.Context, *SpanData)
+}
+
+// Batcher is a type for functions that receive sampled trace spans.


nit; s/sampled/batch of sampled/
also change
s/a single trace/a single sampled trace/

rghetia · 2019-10-04T16:04:48Z

sdk/trace/span.go

 		sps, _ := spanProcessors.Load().(spanProcessorMap)
-		mustExportOrProcess := len(sps) > 0 || (s.spanContext.IsSampled() && len(exp) > 0)
+		mustExportOrProcess := len(sps) > 0
 		// TODO(rghetia): when exporter is migrated to use processors simply check for the number
 		// of processors. Exporter will export based on sampling.


nit: please remove the TODO.

rghetia · 2019-10-04T16:05:42Z

sdk/trace/span.go

-				for e := range exp {
-					e.ExportSpan(sd)
-				}
-			}


This check should be added to SimpleSpanProcessor and BatchedSpanProcessor.

Sorry, I missed this comment. Wouldn't that change the behavior though? OnEnd is currently being executed without that check.
Also, spanContext isn't available from within the processor.

I believe you are referring to change in behavior of SSP and BSP as neither of them have the check right now. The current behavior is incorrect as it exports everything instead of just the sampled spans. SpanData has SpanContext so it should be able to do the check.

All right, I've added the check. I've been looking for ways to setup an unsampled SpanContext to no luck though. So I'm happy to get any pointers to a way to test it.

akvanhar · 2019-10-04T16:26:41Z

sdk/exporter/exporter.go

+
+// Syncer is a type for functions that receive a single trace span.
+//
+// The ExportSpan method is called synchronously.


Might be worth adding a note that ExportSpan implementations should not block indefinitely

Sure, though that was implied by the fact that the method is called synchronously.

jmacd · 2019-10-04T16:29:30Z

sdk/exporter/exporter.go

+// The ExportSpan method is called synchronously.
+//
+// The SpanData should not be modified.
+type Syncer interface {


I'd like a longer name, exporter.Synchronous sounds good.

I'm conflicted over the naming in general, though. You're moving trace-specific exporter into a general-purpose package named "exporter". Now if we want to export different kinds of data, these names are claimed for trace-specific purposes.

I agree with moving this stuff out of the SDK proper, though.

How about SpanSyncer and SpanBatcher? Then the possible naming conflict shouldn't happen.
I don't mind SpanSynchronous, though that seems unnecessarily longer now.

IMO, exporter package should be renamed to export. export.SpanSyncer sounds better than exporter.SpanSyncer.

I'm not 100% convinced about that, since the package for exporters themselves is named exporter. I've done it nevertheless.

sjkaris · 2019-10-07T18:12:29Z

sdk/export/exporter.go

+//
+// The SpanData should not be modified.
+type SpanSyncer interface {
+	ExportSpan(context.Context, *SpanData)


This is probably a bigger questions, but how do we plan on propagating errors back from these interfaces?

An exporter getting an error shouldn't impact other exporters in the stack. So I believe they should log their errors, but there's no point in them sending it upstream.

Exporter should allow registering optional callback function OnError()

That looks like it's outside the scope of this PR though? I've opened #174.

Agree, that's out of scope. Thanks for #174.

jmacd · 2019-10-08T17:01:38Z

sdk/export/exporter.go

+//
+// The SpanData should not be modified.
+type SpanSyncer interface {
+	ExportSpan(context.Context, *SpanData)


Agree, that's out of scope. Thanks for #174.

rghetia · 2019-10-08T17:21:23Z

@dmathieu are you going to resolve this comment in this PR or in a separate PR?

So it's explicit they are for spans, and we reduce the risk of name conflict

…aws (#162) * Bump github.com/aws/aws-sdk-go from 1.33.14 to 1.33.15 in /detectors/aws Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.33.14 to 1.33.15. - [Release notes](https://github.com/aws/aws-sdk-go/releases) - [Changelog](https://github.com/aws/aws-sdk-go/blob/master/CHANGELOG.md) - [Commits](aws/aws-sdk-go@v1.33.14...v1.33.15) Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

dmathieu requested review from bogdandrutu, jmacd, pjanotti, rghetia, sjkaris and tedsuo as code owners October 3, 2019 14:59

dmathieu commented Oct 3, 2019

View reviewed changes

rghetia reviewed Oct 3, 2019

View reviewed changes

freeformz suggested changes Oct 3, 2019

View reviewed changes

dmathieu requested a review from rghetia October 4, 2019 08:31

rghetia reviewed Oct 4, 2019

View reviewed changes

akvanhar reviewed Oct 4, 2019

View reviewed changes

jmacd reviewed Oct 4, 2019

View reviewed changes

rghetia mentioned this pull request Oct 4, 2019

Add Stackdriver Trace exporter for trace. #160

Merged

rghetia approved these changes Oct 7, 2019

View reviewed changes

sjkaris reviewed Oct 7, 2019

View reviewed changes

dmathieu mentioned this pull request Oct 8, 2019

SDK should support an error callback #174

Closed

jmacd approved these changes Oct 8, 2019

View reviewed changes

dmathieu added 9 commits October 8, 2019 19:39

setup sdk exporter package

a2f608b

use sdk exporter package in sdk trace

8b0b3a6

use sdk exporter package in all exporters

490cbf9

empty the exporters list before testing Load

3168dc6

move SpanData to the exporter package

6e0359f

use the SpanProcessor registration, don't register exporters

3e7b11e

rename exporter structs to avoid stutter

2cefe51

rename Syncer and Batcher to SpanSyncer and SpanBatcher

8ea87b8

So it's explicit they are for spans, and we reduce the risk of name conflict

remove not moot todo

902d20f

dmathieu added 2 commits October 8, 2019 19:40

rename sdk exporter to export

80ab23a

only execute the SpanData if it is sampled

81c0a64

rghetia merged commit c2d5c66 into open-telemetry:master Oct 8, 2019

rghetia mentioned this pull request Oct 8, 2019

SDK: Create separate package for exporter interface. #77

Closed

dmathieu deleted the export-package branch October 15, 2019 10:07

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export package #162

Export package #162

dmathieu commented Oct 3, 2019

dmathieu Oct 3, 2019

rghetia Oct 3, 2019

dmathieu Oct 3, 2019

dmathieu Oct 3, 2019

rghetia Oct 3, 2019

dmathieu Oct 3, 2019

rghetia Oct 3, 2019

rghetia Oct 3, 2019

rghetia Oct 3, 2019

freeformz Oct 3, 2019

freeformz Oct 3, 2019

freeformz Oct 3, 2019

freeformz Oct 3, 2019

rghetia Oct 4, 2019

rghetia Oct 4, 2019

rghetia Oct 4, 2019

dmathieu Oct 8, 2019

rghetia Oct 8, 2019

dmathieu Oct 8, 2019

akvanhar Oct 4, 2019

dmathieu Oct 7, 2019

jmacd Oct 4, 2019 •

edited

Loading

dmathieu Oct 7, 2019

rghetia Oct 7, 2019

dmathieu Oct 7, 2019

sjkaris Oct 7, 2019

dmathieu Oct 7, 2019

rghetia Oct 7, 2019

dmathieu Oct 8, 2019

jmacd Oct 8, 2019

jmacd Oct 8, 2019

rghetia commented Oct 8, 2019

Export package #162

Export package #162

Conversation

dmathieu commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmacd Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rghetia commented Oct 8, 2019

jmacd Oct 4, 2019 •

edited

Loading