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

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Feb 17, 2021

Currently, the instrumentation libraries depend on ActivitySourceAdapter to set the activity sampling to comply with the TracerProvider sampler. This PR demonstrates how we could implement instrumentation libraries without using ActivitySourceAdapter. I have updated AspNet, AspNetCore, HttpClient, and gRPC client instrumentation libraries to work without ActivitySourceAdapter.

Changes

  • Removed ActivitySourceAdapter
  • Removed AddDiagnosticSourceInstrumentation
  • Added a public API called AddLegacyOperationName which creates an activity listener for the empty ActivitySourceName and
    maintains a list of operation names to be looked up against on ActivityStarted callback to decide if the legacy activity should be processed
  • Added the sampling check in ActivityListener OnStarted to sample the activities generated by instrumentations correctly
  • Added the instrumentation ActivitySource such as "OpenTelemetry.Instrumentation.AspNetCore" and "OpenTelemetry.Instrumentation.Http" to sources in AddInstrumentation method of TracerProviderBuilderSdk.cs. ActivitySource is updated from an empty string to the assembly name in OnStartActivity of the instrumentation listeners, therefore we need to add these assembly names as a Source to the provider so that ActivityStopped callback can be called.
  • Moved the code to set ActivitySource and Activity Kind for instrumented activities to ActivityInstrumentationHelper.cs under OpenTelemetry\Internal which is used by both AspNetCore and HttpClient instrumentation libraries.
  • CHANGELOG.md updated for non-trivial changes

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks! The overall approach looks good. Need to optimize the perf in hot path, so some bencmark results before/after would need to be seen.

@cijothomas
Copy link
Member

@alanwest @CodeBlanch please take a look. This would eliminate the need of ActivitySourceAdapater, by making the TracerProviderSdk itself being aware of legacy activities.

@CodeBlanch
Copy link
Member

I like this approach! It kind of makes what I'm doing on #1761 impossible, but probably better to approach that through the runtime team anyway 😄

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #1836 (b6e38cc) into main (cb066cb) will increase coverage by 0.17%.
The diff coverage is 99.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1836      +/-   ##
==========================================
+ Coverage   83.77%   83.95%   +0.17%     
==========================================
  Files         187      187              
  Lines        5967     5970       +3     
==========================================
+ Hits         4999     5012      +13     
+ Misses        968      958      -10     
Impacted Files Coverage Δ
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 71.42% <ø> (ø)
src/OpenTelemetry/Trace/TracerProviderSdk.cs 95.16% <98.64%> (+4.38%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <100.00%> (+0.05%) ⬆️
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 89.10% <100.00%> (ø)
...entation.AspNet/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...umentation.AspNetCore/AspNetCoreInstrumentation.cs 100.00% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 86.56% <100.00%> (ø)
...tion.AspNetCore/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...ntation.GrpcNetClient/GrpcClientInstrumentation.cs 100.00% <100.00%> (+12.50%) ⬆️
... and 12 more

@utpilla
Copy link
Contributor Author

utpilla commented Feb 19, 2021

With this new approach, we have introduced a few conditional checks to the TracerProviderSdk hot path for processing activities. I ran some benchmark tests and found that this has increased the mean time to run for the Tracer Benchmark test by up to ~20ns.

I feel this is acceptable considering the convenience it brings by getting rid of ActivitySourceAdapter entirely.

Here are the benchmarking results:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET Core SDK=5.0.103
[Host] : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
DefaultJob : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT

With the new changes:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
NoListener 23.50 ns 0.034 ns 0.058 ns - - - -
PropagationDataListner 688.19 ns 2.647 ns 2.476 ns 0.1078 - - 680 B
AllDataListner 684.48 ns 3.639 ns 3.404 ns 0.1078 - - 680 B
AllDataAndRecordedListner 687.80 ns 2.414 ns 2.140 ns 0.1078 - - 680 B
OneProcessor 716.29 ns 1.442 ns 1.349 ns 0.1078 - - 680 B
TwoProcessors 736.54 ns 5.225 ns 4.632 ns 0.1078 - - 680 B
ThreeProcessors 743.95 ns 3.218 ns 2.687 ns 0.1078 - - 680 B
OneInstrumentation 730.72 ns 1.876 ns 1.663 ns 0.1078 - - 680 B
TwoInstrumentations 708.67 ns 1.221 ns 1.020 ns 0.1078 - - 680 B

I added two Benchmark tests called OneInstrumentation and TwoInstrumentations:

  • OneInstrumentation adds AspNetCoreInstrumentation to the provider
  • TwoInstrumentations adds AspNetCoreInstrumentation and HttpClientInstrumentation to the provider
    Both of these tests also add one processor to the TracerProvider.

Without these changes (main branch):

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
NoListener 23.45 ns 0.034 ns 0.042 ns - - - -
PropagationDataListner 677.92 ns 1.638 ns 1.368 ns 0.1078 - - 680 B
AllDataListner 685.51 ns 2.517 ns 2.354 ns 0.1078 - - 680 B
AllDataAndRecordedListner 684.66 ns 1.621 ns 1.354 ns 0.1078 - - 680 B
OneProcessor 720.67 ns 1.455 ns 1.361 ns 0.1078 - - 680 B
TwoProcessors 711.47 ns 2.183 ns 1.823 ns 0.1078 - - 680 B
ThreeProcessors 721.47 ns 1.324 ns 1.239 ns 0.1078 - - 680 B

@utpilla utpilla marked this pull request as ready for review February 19, 2021 19:56
@utpilla utpilla requested a review from a team February 19, 2021 19:56
@@ -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);
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

/// <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)

builder.AddDiagnosticSourceInstrumentation((activitySource) => new AspNetInstrumentation(activitySource, aspnetOptions));
builder.AddInstrumentation(() => new AspNetInstrumentation(aspnetOptions));
builder.AddSource(typeof(AspNetInstrumentation).Assembly.GetName().Name);
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.

this TracerProviderBuilder tracerProviderBuilder,
Func<ActivitySourceAdapter, TInstrumentation> instrumentationFactory)
where TInstrumentation : class
public static TracerProviderBuilder AddLegacyActivityOperationName(this TracerProviderBuilder tracerProviderBuilder, string operationName)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static TracerProviderBuilder AddLegacyActivityOperationName(this TracerProviderBuilder tracerProviderBuilder, string operationName)
public static TracerProviderBuilder AddLegacyActivity(this TracerProviderBuilder tracerProviderBuilder, string operationName)

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 name to AddLegacyActivity and updated the public API docs as well.

/// Adds a DiagnosticSource based instrumentation.
/// This is required for libraries which is already instrumented with
/// DiagnosticSource and Activity, without using ActivitySource.
/// Adds an activity listener for activities created the legacy way with the provided OperationName. The listener samples and processes these activities
Copy link
Member

Choose a reason for hiding this comment

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

/// Adds activity with given operation name to the list of subscribed activities. This is for legacy activities (i.e activities created without using ActivituSource API).

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 description.

/// Adds a DiagnosticSource based instrumentation.
/// This is required for libraries which is already instrumented with
/// DiagnosticSource and Activity, without using ActivitySource.
/// Adds the OperationName of an activity created by DiagnosticSource instrumentation to the provider.
Copy link
Member

Choose a reason for hiding this comment

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

its misleading to say "created by DiagnosticSource instrumentation".

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 description for this to match the public extension method's description.

Sdk.CreateTracerProviderBuilder()
.SetSampler(new AlwaysOnSampler())
.AddSource(this.sourceWithOneInstrumentation.Name)
.AddAspNetCoreInstrumentation()
Copy link
Member

Choose a reason for hiding this comment

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

This benchmark has nothing to do with AddAspNetCoreInstrumentation. You can simply use the raw API directly - AddLegacyActivity("somename")

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 removed the instrumentations and used AddLegacyActivity.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM.
Left a few comments, nothing blocking. Okay to have them as follow up PRs.

builder.AddDiagnosticSourceInstrumentation((activitySource) => new GrpcClientInstrumentation(activitySource, grpcOptions));
builder.AddInstrumentation(() => new GrpcClientInstrumentation(grpcOptions));
builder.AddSource(GrpcClientDiagnosticListener.ActivitySourceName);
builder.AddLegacyActivity("Grpc.Net.Client.GrpcOut");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: create as a const, just like aspnetcore

builder.AddDiagnosticSourceInstrumentation((activitySource) => new HttpClientInstrumentation(activitySource, httpClientOptions));
builder.AddInstrumentation(() => new HttpClientInstrumentation(httpClientOptions));
builder.AddSource(HttpHandlerDiagnosticListener.ActivitySourceName);
builder.AddLegacyActivity("System.Net.Http.HttpRequestOut");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: create const


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 TracerProviderBuilder AddDiagnosticSourceInstrumentation<TInstrumentation>(
Func<ActivitySourceAdapter, TInstrumentation> instrumentationFactory)
where TInstrumentation : class
internal TracerProviderBuilder AddLegacyActivity(string operationName)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could change this to params string[], we have many cases calling this twice.

@cijothomas
Copy link
Member

@open-telemetry/dotnet-approvers Please take a look. Will proceed to merge this in next few hours as 1st step in unblocking instrumentations in contrib repo.
This is adding new public API, if there are feedbacks about the name, etc - we have opportunity to change it before the next stable release.

static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivity(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this name. How about AddLegacySource? Given we have AddSource already.

Copy link
Member

Choose a reason for hiding this comment

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

I am okay with AddLegacySource

Copy link
Member

Choose a reason for hiding this comment

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

we have opportunity to rename this until 1.1 stable is out, so i'd prefer to not block this pr.

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me, I'll stamp as approved.

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