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 5 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
26 changes: 26 additions & 0 deletions docs/trace/getting-started/ActivitySourceDemo.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// <copyright file="ActivitySourceDemo.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Diagnostics;

namespace Demo
{
internal static class ActivitySourceDemo
{
internal static readonly ActivitySource MyActivitySource = new ActivitySource(
"MyCompany.MyProduct.MyLibrary");
}
}
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 @@ -108,7 +106,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 +207,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 @@ -15,6 +15,7 @@
// </copyright>

using System;
using System.Reflection;
using OpenTelemetry.Instrumentation.AspNetCore;

namespace OpenTelemetry.Trace
Expand All @@ -41,7 +42,9 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation(

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");

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 TracerProviderBuilder AddHttpClientInstrumentation(

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
@@ -0,0 +1 @@
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder
44 changes: 44 additions & 0 deletions src/OpenTelemetry/Internal/ActivityInstrumentationHelper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// <copyright file="ActivityInstrumentationHelper.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using System.Linq.Expressions;

namespace OpenTelemetry.Instrumentation
{
internal static class ActivityInstrumentationHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

should we compile the existing extension instead of duplicating it?

{
internal static readonly Action<Activity, ActivityKind> SetKindProperty = CreateActivityKindSetter();
internal static readonly Action<Activity, ActivitySource> SetActivitySourceProperty = CreateActivitySourceSetter();

private static Action<Activity, ActivitySource> CreateActivitySourceSetter()
{
ParameterExpression instance = Expression.Parameter(typeof(Activity), "instance");
ParameterExpression propertyValue = Expression.Parameter(typeof(ActivitySource), "propertyValue");
var body = Expression.Assign(Expression.Property(instance, "Source"), propertyValue);
return Expression.Lambda<Action<Activity, ActivitySource>>(body, instance, propertyValue).Compile();
}

private static Action<Activity, ActivityKind> CreateActivityKindSetter()
{
ParameterExpression instance = Expression.Parameter(typeof(Activity), "instance");
ParameterExpression propertyValue = Expression.Parameter(typeof(ActivityKind), "propertyValue");
var body = Expression.Assign(Expression.Property(instance, "Kind"), propertyValue);
return Expression.Lambda<Action<Activity, ActivityKind>>(body, instance, propertyValue).Compile();
}
}
}
Loading