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

Tracing configuration API #267

Merged
merged 8 commits into from
Oct 12, 2019

Conversation

lmolkova
Copy link

@lmolkova lmolkova commented Oct 9, 2019

This has started in #261.

  • TraceFactory provides builder-like APIs to configure tracing. TracerFactory is OTel concept very similar to LoggerFactory - allows to get tracer for a category.
  • you could use it with DI or without
  • It also allows configuring the default factory (for libs integration).

Fixes #206, #93

Before

var exporter = new ZipkinTraceExporter(
  new ZipkinTraceExporterOptions() {
    Endpoint = new Uri("https://<zipkin-server:9411>/api/v2/spans"),
    ServiceName = typeof(Program).Assembly.GetName().Name,
  });
var tracer = new TracerFactory(new BatchingSpanProcessor(exporter), TraceConfig.Default)
    .GetTracer("zipkin-test");
// ...

await exporter.ShutdownAsync(CancellationToken.None);

After

using (var tracerFactory = TracerFactory.Create(
    builder => builder.UseZipkin(o =>
        o.ServiceName = "my-service";
        o.Endpoint = new Uri("https://<zipkin-server:9411>/api/v2/spans"))))
{
    var tracer = tracerFactory.GetTracer("zipkin-test");

    // ...
}

@JamesNK
Copy link
Contributor

JamesNK commented Oct 10, 2019

Looks good! Lots of public method parameters should have null checks but that's picking nits 😄

Copy link
Member

@austinlparker austinlparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the null checks, I think it might be useful to have an explicit setter for the tracer name, as it's not entirely clear how that gets set right now or what you should use? Overall I think it's an improvement though.

README.md Outdated Show resolved Hide resolved
o.ServiceName = "my-service";
o.Endpoint = new Uri("https://<zipkin-server:9411>/api/v2/spans"))))
{
var tracer = tracerFactory.GetTracer("zipkin-test");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the tracer name coming from here?

using (var tracerFactory = TracerFactory.Create(
builder => builder.SetExporter(new StackdriverTraceExporter("YOUR-GOOGLE-PROJECT-ID"))))
{
var tracer = tracerFactory.GetTracer("stackdriver-test");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, where is tracer named in example?

using (var tracerFactory = TracerFactory.Create(builder => builder
.UseApplicationInsights(config => config.InstrumentationKey = "instrumentation-key")))
{
var tracer = tracerFactory.GetTracer("application-insights-test");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracer name?

@lmolkova
Copy link
Author

lmolkova commented Oct 11, 2019

I think it might be useful to have an explicit setter for the tracer name, as it's not entirely clear how that gets set right now or what you should use?

@austinlparker can you elaborate?
The tracer name is part of the named-tracers spec and it's like logging category, so you always request named tracer and factory creates (reuses) one for you.

New Tracer instances can be created via a TracerFactory and its getTracer
method. This method expects two string arguments:

  • name (required): This name must identify the instrumentation library (also
    referred to as integration, e.g. io.opentelemetry.contrib.mongodb) and not
    the instrumented library.
    In case an invalid name (null or empty string) is specified, a working
    default Tracer implementation as a fallback is returned rather than returning
    null or throwing an exception.
    A library, implementing the OpenTelemetry API may also ignore this name and
    return a default instance for all calls, if it does not support "named"
    functionality (e.g. an implementation which is not even observability-related).
    A TracerFactory could also return a no-op Tracer here if application owners configure
    the SDK to suppress telemetry produced by this library.
  • version (optional): Specifies the version of the instrumentation library
    (e.g. semver:1.0.0).

So, could you help me understand if you want to improve the documentation or mean some API change?

@lmolkova
Copy link
Author

Thanks everyone for the review! and special thanks to @JamesNK and @pakrym

Merging this, if anyone has more feedback, I'll address it in another PR.

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.

Construction by dependency injection / excessive reliance on static singletons
4 participants