-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[tracing] add simplified export pipeline setup for Jaeger #459
[tracing] add simplified export pipeline setup for Jaeger #459
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have tests on InstallNewPipeline/NewExportPipeline?
Sure thing, should not be a big deal, I'll provide tests as well |
example/jaeger/main.go
Outdated
// For demoing purposes, use trace.AlwaysSample to sample every trace. | ||
// In a production application, you should configure this to a trace.ProbabilitySampler | ||
// set at the desired probability. | ||
tp.ApplyConfig(sdktrace.Config{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to see the sdktrace.Config
argument passed directly to InstallNewPipeline
. Any reason not to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess it depends on what is expected of NewExportPipeline
/ InstallNewPipeline
- as this is supposed to be a simplified setup and there are default values which will be used if no config is passed to the trace provider, it is capable of returning pipeline with default configuration without needing this extra parameter.
On the other hand, I guess default does not necessarily correspond to recommended (and universally recommended config might not exist in this case), which is probably what the simplified pipeline setup should actually do for the user, so providing config would be reasonable as well. I was considering both implementations, so I'm not opposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're already passing in other Jaeger properties to InstallNewPipeline, just not sdktrace.Config... so I'm on the fence here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing functional options with an option struct seems a little weird.
I would love to see an unified functional options for exporters that could configure the underlying SDK as well but I don't know if it is feasible without some adaptations.
Something along the lines should be possible and I think it is better than passing the Options struct directly:
// Create and install Jaeger export pipeline
tp, flush, err := jaeger.InstallNewPipeline(
jaeger.WithCollectorEndpoint("http://localhost:14268/api/traces"),
jaeger.WithProcess(jaeger.Process{
ServiceName: "trace-demo",
Tags: []core.KeyValue{
key.String("exporter", "jaeger"),
key.Float64("float", 312.23),
},
}),
jaeger.WithSDK(
sdktrace.AlwaysSample(),
sdktrace.WithDefaultLabelEncoder()
),
)
@matej-g would you have time to address the comments? |
415164f
to
5f3c381
Compare
@jmacd , @paivagustavo PTAL, I have updated using consistent With* option as per the review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this. I still think we need to standardize more the initialization of all exporters.
exporters/trace/jaeger/jaeger.go
Outdated
|
||
// Registration is set to true if the trace provider of the new pipeline should be | ||
// registered as Global Trace Provider | ||
Registeration bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo here. Also, I would suggest to change this to RegisterTrace
or RegisterGlobal
, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either one seems fine. I will pick RegisterGlobal.
I do agree that exporter initialization should be standardized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to @paivagustavo , I'm good with this getting merged, but think we need to standardize initialization. I created #536 to track that work.
…etry#459) * add simplified export pipeline setup for Jaeger * add With* options to configure SDK options. * add test for WithRegistration and WithSDK * rename Registeration with RegisterGlobal * rename WithRegistration to RegisterAsGlobal Co-authored-by: rahulpa <rahulpa@google.com> Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
This PR introduces a simplified exporter for Jaeger, similar to recently introduced export pipeline setups (#395) for metrics.
Resolves #424.