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 jaeger options to functional style #161

Merged
merged 4 commits into from
Oct 4, 2019
Merged

change jaeger options to functional style #161

merged 4 commits into from
Oct 4, 2019

Conversation

paivagustavo
Copy link
Member

Fixes #146

With this i have separated the Options struct in 3:

  1. General Options that apply to any jaeger exporter;
  2. Collector Endpoint Options;
  3. Agent Endpoint Options

Every exporter must pass one, and only one, endpoint (through the EndpointOption). The new signature is:

func NewExporter(endpointOption EndpointOption, opts ...Option) (*Exporter, error) {

Usage examples:

	exporter, err := jaeger.NewExporter(
		jaeger.WithCollectorEndpoint("http://localhost:14268/api/traces"),
		jaeger.WithProcess(jaeger.Process{
			ServiceName: "trace-demo",
		}),
	)

	exporter, err = jaeger.NewExporter(
		jaeger.WithCollectorEndpoint(
			"http://localhost:14268/api/traces",
			jaeger.WithUsername("user"), // this only exists inside "WithCollectorEndpoint" option
			jaeger.WithPassword("password")), // this only exists inside "WithCollectorEndpoint" option
		jaeger.WithOnError(func(err error) {
			log.Fatal(err.Error())
		}),
	)

	exporter, err = jaeger.NewExporter(
		jaeger.WithAgentEndpoint("localhost:6831"),
		jaeger.WithBufferMaxCount(100),
	)

I have also extracted the upload to an interface:

// batchUploader send a batch of spans to Jaeger
type batchUploader interface {
	upload(batch *gen.Batch) error
}

The EndpointOption generates an implementation of batchUploader based on the option passed by the user.

exporter/trace/jaeger/uploader.go Show resolved Hide resolved
exporter/trace/jaeger/uploader.go Show resolved Hide resolved
exporter/trace/jaeger/jaeger.go Outdated Show resolved Hide resolved
@paivagustavo
Copy link
Member Author

@rghetia I've changed what you request, could you review it again?

@paivagustavo paivagustavo requested a review from rghetia October 3, 2019 21:01
ymotongpoo added a commit to ymotongpoo/opentelemetry-go that referenced this pull request Oct 4, 2019
passing option struct directly.
This fix is aliging the same fix for Jaeger (#146, open-telemetry#161)

* Change Option struct to be function type

* Change the original Option struct to be private
ymotongpoo added a commit to ymotongpoo/opentelemetry-go that referenced this pull request Oct 4, 2019
passing option struct directly.
This fix is aliging the same fix for Jaeger (#146, open-telemetry#161)

* Change Option struct to be function type

* Change the original Option struct to be private
@rghetia rghetia merged commit b1bb19a into open-telemetry:master Oct 4, 2019
ymotongpoo added a commit to ymotongpoo/opentelemetry-go that referenced this pull request Oct 8, 2019
passing option struct directly.
This fix is aliging the same fix for Jaeger (#146, open-telemetry#161)

* Change Option struct to be function type

* Change the original Option struct to be private
ymotongpoo added a commit to ymotongpoo/opentelemetry-go that referenced this pull request Oct 8, 2019
passing option struct directly.
This fix is aliging the same fix for Jaeger (#146, open-telemetry#161)

* Change Option struct to be function type

* Change the original Option struct to be private
rghetia pushed a commit that referenced this pull request Oct 9, 2019
* Add Stackdriver Trace exporter for trace.

TODOs for future work is:

* to replace bundler.Bundler

* to add proper tests for the exporter

* to move the exporter to proper repository once it will be created.

* Change to use functions for the exporter initialization instead of
passing option struct directly.
This fix is aliging the same fix for Jaeger (#146, #161)

* Change Option struct to be function type

* Change the original Option struct to be private

* Add line comments to maxMessageEventsPerSpan to leave it for future implementation

* Fix unnessesary expressions specified by `make precommit`

Left errors by `make precommit` in experimental/bridge/opentracing.

* Ran make precommit

* Add new line at EOF

* WIP: Start implementing BatchSpanExporter interfaces

* Change to use RegisterSpanProcessor to register bsp

* Change function names to fit current implementation of sdk

* Removed google.golang.org/api/support/bundler and implement ssp and bsp

* Change spanProcessor as a member of Exporter.

* Fix option names used for BatchSpanProcessor initialization.

* Change Exporter.Shutdown just to unregister spanProcessor.

* Removed copyright statements of OpenCensus.

* Fix small typo and EOF new line

* Fix interfaces of ExportSpan/ExportSpans to meet SpanSyncer/SpanBatcher

* Change to follow context.Context passed in ExportSpan/ExportSpans

* Fix Stackdriver Exporter to hold sync.Once to lock when it is registered and
unregistered.
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jaeger exporter should use functional options
4 participants