Multiple processors on tracer: configuration

[lmolkova]

Fixes #226: tracers should allow multiple processors to be registered on them.
Processors are executed sequentially.

At the same time it should be possible to chain processors (e.g. filter or enrich + export).

This PR attempts to fix both.

In the example

• we add pipeline that exports each span to Zipkin,
• another pipeline that exports to AppInsights, but filters some spans out first
• adds debugging processor that just writes all spans to debug output

all pipelines work together on each span.

using (var tracerFactory = TracerFactory.Create(builder => builder
    .UseZipkin(o =>
    {
        o.ServiceName = "test-zipkin";
        o.Endpoint = new Uri(zipkinUri);
    })
    .UseApplicationInsights(
        o => o.InstrumentationKey = "e0fbfdfa-e302-49c8-9310-c3919b163c00", 
        p => p.AddProcessor(next => new FilteringSpanProcessor(next)))
    .UseDebuggingProcessor()))
{
}

Assuming there are Use extensions it looks not that bad.
But advanced use case (without Use sugar) looks a bit scary

 using (var tracerFactory = TracerFactory.Create(builder => builder
	.AddProcessorPipeline(p => p
		.AddProcessor(next => new FilteringSpanProcessor(next))
		.SetExporter(new ZipkinTraceExporter(new ZipkinTraceExporterOptions
		{
			ServiceName = "test",
		})))))
{
}

[lmolkova]

@pakrym @JamesNK would be great to hear your feedback on this if you have time

[SergeyKanzhelev]

perhaps some protection from processors returning null task can be added here.

[SergeyKanzhelev]

so "WhenAll" will not crash

[lmolkova]

still, protecting from anything in multi-processor makes no sense (it's optional and only present when advanced config with multiple processors is used), if some processor is misbehaving - we should not hide it here. may be catch in span.End()

[lmolkova]

i did exception handling in start/stop, but not her. We don't await WhenAny here and chances are it throws OperationCancelledException, so users will have to try/catch it anyway.

[SergeyKanzhelev]

I think this design is great for filtering and enriching telemetry.

[pcwiese]

Is this going to be the recommended approach for Span filtering? e.g. if I don't want to export any spans for a health check endpoint would people put that logic in a filtering processor?

[lmolkova]

Is this going to be the recommended approach for Span filtering? e.g. if I don't want to export any spans for a health check endpoint would people put that logic in a filtering processor?

Nope.

But this is the only one available today. There are discussions: open-telemetry/opentelemetry-specification#300 and #251.

Basically

• you filter by processor OR
• you may give noop tracer to certain libraries, but this is not configurable today. OR
• there are future hooks for incoming and outgoing requests filtering. They will happen extremely early so if you can filter there, the instrumentation will not even start (and this is .NET for certain libraries only):

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Collector.Dependencies/HttpClientCollectorOptions.cs#L42

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Collector.AspNetCore/AspNetCoreCollectorOptions.cs#L32

[SergeyKanzhelev]

@lmolkova is the "draft" in title still means draft? Or we can merge it in?

[lmolkova]

@SergeyKanzhelev still draft, need docs, tests, some clean up. I will probably finish it on the next week.

[lmolkova]

Not draft anymore. Will keep it open for a few days to get more feedback.

[lmolkova]

merging now. Feel free to add feedback here or file an issue if you are not happy with it.

[pcwiese]

@lmolkova
Whats the current status for filtering inbound requests using:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Collector.AspNetCore/AspNetCoreCollectorOptions.cs#L32

I'd like to filter some things by specifying my own predicate here but I see this is still only internally available. Is there a reason for that?

[lmolkova]

@pcwiese

I'd like to filter some things by specifying my own predicate here but I see this is still only internally available. Is there a reason for that?

The reason is filtering API is not very user friendly and very likely to change. Since this is rather advanced configuration and not the first priority (P1 is shipping OTel core packages in beta and GA), I decided to keep it internal until we have any cycles to understand requirements for this API and make it usable. Also, it seems there is no 'early' filtering concept available in OTel.

If you need it, I believe there are three ways to proceed:

1. implement filtering with SpanProcessor. No changes to OTel required, but perf may be impacted.
2. implement filtering with Sampler that filters based on attributes/span name. Should not require any OTel changes. Improves perf comparing to the 1.
3. play around with filtering internal API, design it well and make it public. Gives least possible perf impact. Since there are similar filtering API for any Activity/DiagnosticSource instrumented library, I wanted to understand if there is anything common for different libraries. Perhaps there is nothing in common and each adapter can have it's own filter (or none).