Added create trace pipeline method (Breaking Change: removes implementation of "SpanBatcher") #56
Conversation
exporter/trace/cloudtrace.go
Outdated
| @@ -229,6 +253,44 @@ type Exporter struct { | |||
| traceExporter *traceExporter | |||
| } | |||
|
|
|||
| // NewExportPipeline sets up a complete export pipeline with the recommended setup | |||
| // for trace provider. Returns provider, flush function, and errors. | |||
| func NewExportPipeline(opts ...Option) (apitrace.Provider, func(), error) { | |||
There was a problem hiding this comment.
This doesn't include an option to set the resource of the trace provider as per WithResource: https://github.com/open-telemetry/opentelemetry-go/blob/master/sdk/trace/provider.go#L211
In general, I think you followed the Jaeger example, but it might be better to look at how a similar function is implemented in the operations metric exporter, see: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/master/exporter/metric/metric.go#L115
It allows you to pass in sdk push.Options and applies them directly. You could do something similar here, but for traces, meaning you provide a way for users to specify all current and future sdktrace options (you wouldn't need to add SDKConfig, Resource, etc).
i.e. something like:
func NewExportPipeline(opts []Option, topts ...sdktrace.Option) (*apitrace.Provider, func(), error) {
...
tp, err := sdktrace.NewProvider(
append([]sdktrace.Option{
sdktrace.WithSyncer(&Exporter{traceExporter: te},
}, topts...)...,
)It would also be nice to separate the registration of the trace provider in an InstallNewPipeline function similar to what's done in the metric exporter.
…InstallNewPipeline method
|
I keep running into this issue with the timeout test. It passes on my machine when I run |
example/trace/http/client/client.go
Outdated
| texporter.WithProjectID(projectID), | ||
| _, flush, err := texporter.InstallNewPipeline( | ||
| []texporter.Option {texporter.WithProjectID(projectID)}, | ||
| // For the demonstration, use sdktrace.AlwaysSample sampler to sample all traces. |
There was a problem hiding this comment.
Should it be
| // For the demonstration, use sdktrace.AlwaysSample sampler to sample all traces. | |
| // For this example code we use sdktrace.AlwaysSample sampler to sample all traces. |
example/trace/http/server/server.go
Outdated
| cloudtrace.WithProjectID(projectID), | ||
| _, flush, err := cloudtrace.InstallNewPipeline( | ||
| []cloudtrace.Option {cloudtrace.WithProjectID(projectID)}, | ||
| // For the demonstration, use sdktrace.AlwaysSample sampler to sample all traces. |
There was a problem hiding this comment.
| // For the demonstration, use sdktrace.AlwaysSample sampler to sample all traces. | |
| // For this example code we use sdktrace.AlwaysSample sampler to sample all traces. |
exporter/trace/README.md
Outdated
| texporter.WithProjectID(projectID), | ||
| // other optional exporter options | ||
| }, | ||
| // For the demonstration, use sdktrace.AlwaysSample sampler to sample all traces. |
There was a problem hiding this comment.
| // For the demonstration, use sdktrace.AlwaysSample sampler to sample all traces. | |
| // This example code uses sdktrace.AlwaysSample sampler to sample all traces. |
That's weird. I just reran the build and it worked 😕 |
james-bebbington
changed the title
james-bebbington
changed the title
Pipeline method to create exporter and provider in one method. Also updated tests, example, and readme to use pipeline.
This PR also removes the
ExportSpansmethod, so the provider can no longer use thewithBatcheroption when creating the exporter.(NOTE: THIS IS A BREAKING CHANGE)