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

Remove ActivitySourceAdapter #1836

Merged
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
febda2d
Removed ActivitySourceAdapatee
utpilla Feb 17, 2021
1c8e418
Address PR comments
utpilla Feb 17, 2021
0f21319
Address PR Comments
utpilla Feb 17, 2021
999ca2b
Updated TraceBenchmark
utpilla Feb 18, 2021
6cd0ced
Update TracerProviderSdk
utpilla Feb 18, 2021
9291faa
Address PR comments
utpilla Feb 18, 2021
c696f15
Merge remote-tracking branch 'origin/main' into utpilla/Remove-Activi…
utpilla Feb 18, 2021
808a25b
Address PR comments
utpilla Feb 18, 2021
2a3fac7
Update ASP.NET and gRPC client instrumentation to work without Activi…
utpilla Feb 18, 2021
6d71638
Remove ActivitySourceAdapter from ASP.NET and gRPC client instrumenta…
utpilla Feb 19, 2021
1f32aa6
Remove the incorrectly added file
utpilla Feb 19, 2021
0dc0772
Updated CHANGELOG.md
utpilla Feb 19, 2021
1970d6f
Update Unit Tests
utpilla Feb 19, 2021
7f0b2f8
Added Unit Tests for sampling
utpilla Feb 19, 2021
f24b04d
Add Unit Tests for SamplingParameters
utpilla Feb 20, 2021
bc32f82
Updated Unit Tests to test if there are listeners for emptyActivitySo…
utpilla Feb 20, 2021
0869911
Merge branch 'main' into utpilla/Remove-ActivitySourceAdapter
cijothomas Feb 22, 2021
04fe5e0
Updated TracerProviderSk ActivityStop callback logic; Set framework a…
utpilla Feb 23, 2021
1685599
Updated the data structure for legacyOperationNames from HashSet<stri…
utpilla Feb 23, 2021
90df48a
Resolve merge conflicts
utpilla Feb 23, 2021
4c5df69
Fix typos
utpilla Feb 23, 2021
5fe89c0
Added unit tests for AddLegacyOperationName
utpilla Feb 24, 2021
0e337f7
Update unit test to dispose the TracerProvider in unit tests
utpilla Feb 24, 2021
ce2173c
Merge branch 'main' into utpilla/Remove-ActivitySourceAdapter
cijothomas Feb 25, 2021
d499a4d
Address PR comments
utpilla Feb 25, 2021
32824d0
Merge remote-tracking branch 'fork/utpilla/Remove-ActivitySourceAdapt…
utpilla Feb 25, 2021
c9deb90
Fix unit tests
utpilla Feb 25, 2021
a8509ba
Merge branch 'main' into utpilla/Remove-ActivitySourceAdapter
cijothomas Feb 25, 2021
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
2 changes: 1 addition & 1 deletion docs/trace/getting-started/getting-started.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<ItemGroup>
<!---
<PackageReference Include="OpenTelemetry.Exporter.Console" Version="$(OpenTelemetryExporterConsolePkgVer)" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,11 @@ internal class AspNetInstrumentation : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="AspNetInstrumentation"/> class.
/// </summary>
/// <param name="activitySource">ActivitySource adapter instance.</param>
/// <param name="options">Configuration options for ASP.NET instrumentation.</param>
public AspNetInstrumentation(ActivitySourceAdapter activitySource, AspNetInstrumentationOptions options)
public AspNetInstrumentation(AspNetInstrumentationOptions options)
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(
name => new HttpInListener(name, options, activitySource),
name => new HttpInListener(name, options),
listener => listener.Name == AspNetDiagnosticListenerName,
null);
this.diagnosticSourceSubscriber.Subscribe();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,11 @@ internal class HttpInListener : ListenerHandler
private readonly PropertyFetcher<object> routeFetcher = new PropertyFetcher<object>("Route");
private readonly PropertyFetcher<string> routeTemplateFetcher = new PropertyFetcher<string>("RouteTemplate");
private readonly AspNetInstrumentationOptions options;
private readonly ActivitySourceAdapter activitySource;

public HttpInListener(string name, AspNetInstrumentationOptions options, ActivitySourceAdapter activitySource)
public HttpInListener(string name, AspNetInstrumentationOptions options)
: base(name)
{
this.options = options ?? throw new ArgumentNullException(nameof(options));
this.activitySource = activitySource;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Activity is retrieved from Activity.Current later and disposed.")]
Expand Down Expand Up @@ -98,6 +96,9 @@ public override void OnStartActivity(Activity activity, object payload)
// correctly stop and restore Activity.Current.
newOne.SetCustomProperty("OTel.ActivityByAspNet", activity);
activity.SetCustomProperty("OTel.ActivityByHttpInListener", newOne);

// Set IsAllDataRequested to false for the activity created by the framework to only export the sibling activity and not the framework activity
activity.IsAllDataRequested = false;
activity = newOne;
}

Expand All @@ -111,7 +112,8 @@ public override void OnStartActivity(Activity activity, object payload)
var path = requestValues.Path;
activity.DisplayName = path;

this.activitySource.Start(activity, ActivityKind.Server, ActivitySource);
ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);

if (activity.IsAllDataRequested)
{
Expand Down Expand Up @@ -244,8 +246,6 @@ public override void OnStopActivity(Activity activity, object payload)
Activity.Current = activity;
}
}

this.activitySource.Stop(activityToEnrich);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ public static class TracerProviderBuilderExtensions
var aspnetOptions = new AspNetInstrumentationOptions();
configureAspNetInstrumentationOptions?.Invoke(aspnetOptions);

builder.AddDiagnosticSourceInstrumentation((activitySource) => new AspNetInstrumentation(activitySource, aspnetOptions));
builder.AddInstrumentation(() => new AspNetInstrumentation(aspnetOptions));
builder.AddSource(typeof(AspNetInstrumentation).Assembly.GetName().Name);
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

please move the sourcename to a constant. There is inconsistency now as this uses AspNetInstrumentation vs HttpInListener actually. (name is ultimately same, but lets do this consistently)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to use ActivitySourceName from the listener implementation for all four instrumentation libraries

builder.AddLegacyActivityOperationName("Microsoft.AspNet.HttpReqIn");
Copy link
Member

Choose a reason for hiding this comment

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

dont we create sibling activities in asp.net also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I missed this one. I have added it.


return builder;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ internal class AspNetCoreInstrumentation : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="AspNetCoreInstrumentation"/> class.
/// </summary>
/// <param name="activitySource">ActivitySource adapter instance.</param>
/// <param name="options">Configuration options for ASP.NET Core instrumentation.</param>
public AspNetCoreInstrumentation(ActivitySourceAdapter activitySource, AspNetCoreInstrumentationOptions options)
public AspNetCoreInstrumentation(AspNetCoreInstrumentationOptions options)
Copy link
Member

Choose a reason for hiding this comment

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

I initially thought (inccoreclty) this is a breaking change! The public api analyzer is a gift!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we change this to internal?

{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options, activitySource), null);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options), null);
Copy link
Member

Choose a reason for hiding this comment

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

there are lot of special names - lets move them to a common place where it can be commented and probbaly linked to aspnetcore repo source code.
(separate PR)

this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,12 @@ internal class HttpInListener : ListenerHandler
private readonly PropertyFetcher<string> beforeActionTemplateFetcher = new PropertyFetcher<string>("Template");
private readonly bool hostingSupportsW3C;
private readonly AspNetCoreInstrumentationOptions options;
private readonly ActivitySourceAdapter activitySource;

public HttpInListener(string name, AspNetCoreInstrumentationOptions options, ActivitySourceAdapter activitySource)
public HttpInListener(string name, AspNetCoreInstrumentationOptions options)
: base(name)
{
this.hostingSupportsW3C = typeof(HttpRequest).Assembly.GetName().Version.Major >= 3;
this.options = options ?? throw new ArgumentNullException(nameof(options));
this.activitySource = activitySource;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")]
Expand Down Expand Up @@ -99,6 +97,9 @@ public override void OnStartActivity(Activity activity, object payload)

// Starting the new activity make it the Activity.Current one.
newOne.Start();

// Set IsAllDataRequested to false for the activity created by the framework to only export the sibling activity and not the framework activity
activity.IsAllDataRequested = false;
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
activity = newOne;
}

Expand All @@ -108,7 +109,8 @@ public override void OnStartActivity(Activity activity, object payload)
}
}

this.activitySource.Start(activity, ActivityKind.Server, ActivitySource);
ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server);

if (activity.IsAllDataRequested)
{
Expand Down Expand Up @@ -208,8 +210,6 @@ public override void OnStopActivity(Activity activity, object payload)
// the one created by the instrumentation.
// And retrieve it here, and set it to Current.
}

this.activitySource.Stop(activity);
}

public override void OnCustom(string name, Activity activity, object payload)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ public static class TracerProviderBuilderExtensions

var aspnetCoreOptions = new AspNetCoreInstrumentationOptions();
configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions);
builder.AddDiagnosticSourceInstrumentation((activitySource) => new AspNetCoreInstrumentation(activitySource, aspnetCoreOptions));
builder.AddInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions));
builder.AddSource(typeof(AspNetCoreInstrumentation).Assembly.GetName().Name);
builder.AddLegacyActivityOperationName("Microsoft.AspNetCore.Hosting.HttpRequestIn"); // for the activities created by AspNetCore
builder.AddLegacyActivityOperationName("ActivityCreatedByHttpInListener"); // for the sibling activities created by the instrumentation library

return builder;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,10 @@ internal class GrpcClientInstrumentation : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="GrpcClientInstrumentation"/> class.
/// </summary>
/// <param name="activitySource">ActivitySource adapter instance.</param>
/// <param name="options">Configuration options for Grpc client instrumentation.</param>
public GrpcClientInstrumentation(ActivitySourceAdapter activitySource, GrpcClientInstrumentationOptions options = null)
public GrpcClientInstrumentation(GrpcClientInstrumentationOptions options = null)
{
if (activitySource == null)
{
throw new ArgumentNullException(nameof(activitySource));
}

this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new GrpcClientDiagnosticListener(activitySource, options), null);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new GrpcClientDiagnosticListener(options), null);
this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,13 @@ internal class GrpcClientDiagnosticListener : ListenerHandler
internal static readonly ActivitySource ActivitySource = new ActivitySource(ActivitySourceName, Version.ToString());

private readonly GrpcClientInstrumentationOptions options;
private readonly ActivitySourceAdapter activitySource;
private readonly PropertyFetcher<HttpRequestMessage> startRequestFetcher = new PropertyFetcher<HttpRequestMessage>("Request");
private readonly PropertyFetcher<HttpResponseMessage> stopRequestFetcher = new PropertyFetcher<HttpResponseMessage>("Response");

public GrpcClientDiagnosticListener(ActivitySourceAdapter activitySource, GrpcClientInstrumentationOptions options)
public GrpcClientDiagnosticListener(GrpcClientInstrumentationOptions options)
: base("Grpc.Net.Client")
{
if (activitySource == null)
{
throw new ArgumentNullException(nameof(activitySource));
}

this.options = options;
this.activitySource = activitySource;
}

public override void OnStartActivity(Activity activity, object payload)
Expand Down Expand Up @@ -89,7 +82,8 @@ public override void OnStartActivity(Activity activity, object payload)

activity.DisplayName = grpcMethod?.Trim('/');

this.activitySource.Start(activity, ActivityKind.Client, ActivitySource);
ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);

if (activity.IsAllDataRequested)
{
Expand Down Expand Up @@ -158,8 +152,6 @@ public override void OnStopActivity(Activity activity, object payload)
}
}
}

this.activitySource.Stop(activity);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ public static class TracerProviderBuilderExtensions
var grpcOptions = new GrpcClientInstrumentationOptions();
configure?.Invoke(grpcOptions);

builder.AddDiagnosticSourceInstrumentation((activitySource) => new GrpcClientInstrumentation(activitySource, grpcOptions));
builder.AddInstrumentation(() => new GrpcClientInstrumentation(grpcOptions));
builder.AddSource(typeof(GrpcClientInstrumentation).Assembly.GetName().Name);
builder.AddLegacyActivityOperationName("Grpc.Net.Client.GrpcOut");

return builder;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ internal class HttpClientInstrumentation : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="HttpClientInstrumentation"/> class.
/// </summary>
/// <param name="activitySourceAdapter">ActivitySource adapter instance.</param>
/// <param name="options">Configuration options for HTTP client instrumentation.</param>
public HttpClientInstrumentation(ActivitySourceAdapter activitySourceAdapter, HttpClientInstrumentationOptions options)
public HttpClientInstrumentation(HttpClientInstrumentationOptions options)
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpHandlerDiagnosticListener(options, activitySourceAdapter), null);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpHandlerDiagnosticListener(options), null);
this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,14 @@ internal class HttpHandlerDiagnosticListener : ListenerHandler

private static readonly Regex CoreAppMajorVersionCheckRegex = new Regex("^\\.NETCoreApp,Version=v(\\d+)\\.", RegexOptions.Compiled | RegexOptions.IgnoreCase);

private readonly ActivitySourceAdapter activitySource;
private readonly PropertyFetcher<HttpRequestMessage> startRequestFetcher = new PropertyFetcher<HttpRequestMessage>("Request");
private readonly PropertyFetcher<HttpResponseMessage> stopResponseFetcher = new PropertyFetcher<HttpResponseMessage>("Response");
private readonly PropertyFetcher<Exception> stopExceptionFetcher = new PropertyFetcher<Exception>("Exception");
private readonly PropertyFetcher<TaskStatus> stopRequestStatusFetcher = new PropertyFetcher<TaskStatus>("RequestTaskStatus");
private readonly bool httpClientSupportsW3C;
private readonly HttpClientInstrumentationOptions options;

public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options, ActivitySourceAdapter activitySource)
public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options)
: base("HttpHandlerDiagnosticListener")
{
var framework = Assembly
Expand All @@ -64,7 +63,6 @@ public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options, A
}

this.options = options;
this.activitySource = activitySource;
}

public override void OnStartActivity(Activity activity, object payload)
Expand Down Expand Up @@ -107,7 +105,8 @@ public override void OnStartActivity(Activity activity, object payload)

activity.DisplayName = HttpTagHelper.GetOperationNameForHttpMethod(request.Method);

this.activitySource.Start(activity, ActivityKind.Client, ActivitySource);
ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource);
ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client);

if (activity.IsAllDataRequested)
{
Expand Down Expand Up @@ -178,8 +177,6 @@ public override void OnStopActivity(Activity activity, object payload)
}
}
}

this.activitySource.Stop(activity);
}

public override void OnException(Activity activity, object payload)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ public static class TracerProviderBuilderExtensions

configureHttpClientInstrumentationOptions?.Invoke(httpClientOptions);

builder.AddDiagnosticSourceInstrumentation((activitySource) => new HttpClientInstrumentation(activitySource, httpClientOptions));
builder.AddInstrumentation(() => new HttpClientInstrumentation(httpClientOptions));
builder.AddSource(typeof(HttpClientInstrumentation).Assembly.GetName().Name);
builder.AddLegacyActivityOperationName("System.Net.Http.HttpRequestOut");

#if NETFRAMEWORK
builder.AddHttpWebRequestInstrumentation(configureHttpWebRequestInstrumentationOptions);
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder
static OpenTelemetry.Trace.TracerProviderExtensions.ForceFlush(this OpenTelemetry.Trace.TracerProvider provider, int timeoutMilliseconds = -1) -> bool
13 changes: 10 additions & 3 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@ please check the latest changes

## Unreleased

* Added `ForceFlush` to `TracerProvider`. ([#1837](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1837))
* Added `ForceFlush` to `TracerProvider`.
([#1837](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1837))

* Added a TracerProvierBuilder extension method called
`AddLegacyActivityOperationName` which is used by instrumentation libraries
that use DiagnosticSource to get activities processed without
ActivitySourceAdapter.
[#1836](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1836)

## 1.0.1

Expand Down Expand Up @@ -57,8 +64,8 @@ Released 2021-Jan-29
invalid attributes we now throw an exception instead of logging an error.
([#1720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1720))
* Merging "this" resource with an "other" resource now prioritizes the "other"
resource's attributes in a conflict. We've rectified to follow a recent
change to the spec. We previously prioritized "this" resource's tags.
resource's attributes in a conflict. We've rectified to follow a recent change
to the spec. We previously prioritized "this" resource's tags.
([#1728](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1728))
* `BatchExportProcessor` will now flush any remaining spans left in a `Batch`
after the export operation has completed.
Expand Down
Loading