Add bundler to exporter #39

stevencl1013 · 2020-06-08T18:25:07Z

Bundles spans together and uploads them in separate worker threads

ymotongpoo · 2020-06-08T22:36:01Z

@stevencl1013 Please run make precommit

nilebox

Overall looks good, but a few bugs and nits. See review comments.

nilebox · 2020-06-09T02:44:12Z

exporter/trace/trace.go

+	if o.BundleDelayThreshold > 0 {
+		b.DelayThreshold = o.BundleDelayThreshold
+	} else {
+		b.DelayThreshold = 2 * time.Second


Could you extract constants to the top of the file, and give them descriptive names, please?

nilebox · 2020-06-09T02:49:19Z

exporter/trace/trace.go

+		b.BundleCountThreshold = 50
+	}
+	if o.NumberOfWorkers > 0 {
+		b.HandlerLimit = o.NumberOfWorkers


Based on the description https://github.com/googleapis/google-api-go-client/blob/cfc62336a21b9af45e2830a9898569d8aac1c5cc/support/bundler/bundler.go#L80-L82, the config parameter should be called MaxNumberOfWorkers rather than just NumberOfWorkers, or Just BundleHandlerLimit to directly refer to the bundler parameter.

nilebox · 2020-06-09T02:53:44Z

exporter/trace/trace.go

+		b.HandlerLimit = o.NumberOfWorkers
+	}
+	// The measured "bytes" are not really bytes, see exportReceiver.
+	b.BundleByteThreshold = b.BundleCountThreshold * 200


This should be done right after setting BundleCountThreshold, to make sure they are set together.
Also, there is no exportReceiver in this repo?

nilebox · 2020-06-09T02:53:59Z

exporter/trace/trace.go

+	}
+	// The measured "bytes" are not really bytes, see exportReceiver.
+	b.BundleByteThreshold = b.BundleCountThreshold * 200
+	b.BundleByteLimit = b.BundleCountThreshold * 1000


Move next to setting BundleCountThreshold

exporter/trace/trace.go

nilebox · 2020-06-10T03:19:41Z

Looks like some changes aren't committed, please run make precommit and push missing changes.

ymotongpoo · 2020-06-11T07:35:06Z

exporter/trace/trace.go

@@ -29,11 +34,19 @@ import (
 type traceExporter struct {
 	o         *options
 	projectID string
+	bundler   *bundler.Bundler


@rghetia As far as I remember, there's some issues on using bundler and that's the reason why I avoided using bundler and used BatchWriteSpans in the first implementation. Could you share the background for the record?

@ymotongpoo I kind of remember some issue with bundler but I don't remember the details.

For record: at least the report in OC pointed the possibility of memory leak.
census-ecosystem/opencensus-go-exporter-ocagent#71

From that issue, looks like the memory leak issue only appears when the backend is not reachable. Should be relatively easy to reproduce the issue?

@ymotongpoo I don't really understand that issue in the ocagent repo. From the issue:

as long as there is a valid connection to the collector, the “handler” function, i.e. uploadTraces, will be called on the bundle to offload the bundled traces to the collector.

Isn't the handler function called regardless of whether there is a connection? That connection check in the issue is already inside the uploadTraces function. And in this exporter, there is no such check for a connection, the upload function just calls BatchWriteSpans.

Also, since the bundler's BufferedByteLimit is set here, the bundler will not keep more than that many bytes in memory.

rghetia · 2020-06-16T04:14:49Z

exporter/trace/cloudtrace.go

 	// TraceSpansBufferMaxBytes is the maximum size (in bytes) of spans that
 	// will be buffered in memory before being dropped.
 	//
 	// If unset, a default of 8MB will be used.
-	// TraceSpansBufferMaxBytes int
+	TraceSpansBufferMaxBytes int


This should be just BufferMaxBytes similar to other option names (which are without TraceSpans).
This could potentially be refactored and shared with metrics.

rghetia · 2020-06-16T04:15:52Z

exporter/trace/cloudtrace.go

+	// BundleDelayThreshold determines the max amount of time
+	// the exporter can wait before uploading view data or trace spans to
+	// the backend.
+	// Optional.


Please specify the default value.

rghetia · 2020-06-16T04:16:06Z

exporter/trace/cloudtrace.go

+
+	// BundleCountThreshold determines how many view data events or trace spans
+	// can be buffered before batch uploading them to the backend.
+	// Optional.


rghetia · 2020-06-16T04:18:00Z

exporter/trace/cloudtrace.go

-	// for now. The minimum number of workers is 1.
-	NumberOfWorkers int
+	// MaxNumberOfWorkers sets the maximum number of go rountines that send requests
+	// to Stackdriver Trace. The minimum number of workers is 1.


Replace Stackdriver with 'Cloud Trace'. Here and on line 91.

rghetia · 2020-06-16T04:18:29Z

exporter/trace/cloudtrace.go

+	// UploadFunction is the function used to upload spans to Cloud Trace. Used for
+	// testing. Defaults to uploadSpans in trace.go.
+	// Optional.
+	UploadFunction func(ctx context.Context, spans []*tracepb.Span)


You can use mock server instead of exporting Test-only interface.

rghetia · 2020-06-16T05:10:55Z

exporter/trace/trace.go

+		case o.accum == 0:
+			o.pause = false
+		case o.accum == 1:
+			log.Println("OpenCensus Stackdriver exporter: failed to upload span: buffer full")


s/OpenCensus Stackdriver/OpenTelemetry Cloud Trace/

rghetia · 2020-06-16T05:11:11Z

exporter/trace/trace.go

+			o.accum = 0
+			o.delay()
+		default:
+			log.Printf("OpenCensus Stackdriver exporter: failed to upload %d spans: buffer full", o.accum)


same as above.

rghetia · 2020-06-16T05:11:23Z

exporter/trace/trace.go

+	o.mu.Lock()
+	defer o.mu.Unlock()
+	if !o.pause {
+		log.Println("OpenCensus Stackdriver exporter: failed to upload span: buffer full")


nilebox

LGTM, but leaving up to @ymotongpoo whether a potential memory leak is a blocking issue for merging this.

ymotongpoo · 2020-06-18T03:32:42Z

Thank you @nilebox for asking me comments. As far as I have heard OC exporter users' experiences, I seldom heard the issues that originate to memory leak. However, still the issue is known, and haven't solved yet. So my request here is for this PR to have at least either (preferably both) of the following:

Add todo or note comment in bundler field in traceExporter to touch the potential issue and existing similar issue linked in my previous comment.
Add test to reproduce the issue.

I think we need at least 1.

stevencl1013 · 2020-06-22T15:08:30Z

@ymotongpoo I don't understand how that issue relates to this exporter. I've looked into it, and I mentioned what I found in this comment: #39 (comment)

james-bebbington · 2020-06-23T00:15:39Z

Sorry to come to this one very late. Yesterday I noticed that there are two different versions of the TraceExporter in the OpenTelemetry SDK: https://github.com/open-telemetry/opentelemetry-go/blob/master/sdk/export/trace/trace.go

SpanSyncer which implements ExportSpan
SpanBatcher which implements ExportSpans

When setting up your Tracing pipeline, you can configure either of these, by choosing using WithSyncer or WithBatcher (see https://github.com/open-telemetry/opentelemetry-go/blob/master/sdk/trace/provider.go#L186). The Batching work is done in https://github.com/open-telemetry/opentelemetry-go/blob/master/sdk/trace/batch_span_processor.go. In the operations exporter, we could just have a helper function that sets up the SpanBatcher pipeline.

So, I wanted to make sure you'd taken a look at that & considered what do we gain by doing our own bundling in the exporter as opposed to just making use of the work that's already done when using the Batch Span Processor?

nilebox · 2020-06-23T00:25:38Z

Follow-up to @james-bebbington 's comment: currently we basically have 2 identical implementations of ExportSpan and ExportSpans:

opentelemetry-operations-go/exporter/trace/trace.go

Lines 51 to 64 in 8c1bbc5

    
           // ExportSpan exports a SpanData to Stackdriver Trace. 
        
           func (e *traceExporter) ExportSpan(ctx context.Context, sd *export.SpanData) { 
        
           	protoSpan := protoFromSpanData(sd, e.projectID) 
        
           	e.uploadFn(ctx, []*tracepb.Span{protoSpan}) 
        
           } 
        
           // ExportSpans exports a slice of SpanData to Stackdriver Trace in batch 
        
           func (e *traceExporter) ExportSpans(ctx context.Context, sds []*export.SpanData) { 
        
           	pbSpans := make([]*tracepb.Span, len(sds)) 
        
           	for i, sd := range sds { 
        
           		pbSpans[i] = protoFromSpanData(sd, e.projectID) 
        
           	} 
        
           	e.uploadFn(ctx, pbSpans) 
        
           }

, implementing both interfaces he mentioned above.

What if we remove the ExportSpan function (implementation of SpanSyncer), and only support SpanBatcher with ExportSpans?
That way we will force users to always register our exporter via WithBatcher, and the bundler won't be necessary.

FWIW there is already a precedent of this approach:

Zipkin exporter only implements SpanBatcher: https://github.com/open-telemetry/opentelemetry-go/blob/e5267a3aa898f14cce75cb640ee4153379965057/exporters/trace/zipkin/zipkin.go#L92
Jaeger only implements SpanSyncer: https://github.com/open-telemetry/opentelemetry-go/blob/e5267a3aa898f14cce75cb640ee4153379965057/exporters/trace/jaeger/jaeger.go#L193

stevencl1013 · 2020-06-23T19:43:25Z

Thanks for the info @james-bebbington and @nilebox, I wasn't aware of the difference between WithSyncer and WithBatcher before. I've been doing my experimentation on a simple application similar to the example in this repo, which uses the WithSyncer option.

I read in internal documentation that OpenTelemetry exporters should use both batching and async writes, so I still believe this PR implements those functionalities for this exporter when using the WithSyncer option. When I created this PR, I had two main goals:

Add the ability to export spans in parallel, and let users configure the number of worker threads (Add workers to export spans in parallel. #19).
Reduce the high latency that I had experienced in my testing, and that was also reported later by a user in Latency issues of HTTP requests with Stackdriver tracing v0.2.0 and otel v0.6.0 #47.

If we were to only support SpanBatcher and remove the bundler, then I think the 2nd goal would be achieved, but the 1st would not. Would it make sense to keep supporting SpanSyncer, either by keeping implementations of both interfaces, or by taking a similar approach to the Jaeger exporter (only implementing SpanSyncer and using a bundler)?

james-bebbington · 2020-06-24T08:00:32Z

Thanks for the info @james-bebbington and @nilebox, I wasn't aware of the difference between WithSyncer and WithBatcher before. I've been doing my experimentation on a simple application similar to the example in this repo, which uses the WithSyncer option.

No worries, we also weren't aware of it until recently. There is probably some general OpenTelemetry documentation missing with recommendations around how to go about implementing exporters.

If we were to only support SpanBatcher and remove the bundler, then I think the 2nd goal would be achieved, but the 1st would not. Would it make sense to keep supporting SpanSyncer, either by keeping implementations of both interfaces, or by taking a similar approach to the Jaeger exporter (only implementing SpanSyncer and using a bundler)?

Yea we definitely want to support batched & parallel exporting. At the same time, we want to make sure we don't make this confusing for users:

If they use WithSyncer but then we batch the data in the exporter anyway, that's somewhat confusing, although maybe that's not a big deal
If they use WithBatcher, then it will be confusing that the batch size can be configured in both the batch span processor and in the exporter via BundleCountThreshold

Note we could enforce which setup the user has to use by only implementing one of those interfaces (and optionally also creating a wrapper initialization function to set up the pipeline for them).

I'm not really sure what the right thing is to do here though. Any thoughts @rghetia @ymotongpoo @nilebox?

(overall this PR is probably okay as is but I just want to make sure we've thought this through)

Also note even the Go SDK is not consistent:

The Zipkin exporter only implements the SpanBatcher interface
The Jaeger exporter only implements the SpanSyncer interface, and then implements batching internally

As another point of reference, the Python SDK only has a single Export function that takes a list of spans. Both the Simple (Syncer) & Batch span processors use this function (the simpler processor will just send a single span at a time).

james-bebbington · 2020-06-24T08:22:35Z

exporter/trace/cloudtrace_test.go

@@ -127,14 +134,19 @@ func TestExporter_Timeout(t *testing.T) {
 	mockTrace.spansUploaded = nil
 	mockTrace.delay = 20 * time.Millisecond
 	var exportErrors []error
+	var wg sync.WaitGroup
+	wg.Add(1)


Would be more idiomatic to use a channel for this I think

nilebox · 2020-06-24T08:32:01Z

If we were to only support SpanBatcher and remove the bundler, then I think the 2nd goal would be achieved, but the 1st would not.

@stevencl1013 Can we try addressing this issue in the SDK batcher then instead? That would benefit all users of Go SDK rather than just Stackdriver.

I'm not sure what to suggest that would allow using bundler without making it confusing tbh:

It seems that Syncer is designed for use when user wants to send ("sync") spans straight to the backend without buffering, so bundler would go against that design.
Similarly, cascading batchers (SDK batcher + bundler) might be confusing as well.

ymotongpoo · 2020-06-24T13:50:53Z

@stevencl1013 Sorry for delayed response.

Here's the reason why I mentioned that issue in OC exporter:

Having connectivity check should be TODO, thus the implementation should be similar to OC exporter.
Though BufferedByteLimit exists, that code is copied from OC exporter, where the issue exists.

Anyway, as @nilebox and @james-bebbington suggested, now we have SpanBatcher in the SDK and it look better to use it for batching to avoid additional complexity.

ymotongpoo · 2020-06-25T00:48:32Z

@stevencl1013 I had a offline discussion with @rghetia about the use of bundler. In short, as you confirmed first, we don't check the connectivity and won't in this exporter, so it's safe to use bundler here.

Background:
Originally I thought we need to have the connectivity check later on like OC exporter, which may pile up the unsent spans in the memory. However, the spec for error handling in the upstream doesn't mention such behavior, and how to handle these unsent spans are up to exporter implementations. We should just throw the unbuffered spans in this exporter and let the collector to handle such complex cases.

reazahmed · 2020-06-25T14:19:27Z

Apologies for jumping in in the middle of the discussion.

As per OTel specification (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#built-in-span-processors):

The OTel standard SDK MUST implement the simple processor (i.e., sync case). With current simple process, a user will experience very high delay for each RPC. This change handles the simple processor case, by brining down latency from ~10sec to ~200 milliseconds.

james-bebbington · 2020-06-29T00:21:41Z

For reference, given the way the SDK is set up, we decided there isn't any ideal solution, but for now decided to:

Merge this PR as is
Create a follow up PR, that includes a breaking change, to:
- Remove the ExportTraces function, and only keep ExportTrace function
- Add a NewExportPipeline function that we expect consumers to use rather than setting up a Syncer themselves

i.e. we will implement this in the same way as the Jaeger exporter (see the Otel Go SDK).

FYI @nilebox I know you had some reservations around the explicitness of the word Syncer but we decided this is the least bad of two not great solutions for now: duplicate batching options is likely to add more confusion and create more difficulties with regards to making backwards compatible changes in the future.

@stevencl1013 - in the next PR it'd also be good to add/update the example code in this repo

stevencl1013 added 3 commits June 1, 2020 16:44

use bundler to export spans

82ce91d

add method to set NumberOfWorkers in trace exporter

f8231e9

upload spans with same context in batch

6a50703

nilebox suggested changes Jun 9, 2020

View reviewed changes

stevencl1013 added 2 commits June 9, 2020 19:22

update tests to handle bundles before checking results

a6cfd24

handle OversizedItem error and fix nits

decac37

update trace example's go.sum

5955ba2

ymotongpoo reviewed Jun 11, 2020

View reviewed changes

ymotongpoo requested a review from rghetia June 11, 2020 07:35

ymotongpoo added the trace label Jun 11, 2020

stevencl1013 added 5 commits June 11, 2020 18:48

allow changes to uploadFn for testing

5e2cf5f

add test for bundling

941ab9c

set trace client options in bundling test

77b93ff

increase byte threshold to pass concurrency test

05ad88c

add concurrency test

bd2f2b3

rghetia reviewed Jun 16, 2020

View reviewed changes

stevencl1013 added 3 commits June 16, 2020 18:19

small fixes/changes to print statements, variable names, and comments

3a48378

use mock server in bundling and concurrency tests

f0594dd

remove ability to change uploadfn

9aef481

stevencl1013 requested review from rghetia and nilebox June 16, 2020 20:23

nilebox approved these changes Jun 18, 2020

View reviewed changes

stevencl1013 mentioned this pull request Jun 18, 2020

Latency issues of HTTP requests with Stackdriver tracing v0.2.0 and otel v0.6.0 #47

Closed

reset mockTrace server fields in bundling and concurrency tests

feaffbb

james-bebbington reviewed Jun 24, 2020

View reviewed changes

use channel instead of waitgroup in timeout test

d023626

james-bebbington merged commit 4596d11 into GoogleCloudPlatform:master Jun 29, 2020

nilebox mentioned this pull request Jul 27, 2020

Move stackdriver trace exporter to new interface and pdata open-telemetry/opentelemetry-collector-contrib#486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bundler to exporter #39

Add bundler to exporter #39

stevencl1013 commented Jun 8, 2020

ymotongpoo commented Jun 8, 2020

nilebox left a comment

nilebox Jun 9, 2020

nilebox Jun 9, 2020

nilebox Jun 9, 2020

nilebox Jun 9, 2020

nilebox commented Jun 10, 2020

ymotongpoo Jun 11, 2020

rghetia Jun 15, 2020

ymotongpoo Jun 16, 2020

nilebox Jun 16, 2020

stevencl1013 Jun 18, 2020 •

edited

rghetia Jun 16, 2020

rghetia Jun 16, 2020

rghetia Jun 16, 2020

rghetia Jun 16, 2020

rghetia Jun 16, 2020

rghetia Jun 16, 2020

rghetia Jun 16, 2020

rghetia Jun 16, 2020

nilebox left a comment

ymotongpoo commented Jun 18, 2020

stevencl1013 commented Jun 22, 2020

james-bebbington commented Jun 23, 2020

nilebox commented Jun 23, 2020 •

edited

stevencl1013 commented Jun 23, 2020

james-bebbington commented Jun 24, 2020 •

edited

james-bebbington Jun 24, 2020

nilebox commented Jun 24, 2020

ymotongpoo commented Jun 24, 2020

ymotongpoo commented Jun 25, 2020

reazahmed commented Jun 25, 2020

james-bebbington commented Jun 29, 2020

Add bundler to exporter #39

Add bundler to exporter #39

Conversation

stevencl1013 commented Jun 8, 2020

ymotongpoo commented Jun 8, 2020

nilebox left a 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

nilebox commented Jun 10, 2020

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

stevencl1013 Jun 18, 2020 • edited

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

nilebox left a comment

Choose a reason for hiding this comment

ymotongpoo commented Jun 18, 2020

stevencl1013 commented Jun 22, 2020

james-bebbington commented Jun 23, 2020

nilebox commented Jun 23, 2020 • edited

stevencl1013 commented Jun 23, 2020

james-bebbington commented Jun 24, 2020 • edited

Choose a reason for hiding this comment

nilebox commented Jun 24, 2020

ymotongpoo commented Jun 24, 2020

ymotongpoo commented Jun 25, 2020

reazahmed commented Jun 25, 2020

james-bebbington commented Jun 29, 2020

stevencl1013 Jun 18, 2020 •

edited

nilebox commented Jun 23, 2020 •

edited

james-bebbington commented Jun 24, 2020 •

edited