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

Removing Propagator from Options #1448

Merged
merged 7 commits into from
Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions examples/AspNet/Global.asax.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,8 @@ public class WebApiApplication : HttpApplication
protected void Application_Start()
{
var builder = Sdk.CreateTracerProviderBuilder()
.AddAspNetInstrumentation(options => options.Propagator = new B3Propagator())
.AddHttpClientInstrumentation(
httpClientOptions => httpClientOptions.Propagator = new B3Propagator(),
httpWebRequestOptions => httpWebRequestOptions.Propagator = new B3Propagator());
.AddAspNetInstrumentation()
.AddHttpClientInstrumentation();

switch (ConfigurationManager.AppSettings["UseExporter"].ToLowerInvariant())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
using System;
using System.Diagnostics;
using System.Web;
using OpenTelemetry.Context.Propagation;

namespace OpenTelemetry.Instrumentation.AspNet
{
Expand All @@ -26,12 +25,6 @@ namespace OpenTelemetry.Instrumentation.AspNet
/// </summary>
public class AspNetInstrumentationOptions
{
/// <summary>
/// Gets or sets <see cref="TextMapPropagator"/> for context propagation.
/// By default, <see cref="Propagators.DefaultTextMapPropagator" /> will be used.
/// </summary>
public TextMapPropagator Propagator { get; set; }

/// <summary>
/// Gets or sets a Filter function to filter instrumentation for requests on a per request basis.
/// The Filter gets the HttpContext, and should return a boolean.
Expand Down
5 changes: 4 additions & 1 deletion src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@
to CompositeTextMapPropagator. IPropagator is renamed to TextMapPropagator
and changed from interface to abstract class.
([#1427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1427))
* Propagators.DefaultTextMapPropagator will be used as the default Propagator
* Propagators.DefaultTextMapPropagator will be used as the default Propagator.
([#1427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1428))
* Removed Propagator from Instrumentation Options. Instrumentation now always
respect the Propagator.DefaultTextMapPropagator.
([#1448](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1448))

## 0.7.0-beta.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public override void OnStartActivity(Activity activity, object payload)

var request = context.Request;
var requestValues = request.Unvalidated;
var textMapPropagator = this.options.Propagator ?? Propagators.DefaultTextMapPropagator;
var textMapPropagator = Propagators.DefaultTextMapPropagator;

if (!(textMapPropagator is TraceContextPropagator))
{
Expand Down Expand Up @@ -140,7 +140,7 @@ public override void OnStopActivity(Activity activity, object payload)
Activity activityToEnrich = activity;
Activity createdActivity = null;

var textMapPropagator = this.options.Propagator ?? Propagators.DefaultTextMapPropagator;
var textMapPropagator = Propagators.DefaultTextMapPropagator;
bool isCustomPropagator = !(textMapPropagator is TraceContextPropagator);

if (isCustomPropagator)
Expand Down
7 changes: 1 addition & 6 deletions src/OpenTelemetry.Instrumentation.AspNet/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,7 @@ public class WebApiApplication : HttpApplication
## Advanced configuration

This instrumentation can be configured to change the default behavior by using
`AspNetInstrumentationOptions`, which allows configuring `Propagator` and
`Filter` as explained below.

### Propagator

TODO
`AspNetInstrumentationOptions`, which allows configuring `Filter` as explained below.

### Filter

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
using System;
using System.Diagnostics;
using Microsoft.AspNetCore.Http;
using OpenTelemetry.Context.Propagation;

namespace OpenTelemetry.Instrumentation.AspNetCore
{
Expand All @@ -26,12 +25,6 @@ namespace OpenTelemetry.Instrumentation.AspNetCore
/// </summary>
public class AspNetCoreInstrumentationOptions
{
/// <summary>
/// Gets or sets <see cref="TextMapPropagator"/> for context propagation.
/// By default, <see cref="Propagators.DefaultTextMapPropagator" /> will be used.
/// </summary>
public TextMapPropagator Propagator { get; set; } = Propagators.DefaultTextMapPropagator;

/// <summary>
/// Gets or sets a Filter function to filter instrumentation for requests on a per request basis.
/// The Filter gets the HttpContext, and should return a boolean.
Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
([#1427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1427))
* Propagators.DefaultTextMapPropagator will be used as the default Propagator
([#1427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1428))
* Removed Propagator from Instrumentation Options. Instrumentation now always
respect the Propagator.DefaultTextMapPropagator.
([#1448](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1448))

## 0.7.0-beta.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,10 @@ public override void OnStartActivity(Activity activity, object payload)
}

var request = context.Request;
if (!this.hostingSupportsW3C || !(this.options.Propagator is TraceContextPropagator))
var textMapPropagator = Propagators.DefaultTextMapPropagator;
if (!this.hostingSupportsW3C || !(textMapPropagator is TraceContextPropagator))
{
var ctx = this.options.Propagator.Extract(default, request, HttpRequestHeaderValuesGetter);
var ctx = textMapPropagator.Extract(default, request, HttpRequestHeaderValuesGetter);

if (ctx.ActivityContext.IsValid()
&& ctx.ActivityContext != new ActivityContext(activity.TraceId, activity.ParentSpanId, activity.ActivityTraceFlags, activity.TraceStateString, true))
Expand Down
7 changes: 1 addition & 6 deletions src/OpenTelemetry.Instrumentation.AspNetCore/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,9 @@ public void ConfigureServices(IServiceCollection services)
## Advanced configuration

This instrumentation can be configured to change the default behavior by using
`AspNetCoreInstrumentationOptions`, which allows configuring
[`Propagator`](#propagator) and adding [`Filter`](#filter),
`AspNetCoreInstrumentationOptions`, which allows adding [`Filter`](#filter),
[`Enrich`](#enrich) as explained below.

### Propagator

TODO

### Filter

This instrumentation by default collects all the incoming http requests. It
Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
([#1427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1427))
* Propagators.DefaultTextMapPropagator will be used as the default Propagator
([#1427](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1428))
* Removed Propagator from Instrumentation Options. Instrumentation now always
respect the Propagator.DefaultTextMapPropagator.
([#1448](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1448))

## 0.7.0-beta.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
using System.Diagnostics;
using System.Net.Http;
using System.Runtime.CompilerServices;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Instrumentation.Http.Implementation;
using OpenTelemetry.Trace;

Expand All @@ -34,12 +33,6 @@ public class HttpClientInstrumentationOptions
/// </summary>
public bool SetHttpFlavor { get; set; }

/// <summary>
/// Gets or sets <see cref="TextMapPropagator"/> for context propagation.
/// By default, <see cref="Propagators.DefaultTextMapPropagator" /> will be used.
/// </summary>
public TextMapPropagator Propagator { get; set; } = Propagators.DefaultTextMapPropagator;

/// <summary>
/// Gets or sets a Filter function to filter instrumentation for requests on a per request basis.
/// The Filter gets the HttpRequestMessage, and should return a boolean.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
using System;
using System.Diagnostics;
using System.Net;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Instrumentation.Http.Implementation;
using OpenTelemetry.Trace;

Expand All @@ -34,15 +33,6 @@ public class HttpWebRequestInstrumentationOptions
/// </summary>
public bool SetHttpFlavor { get; set; }

/// <summary>
/// Gets or sets <see cref="TextMapPropagator"/> for context propagation. Default value: <see cref="CompositeTextMapPropagator"/> with <see cref="TraceContextPropagator"/> &amp; <see cref="BaggagePropagator"/>.
/// </summary>
public TextMapPropagator Propagator { get; set; } = new CompositeTextMapPropagator(new TextMapPropagator[]
{
new TraceContextPropagator(),
new BaggagePropagator(),
});

/// <summary>
/// Gets or sets a Filter function to filter instrumentation for requests on a per request basis.
/// The Filter gets the HttpWebRequest, and should return a boolean.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public override void OnStartActivity(Activity activity, object payload)
return;
}

if (this.options.Propagator.Extract(default, request, HttpRequestMessageHeaderValuesGetter) != default)
if (Propagators.DefaultTextMapPropagator.Extract(default, request, HttpRequestMessageHeaderValuesGetter) != default)
{
// this request is already instrumented, we should back off
activity.IsAllDataRequested = false;
Expand Down Expand Up @@ -115,9 +115,11 @@ public override void OnStartActivity(Activity activity, object payload)
}
}

if (!(this.httpClientSupportsW3C && this.options.Propagator is TraceContextPropagator))
var textMapPropagator = Propagators.DefaultTextMapPropagator;

if (!(this.httpClientSupportsW3C && textMapPropagator is TraceContextPropagator))
{
this.options.Propagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageHeaderValueSetter);
textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageHeaderValueSetter);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,11 @@ private static void AddExceptionTags(Exception exception, Activity activity)

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void InstrumentRequest(HttpWebRequest request, Activity activity)
=> Options.Propagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpWebRequestHeaderValuesSetter);
=> Propagators.DefaultTextMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpWebRequestHeaderValuesSetter);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static bool IsRequestInstrumented(HttpWebRequest request)
=> Options.Propagator.Extract(default, request, HttpWebRequestHeaderValuesGetter) != default;
=> Propagators.DefaultTextMapPropagator.Extract(default, request, HttpWebRequestHeaderValuesGetter) != default;

private static void ProcessRequest(HttpWebRequest request)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ public void AspNetRequestsAreCollectedSuccessfully(
}

var activityProcessor = new Mock<BaseProcessor<Activity>>();
Sdk.SetDefaultTextMapPropagator(propagator.Object);
using (openTelemetry = Sdk.CreateTracerProviderBuilder()
.AddAspNetInstrumentation(
(options) =>
Expand All @@ -163,11 +164,6 @@ public void AspNetRequestsAreCollectedSuccessfully(
return httpContext.Request.Path != filter;
};

if (!carrierFormat.Equals("TraceContext"))
{
options.Propagator = propagator.Object;
}

options.Enrich = ActivityEnrichment;
})
.SetResource(expectedResource)
Expand Down
23 changes: 7 additions & 16 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,6 @@ public void AddAspNetCoreInstrumentation_BadArgs()
Assert.Throws<ArgumentNullException>(() => builder.AddAspNetCoreInstrumentation());
}

[Fact]
public void DefaultPropagatorIsFromPropagators()
{
var options = new AspNetCoreInstrumentationOptions();
Assert.Same(Propagators.DefaultTextMapPropagator, options.Propagator);
}

[Fact]
public void PropagatorSetDoesNotAffectGlobalPropagators()
{
var options = new AspNetCoreInstrumentationOptions();
options.Propagator = new TraceContextPropagator();
Assert.NotSame(Propagators.DefaultTextMapPropagator, options.Propagator);
}

[Fact]
public async Task StatusIsUnsetOn200Response()
{
Expand Down Expand Up @@ -225,8 +210,9 @@ public async Task CustomPropagator()
.WithWebHostBuilder(builder =>
builder.ConfigureTestServices(services =>
{
Sdk.SetDefaultTextMapPropagator(propagator.Object);
this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation((opt) => opt.Propagator = propagator.Object)
.AddAspNetCoreInstrumentation()
.AddProcessor(activityProcessor.Object)
.Build();
})))
Expand All @@ -250,6 +236,11 @@ public async Task CustomPropagator()

Assert.Equal(expectedTraceId, activity.Context.TraceId);
Assert.Equal(expectedSpanId, activity.ParentSpanId);
Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[]
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
{
new TraceContextPropagator(),
new BaggagePropagator(),
}));
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,6 @@ public async Task HttpClientInstrumentationInjectsHeadersAsync(bool shouldEnrich
parent.TraceStateString = "k1=v1,k2=v2";
parent.ActivityTraceFlags = ActivityTraceFlags.Recorded;

// Ensure that the header value func does not throw if the header key can't be found
var mockPropagator = new Mock<TextMapPropagator>();

// var isInjectedHeaderValueGetterThrows = false;
// mockTextFormat
// .Setup(x => x.IsInjected(It.IsAny<HttpRequestMessage>(), It.IsAny<Func<HttpRequestMessage, string, IEnumerable<string>>>()))
Expand All @@ -97,7 +94,6 @@ public async Task HttpClientInstrumentationInjectsHeadersAsync(bool shouldEnrich
using (Sdk.CreateTracerProviderBuilder()
.AddHttpClientInstrumentation(o =>
{
o.Propagator = mockPropagator.Object;
if (shouldEnrich)
{
o.Enrich = ActivityEnrichment;
Expand Down Expand Up @@ -155,10 +151,11 @@ public async Task HttpClientInstrumentationInjectsHeadersAsync_CustomFormat(bool
parent.TraceStateString = "k1=v1,k2=v2";
parent.ActivityTraceFlags = ActivityTraceFlags.Recorded;

Sdk.SetDefaultTextMapPropagator(propagator.Object);

using (Sdk.CreateTracerProviderBuilder()
.AddHttpClientInstrumentation((opt) =>
{
opt.Propagator = propagator.Object;
if (shouldEnrich)
{
opt.Enrich = ActivityEnrichment;
Expand Down Expand Up @@ -187,6 +184,11 @@ public async Task HttpClientInstrumentationInjectsHeadersAsync_CustomFormat(bool

Assert.Equal($"00/{activity.Context.TraceId}/{activity.Context.SpanId}/01", traceparents.Single());
Assert.Equal("k1=v1,k2=v2", tracestates.Single());
Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[]
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
{
new TraceContextPropagator(),
new BaggagePropagator(),
}));
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,10 @@ public async Task HttpWebRequestInstrumentationInjectsHeadersAsync_CustomFormat(
});

var activityProcessor = new Mock<BaseProcessor<Activity>>();
Sdk.SetDefaultTextMapPropagator(propagator.Object);
using var shutdownSignal = Sdk.CreateTracerProviderBuilder()
.AddProcessor(activityProcessor.Object)
.AddHttpWebRequestInstrumentation(options => options.Propagator = propagator.Object)
.AddHttpWebRequestInstrumentation()
.Build();

var request = (HttpWebRequest)WebRequest.Create(this.url);
Expand Down Expand Up @@ -140,6 +141,11 @@ public async Task HttpWebRequestInstrumentationInjectsHeadersAsync_CustomFormat(
Assert.Equal("k1=v1,k2=v2", tracestate);

parent.Stop();
Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[]
{
new TraceContextPropagator(),
new BaggagePropagator(),
}));
}

[Fact]
Expand Down