Skip to content

Add OpenTelemetry.Hosting#275

Merged
lmolkova merged 4 commits intoopen-telemetry:masterfrom
JamesNK:jamesnk/hosting-v2
Oct 18, 2019
Merged

Add OpenTelemetry.Hosting#275
lmolkova merged 4 commits intoopen-telemetry:masterfrom
JamesNK:jamesnk/hosting-v2

Conversation

@JamesNK
Copy link
Copy Markdown
Contributor

@JamesNK JamesNK commented Oct 12, 2019

Replaces #253. Now uses the new factory API.

OpenTelemetry.Hosting now does a lot less. Its purpose is registering TracerFactoryBase instance with DI, and starting it with the host.

services.AddOpenTelemetry(builder =>
{
    builder
        .SetSampler(Samplers.AlwaysSample)
        .UseZipkin(o => o.ServiceName = "my-service")

        // you may also configure request and dependencies collectors
        .AddRequestCollector()
        .AddDependencyCollector())
});

TelemetryFactoryHostedService creates the factory when the host is started. If the factory and its collectors can be created immediately in ConfigureService then TelemetryFactoryHostedService could be removed.

services.AddLoggingTracer();
services.AddOpenTelemetry(() =>
{
var tracerFactory = new LoggingTracerFactory();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Usually you would use the builder overload. Because this sample completely re-implements the factory, it and the collectors have to be created manually.

@lmolkova
Copy link
Copy Markdown
Member

Looks awesome!

@lmolkova
Copy link
Copy Markdown
Member

Let's keep it open till Monday so everyone has a chance to look

@lmolkova
Copy link
Copy Markdown
Member

@JamesNK if you don't mind, could you please resolve some merge conflicts? We also moved usings outside of namespace and new files might need to be changed too.

Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

LGTM. One nit is that unfortunately the overloads with Func<IServiceProvider, TraceFactoryBase> are not covered by tests

@JamesNK
Copy link
Copy Markdown
Contributor Author

JamesNK commented Oct 15, 2019

I'm at NDC Sydney this week. I should find some time to look at it, I just don't know when.

@lmolkova
Copy link
Copy Markdown
Member

@JamesNK no problem, take your time

@SergeyKanzhelev
Copy link
Copy Markdown
Member

@JamesNK do you mind if I'd merge for you?

@JamesNK
Copy link
Copy Markdown
Contributor Author

JamesNK commented Oct 17, 2019

Be my guest.

I was planning to add xml docs to the new APIs, but that can be in a later PR.

@JamesNK
Copy link
Copy Markdown
Contributor Author

JamesNK commented Oct 18, 2019

Merge pls 🙏

@lmolkova lmolkova merged commit e7bd851 into open-telemetry:master Oct 18, 2019
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.

4 participants