From febda2d6e02badf299ef4c88faa2c27abd41c1a4 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 17 Feb 2021 00:02:20 -0800 Subject: [PATCH 01/22] Removed ActivitySourceAdapatee --- .../getting-started/ActivitySourceDemo.cs | 26 +++ .../AspNetCoreInstrumentation.cs | 5 +- .../Implementation/HttpInListener.cs | 9 +- .../TracerProviderBuilderExtensions.cs | 3 +- .../HttpClientInstrumentation.cs | 5 +- .../HttpHandlerDiagnosticListener.cs | 9 +- .../TracerProviderBuilderExtensions.cs | 2 +- .../Internal/ActivityInstrumentationHelper.cs | 44 ++++ .../Trace/ActivitySourceAdapter.cs | 193 ------------------ .../Trace/TracerProviderBuilderExtensions.cs | 2 +- .../Trace/TracerProviderBuilderSdk.cs | 8 +- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 104 +++++++++- 12 files changed, 186 insertions(+), 224 deletions(-) create mode 100644 docs/trace/getting-started/ActivitySourceDemo.cs create mode 100644 src/OpenTelemetry/Internal/ActivityInstrumentationHelper.cs delete mode 100644 src/OpenTelemetry/Trace/ActivitySourceAdapter.cs diff --git a/docs/trace/getting-started/ActivitySourceDemo.cs b/docs/trace/getting-started/ActivitySourceDemo.cs new file mode 100644 index 00000000000..432118f819e --- /dev/null +++ b/docs/trace/getting-started/ActivitySourceDemo.cs @@ -0,0 +1,26 @@ +// +// 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. +// + +using System.Diagnostics; + +namespace Demo +{ + internal static class ActivitySourceDemo + { + internal static readonly ActivitySource MyActivitySource = new ActivitySource( + "MyCompany.MyProduct.MyLibrary"); + } +} diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs index 2a00a2b7b07..61fbc8e5100 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentation.cs @@ -29,11 +29,10 @@ internal class AspNetCoreInstrumentation : IDisposable /// /// Initializes a new instance of the class. /// - /// ActivitySource adapter instance. /// Configuration options for ASP.NET Core instrumentation. - public AspNetCoreInstrumentation(ActivitySourceAdapter activitySource, AspNetCoreInstrumentationOptions options) + public AspNetCoreInstrumentation(AspNetCoreInstrumentationOptions options) { - this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options, activitySource), null); + this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options), null); this.diagnosticSourceSubscriber.Subscribe(); } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index ed951e04c01..1888ae409d5 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -45,14 +45,12 @@ internal class HttpInListener : ListenerHandler private readonly PropertyFetcher beforeActionTemplateFetcher = new PropertyFetcher("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.")] @@ -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) { @@ -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) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index ef2e61436d2..3f9b153bcb9 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -15,6 +15,7 @@ // using System; +using System.Reflection; using OpenTelemetry.Instrumentation.AspNetCore; namespace OpenTelemetry.Trace @@ -41,7 +42,7 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( var aspnetCoreOptions = new AspNetCoreInstrumentationOptions(); configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions); - builder.AddDiagnosticSourceInstrumentation((activitySource) => new AspNetCoreInstrumentation(activitySource, aspnetCoreOptions)); + builder.AddDiagnosticSourceInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions)); return builder; } diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentation.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentation.cs index 7b982c023b8..4a8a54eddec 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentation.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentation.cs @@ -29,11 +29,10 @@ internal class HttpClientInstrumentation : IDisposable /// /// Initializes a new instance of the class. /// - /// ActivitySource adapter instance. /// Configuration options for HTTP client instrumentation. - 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(); } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 1fbd475564f..4de16246766 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -37,7 +37,6 @@ 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 startRequestFetcher = new PropertyFetcher("Request"); private readonly PropertyFetcher stopResponseFetcher = new PropertyFetcher("Response"); private readonly PropertyFetcher stopExceptionFetcher = new PropertyFetcher("Exception"); @@ -45,7 +44,7 @@ internal class HttpHandlerDiagnosticListener : ListenerHandler private readonly bool httpClientSupportsW3C; private readonly HttpClientInstrumentationOptions options; - public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options, ActivitySourceAdapter activitySource) + public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options) : base("HttpHandlerDiagnosticListener") { var framework = Assembly @@ -64,7 +63,6 @@ public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options, A } this.options = options; - this.activitySource = activitySource; } public override void OnStartActivity(Activity activity, object payload) @@ -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) { @@ -178,8 +177,6 @@ public override void OnStopActivity(Activity activity, object payload) } } } - - this.activitySource.Stop(activity); } public override void OnException(Activity activity, object payload) diff --git a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs index 16a0c58b581..1e701c24bc7 100644 --- a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs @@ -60,7 +60,7 @@ public static TracerProviderBuilder AddHttpClientInstrumentation( configureHttpClientInstrumentationOptions?.Invoke(httpClientOptions); - builder.AddDiagnosticSourceInstrumentation((activitySource) => new HttpClientInstrumentation(activitySource, httpClientOptions)); + builder.AddDiagnosticSourceInstrumentation(() => new HttpClientInstrumentation(httpClientOptions)); #if NETFRAMEWORK builder.AddHttpWebRequestInstrumentation(configureHttpWebRequestInstrumentationOptions); diff --git a/src/OpenTelemetry/Internal/ActivityInstrumentationHelper.cs b/src/OpenTelemetry/Internal/ActivityInstrumentationHelper.cs new file mode 100644 index 00000000000..4e527812f31 --- /dev/null +++ b/src/OpenTelemetry/Internal/ActivityInstrumentationHelper.cs @@ -0,0 +1,44 @@ +// +// 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. +// + +using System; +using System.Diagnostics; +using System.Linq.Expressions; + +namespace OpenTelemetry.Instrumentation +{ + internal static class ActivityInstrumentationHelper + { + internal static readonly Action SetKindProperty = CreateActivityKindSetter(); + internal static readonly Action SetActivitySourceProperty = CreateActivitySourceSetter(); + + private static Action 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>(body, instance, propertyValue).Compile(); + } + + private static Action 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>(body, instance, propertyValue).Compile(); + } + } +} diff --git a/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs b/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs deleted file mode 100644 index 4920bd9ece3..00000000000 --- a/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs +++ /dev/null @@ -1,193 +0,0 @@ -// -// 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. -// - -using System; -using System.Diagnostics; -using System.Linq.Expressions; -using OpenTelemetry.Internal; - -namespace OpenTelemetry.Trace -{ - /// - /// This class encapsulates the logic for performing ActivitySource actions - /// on Activities that are created using default ActivitySource. - /// All activities created without using ActivitySource will have a - /// default ActivitySource assigned to them with their name as empty string. - /// This class is to be used by instrumentation adapters which converts/augments - /// activies created without ActivitySource, into something which closely - /// matches the one created using ActivitySource. - /// - /// - /// This class is meant to be only used when writing new Instrumentation for - /// libraries which are already instrumented with DiagnosticSource/Activity - /// following this doc: - /// https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/ActivityUserGuide.md. - /// - internal class ActivitySourceAdapter - { - private static readonly Action SetKindProperty = CreateActivityKindSetter(); - private static readonly Action SetActivitySourceProperty = CreateActivitySourceSetter(); - private readonly Sampler sampler; - private readonly Action getRequestedDataAction; - private BaseProcessor activityProcessor; - - internal ActivitySourceAdapter(Sampler sampler, BaseProcessor activityProcessor) - { - this.sampler = sampler ?? throw new ArgumentNullException(nameof(sampler)); - if (this.sampler is AlwaysOnSampler) - { - this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOnSampler; - } - else if (this.sampler is AlwaysOffSampler) - { - this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOffSampler; - } - else - { - this.getRequestedDataAction = this.RunGetRequestedDataOtherSampler; - } - - this.activityProcessor = activityProcessor; - } - - private ActivitySourceAdapter() - { - } - - /// - /// Method that starts an . - /// - /// to be started. - /// ActivityKind to be set of the activity. - /// ActivitySource to be set of the activity. - [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1062:Validate arguments of public methods", Justification = "ActivityProcessor is hot path")] - public void Start(Activity activity, ActivityKind kind, ActivitySource source) - { - OpenTelemetrySdkEventSource.Log.ActivityStarted(activity); - - SetActivitySourceProperty(activity, source); - SetKindProperty(activity, kind); - this.getRequestedDataAction(activity); - if (activity.IsAllDataRequested) - { - this.activityProcessor?.OnStart(activity); - } - } - - /// - /// Method that stops an . - /// - /// to be stopped. - public void Stop(Activity activity) - { - OpenTelemetrySdkEventSource.Log.ActivityStopped(activity); - - if (activity?.IsAllDataRequested ?? false) - { - this.activityProcessor?.OnEnd(activity); - } - } - - internal void UpdateProcessor(BaseProcessor processor) - { - this.activityProcessor = processor; - } - - private static Action 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>(body, instance, propertyValue).Compile(); - } - - private static Action 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>(body, instance, propertyValue).Compile(); - } - - private void RunGetRequestedDataAlwaysOnSampler(Activity activity) - { - activity.IsAllDataRequested = true; - activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded; - } - - private void RunGetRequestedDataAlwaysOffSampler(Activity activity) - { - activity.IsAllDataRequested = false; - } - - private void RunGetRequestedDataOtherSampler(Activity activity) - { - ActivityContext parentContext; - - // Check activity.ParentId alone is sufficient to normally determine if a activity is root or not. But if one uses activity.SetParentId to override the TraceId (without intending to set an actual parent), then additional check of parentspanid being empty is required to confirm if an activity is root or not. - // This checker can be removed, once Activity exposes an API to customize ID Generation (https://github.com/dotnet/runtime/issues/46704) or issue https://github.com/dotnet/runtime/issues/46706 is addressed. - if (string.IsNullOrEmpty(activity.ParentId) || activity.ParentSpanId.ToHexString() == "0000000000000000") - { - parentContext = default; - } - else if (activity.Parent != null) - { - parentContext = activity.Parent.Context; - } - else - { - parentContext = new ActivityContext( - activity.TraceId, - activity.ParentSpanId, - activity.ActivityTraceFlags, - activity.TraceStateString, - isRemote: true); - } - - var samplingParameters = new SamplingParameters( - parentContext, - activity.TraceId, - activity.DisplayName, - activity.Kind, - activity.TagObjects, - activity.Links); - - var samplingResult = this.sampler.ShouldSample(samplingParameters); - - switch (samplingResult.Decision) - { - case SamplingDecision.Drop: - activity.IsAllDataRequested = false; - break; - case SamplingDecision.RecordOnly: - activity.IsAllDataRequested = true; - break; - case SamplingDecision.RecordAndSample: - activity.IsAllDataRequested = true; - activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded; - break; - } - - if (samplingResult.Decision != SamplingDecision.Drop) - { - foreach (var att in samplingResult.Attributes) - { - activity.SetTag(att.Key, att.Value); - } - } - } - } -} diff --git a/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs index 599c8985900..4eacdb96a9c 100644 --- a/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs @@ -92,7 +92,7 @@ public static TracerProvider Build(this TracerProviderBuilder tracerProviderBuil /// Returns for chaining. internal static TracerProviderBuilder AddDiagnosticSourceInstrumentation( this TracerProviderBuilder tracerProviderBuilder, - Func instrumentationFactory) + Func instrumentationFactory) where TInstrumentation : class { if (instrumentationFactory == null) diff --git a/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs index 0dca11c1d01..c3e2084311d 100644 --- a/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs @@ -149,7 +149,7 @@ internal TracerProvider Build() /// Function that builds instrumentation. /// Returns for chaining. internal TracerProviderBuilder AddDiagnosticSourceInstrumentation( - Func instrumentationFactory) + Func instrumentationFactory) where TInstrumentation : class { if (instrumentationFactory == null) @@ -163,6 +163,8 @@ internal TracerProviderBuilder AddDiagnosticSourceInstrumentation Factory; + public readonly Func Factory; - internal DiagnosticSourceInstrumentationFactory(string name, string version, Func factory) + internal DiagnosticSourceInstrumentationFactory(string name, string version, Func factory) { this.Name = name; this.Version = version; diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index d0759533a5e..eb647e625fd 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -33,8 +33,8 @@ internal class TracerProviderSdk : TracerProvider private readonly List instrumentations = new List(); private readonly ActivityListener listener; private readonly Sampler sampler; - private readonly ActivitySourceAdapter adapter; private BaseProcessor processor; + private Action getRequestedDataAction; internal TracerProviderSdk( Resource resource, @@ -54,10 +54,9 @@ internal TracerProviderSdk( if (diagnosticSourceInstrumentationFactories.Any()) { - this.adapter = new ActivitySourceAdapter(sampler, this.processor); foreach (var instrumentationFactory in diagnosticSourceInstrumentationFactories) { - this.instrumentations.Add(instrumentationFactory.Factory(this.adapter)); + this.instrumentations.Add(instrumentationFactory.Factory()); } } @@ -76,6 +75,24 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStarted(activity); + if (string.IsNullOrEmpty(activity.Source.Name)) + { + if (this.sampler is AlwaysOnSampler) + { + this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOnSampler; + } + else if (this.sampler is AlwaysOffSampler) + { + this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOffSampler; + } + else + { + this.getRequestedDataAction = this.RunGetRequestedDataOtherSampler; + } + + this.getRequestedDataAction(activity); + } + if (!activity.IsAllDataRequested) { return; @@ -153,11 +170,14 @@ internal TracerProviderSdk( // Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to // or not. - listener.ShouldListenTo = (activitySource) => regex.IsMatch(activitySource.Name); + listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name) || regex.IsMatch(activitySource.Name); } else { - var activitySources = new Dictionary(StringComparer.OrdinalIgnoreCase); + var activitySources = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + { string.Empty, true }, + }; foreach (var name in sources) { @@ -169,6 +189,10 @@ internal TracerProviderSdk( listener.ShouldListenTo = (activitySource) => activitySources.ContainsKey(activitySource.Name); } } + else + { + listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name); + } ActivitySource.AddActivityListener(listener); this.listener = listener; @@ -202,8 +226,6 @@ internal TracerProviderSdk AddProcessor(BaseProcessor processor) }); } - this.adapter?.UpdateProcessor(this.processor); - return this; } @@ -313,5 +335,73 @@ private static ActivitySamplingResult PropagateOrIgnoreData(ActivityTraceId trac ? ActivitySamplingResult.PropagationData : ActivitySamplingResult.None; } + + private void RunGetRequestedDataAlwaysOnSampler(Activity activity) + { + activity.IsAllDataRequested = true; + activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded; + } + + private void RunGetRequestedDataAlwaysOffSampler(Activity activity) + { + activity.IsAllDataRequested = false; + } + + private void RunGetRequestedDataOtherSampler(Activity activity) + { + ActivityContext parentContext; + + // Check activity.ParentId alone is sufficient to normally determine if a activity is root or not. But if one uses activity.SetParentId to override the TraceId (without intending to set an actual parent), then additional check of parentspanid being empty is required to confirm if an activity is root or not. + // This checker can be removed, once Activity exposes an API to customize ID Generation (https://github.com/dotnet/runtime/issues/46704) or issue https://github.com/dotnet/runtime/issues/46706 is addressed. + if (string.IsNullOrEmpty(activity.ParentId) || activity.ParentSpanId.ToHexString() == "0000000000000000") + { + parentContext = default; + } + else if (activity.Parent != null) + { + parentContext = activity.Parent.Context; + } + else + { + parentContext = new ActivityContext( + activity.TraceId, + activity.ParentSpanId, + activity.ActivityTraceFlags, + activity.TraceStateString, + isRemote: true); + } + + var samplingParameters = new SamplingParameters( + parentContext, + activity.TraceId, + activity.DisplayName, + activity.Kind, + activity.TagObjects, + activity.Links); + + var samplingResult = this.sampler.ShouldSample(samplingParameters); + + switch (samplingResult.Decision) + { + case SamplingDecision.Drop: + activity.IsAllDataRequested = false; + break; + case SamplingDecision.RecordOnly: + activity.IsAllDataRequested = true; + break; + case SamplingDecision.RecordAndSample: + activity.IsAllDataRequested = true; + activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded; + break; + } + + if (samplingResult.Decision != SamplingDecision.Drop) + { + foreach (var att in samplingResult.Attributes) + { + activity.SetTag(att.Key, att.Value); + } + } + } } } From 1c8e4188d87cff2451300ed1210ae452120c1311 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 17 Feb 2021 12:49:44 -0800 Subject: [PATCH 02/22] Address PR comments --- .../TracerProviderBuilderExtensions.cs | 4 +- .../TracerProviderBuilderExtensions.cs | 4 +- .../Trace/TracerProviderBuilderExtensions.cs | 37 ++++++----- .../Trace/TracerProviderBuilderSdk.cs | 66 ++++++++----------- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 16 ++--- 5 files changed, 61 insertions(+), 66 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index 3f9b153bcb9..242537abc9f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -42,7 +42,9 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( var aspnetCoreOptions = new AspNetCoreInstrumentationOptions(); configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions); - builder.AddDiagnosticSourceInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions)); + builder.AddInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions)); + builder.AddInstrumentationActivitySource(typeof(AspNetCoreInstrumentation).Assembly.GetName().Name); + builder.AddLegacyActivityOperationName("Microsoft.AspNetCore.Hosting.HttpRequestIn"); return builder; } diff --git a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs index 1e701c24bc7..7c93db1f471 100644 --- a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs @@ -60,7 +60,9 @@ public static TracerProviderBuilder AddHttpClientInstrumentation( configureHttpClientInstrumentationOptions?.Invoke(httpClientOptions); - builder.AddDiagnosticSourceInstrumentation(() => new HttpClientInstrumentation(httpClientOptions)); + builder.AddInstrumentation(() => new HttpClientInstrumentation(httpClientOptions)); + builder.AddInstrumentationActivitySource(typeof(HttpClientInstrumentation).Assembly.GetName().Name); + builder.AddLegacyActivityOperationName("System.Net.Http.HttpRequestOut"); #if NETFRAMEWORK builder.AddHttpWebRequestInstrumentation(configureHttpWebRequestInstrumentationOptions); diff --git a/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs index 4eacdb96a9c..34aeb793d60 100644 --- a/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs @@ -71,41 +71,46 @@ public static TracerProviderBuilder AddProcessor(this TracerProviderBuilder trac return tracerProviderBuilder; } - public static TracerProvider Build(this TracerProviderBuilder tracerProviderBuilder) + /// + /// Adds the ActivitySource of an instrumentation to the provider. + /// + /// TracerProviderBuilder instance. + /// ActivitySource to add. + /// Returns for chaining. + public static TracerProviderBuilder AddInstrumentationActivitySource(this TracerProviderBuilder tracerProviderBuilder, string instrumentationActivitySource) { if (tracerProviderBuilder is TracerProviderBuilderSdk tracerProviderBuilderSdk) { - return tracerProviderBuilderSdk.Build(); + tracerProviderBuilderSdk.AddInstrumentationActivitySource(instrumentationActivitySource); } - return null; + return tracerProviderBuilder; } /// - /// 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. /// - /// Type of instrumentation class. /// TracerProviderBuilder instance. - /// Function that builds instrumentation. + /// OperationName to add. /// Returns for chaining. - internal static TracerProviderBuilder AddDiagnosticSourceInstrumentation( - this TracerProviderBuilder tracerProviderBuilder, - Func instrumentationFactory) - where TInstrumentation : class + public static TracerProviderBuilder AddLegacyActivityOperationName(this TracerProviderBuilder tracerProviderBuilder, string operationName) { - if (instrumentationFactory == null) + if (tracerProviderBuilder is TracerProviderBuilderSdk tracerProviderBuilderSdk) { - throw new ArgumentNullException(nameof(instrumentationFactory)); + tracerProviderBuilderSdk.AddLegacyActivityOperationName(operationName); } + return tracerProviderBuilder; + } + + public static TracerProvider Build(this TracerProviderBuilder tracerProviderBuilder) + { if (tracerProviderBuilder is TracerProviderBuilderSdk tracerProviderBuilderSdk) { - tracerProviderBuilderSdk.AddDiagnosticSourceInstrumentation(instrumentationFactory); + return tracerProviderBuilderSdk.Build(); } - return tracerProviderBuilder; + return null; } } } diff --git a/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs index c3e2084311d..e1288f97893 100644 --- a/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs @@ -26,11 +26,11 @@ namespace OpenTelemetry.Trace /// internal class TracerProviderBuilderSdk : TracerProviderBuilder { - private readonly List diagnosticSourceInstrumentationFactories = new List(); private readonly List instrumentationFactories = new List(); private readonly List> processors = new List>(); private readonly List sources = new List(); + private readonly HashSet legacyActivityOperationNames = new HashSet(); private ResourceBuilder resourceBuilder = ResourceBuilder.CreateDefault(); private Sampler sampler = new ParentBasedSampler(new AlwaysOnSampler()); @@ -129,57 +129,49 @@ internal TracerProviderBuilder AddProcessor(BaseProcessor processor) return this; } - internal TracerProvider Build() + /// + /// Adds the ActivitySource of an instrumentation to the provider. + /// + /// ActivitySource to add. + /// Returns for chaining. + internal TracerProviderBuilder AddInstrumentationActivitySource(string instrumentationActivitySource) { - return new TracerProviderSdk( - this.resourceBuilder.Build(), - this.sources, - this.diagnosticSourceInstrumentationFactories, - this.instrumentationFactories, - this.sampler, - this.processors); + if (string.IsNullOrEmpty(instrumentationActivitySource)) + { + throw new ArgumentException(nameof(instrumentationActivitySource)); + } + + this.sources.Add(instrumentationActivitySource); + + return this; } /// - /// 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. /// - /// Type of instrumentation class. - /// Function that builds instrumentation. + /// OperationName to add. /// Returns for chaining. - internal TracerProviderBuilder AddDiagnosticSourceInstrumentation( - Func instrumentationFactory) - where TInstrumentation : class + internal TracerProviderBuilder AddLegacyActivityOperationName(string operationName) { - if (instrumentationFactory == null) + if (string.IsNullOrEmpty(operationName)) { - throw new ArgumentNullException(nameof(instrumentationFactory)); + throw new ArgumentException(nameof(operationName)); } - this.diagnosticSourceInstrumentationFactories.Add( - new DiagnosticSourceInstrumentationFactory( - typeof(TInstrumentation).Name, - "semver:" + typeof(TInstrumentation).Assembly.GetName().Version, - instrumentationFactory)); - - this.sources.Add(typeof(TInstrumentation).Assembly.GetName().Name); + this.legacyActivityOperationNames.Add(operationName); return this; } - internal readonly struct DiagnosticSourceInstrumentationFactory + internal TracerProvider Build() { - public readonly string Name; - public readonly string Version; - public readonly Func Factory; - - internal DiagnosticSourceInstrumentationFactory(string name, string version, Func factory) - { - this.Name = name; - this.Version = version; - this.Factory = factory; - } + return new TracerProviderSdk( + this.resourceBuilder.Build(), + this.sources, + this.instrumentationFactories, + this.sampler, + this.processors, + this.legacyActivityOperationNames); } internal readonly struct InstrumentationFactory diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index eb647e625fd..cb422b3ec89 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -33,33 +33,27 @@ internal class TracerProviderSdk : TracerProvider private readonly List instrumentations = new List(); private readonly ActivityListener listener; private readonly Sampler sampler; + private readonly HashSet legacyActivityOperationNames; private BaseProcessor processor; private Action getRequestedDataAction; internal TracerProviderSdk( Resource resource, IEnumerable sources, - IEnumerable diagnosticSourceInstrumentationFactories, IEnumerable instrumentationFactories, Sampler sampler, - List> processors) + List> processors, + HashSet legacyActivityOperationNames) { this.Resource = resource; this.sampler = sampler; + this.legacyActivityOperationNames = legacyActivityOperationNames; foreach (var processor in processors) { this.AddProcessor(processor); } - if (diagnosticSourceInstrumentationFactories.Any()) - { - foreach (var instrumentationFactory in diagnosticSourceInstrumentationFactories) - { - this.instrumentations.Add(instrumentationFactory.Factory()); - } - } - if (instrumentationFactories.Any()) { foreach (var instrumentationFactory in instrumentationFactories) @@ -75,7 +69,7 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStarted(activity); - if (string.IsNullOrEmpty(activity.Source.Name)) + if (legacyActivityOperationNames.Contains(activity.OperationName)) { if (this.sampler is AlwaysOnSampler) { From 0f21319e492e949d738d2a867c30782380b21e36 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 17 Feb 2021 13:59:13 -0800 Subject: [PATCH 03/22] Address PR Comments --- .../TracerProviderBuilderExtensions.cs | 2 +- .../TracerProviderBuilderExtensions.cs | 2 +- .../Trace/TracerProviderBuilderExtensions.cs | 16 --------------- .../Trace/TracerProviderBuilderSdk.cs | 17 ---------------- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 20 +++++++++++++------ 5 files changed, 16 insertions(+), 41 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index 242537abc9f..3046580d629 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -43,7 +43,7 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( var aspnetCoreOptions = new AspNetCoreInstrumentationOptions(); configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions); builder.AddInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions)); - builder.AddInstrumentationActivitySource(typeof(AspNetCoreInstrumentation).Assembly.GetName().Name); + builder.AddSource(typeof(AspNetCoreInstrumentation).Assembly.GetName().Name); builder.AddLegacyActivityOperationName("Microsoft.AspNetCore.Hosting.HttpRequestIn"); return builder; diff --git a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs index 7c93db1f471..cbf546c174c 100644 --- a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs @@ -61,7 +61,7 @@ public static TracerProviderBuilder AddHttpClientInstrumentation( configureHttpClientInstrumentationOptions?.Invoke(httpClientOptions); builder.AddInstrumentation(() => new HttpClientInstrumentation(httpClientOptions)); - builder.AddInstrumentationActivitySource(typeof(HttpClientInstrumentation).Assembly.GetName().Name); + builder.AddSource(typeof(HttpClientInstrumentation).Assembly.GetName().Name); builder.AddLegacyActivityOperationName("System.Net.Http.HttpRequestOut"); #if NETFRAMEWORK diff --git a/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs index 34aeb793d60..8fcdc29a917 100644 --- a/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Trace/TracerProviderBuilderExtensions.cs @@ -71,22 +71,6 @@ public static TracerProviderBuilder AddProcessor(this TracerProviderBuilder trac return tracerProviderBuilder; } - /// - /// Adds the ActivitySource of an instrumentation to the provider. - /// - /// TracerProviderBuilder instance. - /// ActivitySource to add. - /// Returns for chaining. - public static TracerProviderBuilder AddInstrumentationActivitySource(this TracerProviderBuilder tracerProviderBuilder, string instrumentationActivitySource) - { - if (tracerProviderBuilder is TracerProviderBuilderSdk tracerProviderBuilderSdk) - { - tracerProviderBuilderSdk.AddInstrumentationActivitySource(instrumentationActivitySource); - } - - return tracerProviderBuilder; - } - /// /// Adds the OperationName of an activity created by DiagnosticSource instrumentation to the provider. /// diff --git a/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs index e1288f97893..de3f58849ea 100644 --- a/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs @@ -129,23 +129,6 @@ internal TracerProviderBuilder AddProcessor(BaseProcessor processor) return this; } - /// - /// Adds the ActivitySource of an instrumentation to the provider. - /// - /// ActivitySource to add. - /// Returns for chaining. - internal TracerProviderBuilder AddInstrumentationActivitySource(string instrumentationActivitySource) - { - if (string.IsNullOrEmpty(instrumentationActivitySource)) - { - throw new ArgumentException(nameof(instrumentationActivitySource)); - } - - this.sources.Add(instrumentationActivitySource); - - return this; - } - /// /// Adds the OperationName of an activity created by DiagnosticSource instrumentation to the provider. /// diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index cb422b3ec89..432744d665d 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -164,20 +164,25 @@ internal TracerProviderSdk( // Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to // or not. - listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name) || regex.IsMatch(activitySource.Name); + listener.ShouldListenTo = (activitySource) => + (legacyActivityOperationNames.Count > 0) ? + string.IsNullOrEmpty(activitySource.Name) || regex.IsMatch(activitySource.Name) : + regex.IsMatch(activitySource.Name); } else { - var activitySources = new Dictionary(StringComparer.OrdinalIgnoreCase) - { - { string.Empty, true }, - }; + var activitySources = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var name in sources) { activitySources[name] = true; } + if (legacyActivityOperationNames.Count > 0) + { + activitySources[string.Empty] = true; + } + // Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to // or not. listener.ShouldListenTo = (activitySource) => activitySources.ContainsKey(activitySource.Name); @@ -185,7 +190,10 @@ internal TracerProviderSdk( } else { - listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name); + if (legacyActivityOperationNames.Count > 0) + { + listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name); + } } ActivitySource.AddActivityListener(listener); From 999ca2bdb03cedbacce9f7c14753d4dc8b226c5f Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 17 Feb 2021 18:06:46 -0800 Subject: [PATCH 04/22] Updated TraceBenchmark --- .../netstandard2.0/PublicAPI.Unshipped.txt | 1 + .../Trace/ActivitySourceAdapterBenchmark.cs | 78 ------------------- test/Benchmarks/Trace/TraceBenchmarks.cs | 33 ++++++++ 3 files changed, 34 insertions(+), 78 deletions(-) delete mode 100644 test/Benchmarks/Trace/ActivitySourceAdapterBenchmark.cs diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index e69de29bb2d..f4e3d753088 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder \ No newline at end of file diff --git a/test/Benchmarks/Trace/ActivitySourceAdapterBenchmark.cs b/test/Benchmarks/Trace/ActivitySourceAdapterBenchmark.cs deleted file mode 100644 index 468d31ba57a..00000000000 --- a/test/Benchmarks/Trace/ActivitySourceAdapterBenchmark.cs +++ /dev/null @@ -1,78 +0,0 @@ -// -// 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. -// - -using System.Diagnostics; -using BenchmarkDotNet.Attributes; -using OpenTelemetry; -using OpenTelemetry.Trace; - -namespace Benchmarks.Trace -{ - [MemoryDiagnoser] - public class ActivitySourceAdapterBenchmark - { - private TestInstrumentation testInstrumentation = null; - private TracerProvider tracerProvider; - - [GlobalSetup] - public void GlobalSetup() - { - this.tracerProvider = Sdk.CreateTracerProviderBuilder() - .AddDiagnosticSourceInstrumentation((adapter) => - { - this.testInstrumentation = new TestInstrumentation(adapter); - return this.testInstrumentation; - }) - .Build(); - } - - [GlobalCleanup] - public void GlobalCleanup() - { - this.tracerProvider.Dispose(); - } - - [Benchmark] - public void ActivitySourceAdapterStartStop() - { - var activity = new Activity("test").Start(); - this.testInstrumentation.Start(activity); - this.testInstrumentation.Stop(activity); - activity.Stop(); - } - - private class TestInstrumentation - { - internal static ActivitySource ActivitySource = new ActivitySource("test", "1.0.0"); - private ActivitySourceAdapter adapter; - - public TestInstrumentation(ActivitySourceAdapter adapter) - { - this.adapter = adapter; - } - - public void Start(Activity activity) - { - this.adapter.Start(activity, ActivityKind.Internal, ActivitySource); - } - - public void Stop(Activity activity) - { - this.adapter.Stop(activity); - } - } - } -} diff --git a/test/Benchmarks/Trace/TraceBenchmarks.cs b/test/Benchmarks/Trace/TraceBenchmarks.cs index fa79c98d5b5..9b3fb5075f3 100644 --- a/test/Benchmarks/Trace/TraceBenchmarks.cs +++ b/test/Benchmarks/Trace/TraceBenchmarks.cs @@ -31,6 +31,8 @@ public class TraceBenchmarks private readonly ActivitySource sourceWithOneProcessor = new ActivitySource("Benchmark.OneProcessor"); private readonly ActivitySource sourceWithTwoProcessors = new ActivitySource("Benchmark.TwoProcessors"); private readonly ActivitySource sourceWithThreeProcessors = new ActivitySource("Benchmark.ThreeProcessors"); + private readonly ActivitySource sourceWithOneInstrumentation = new ActivitySource("Benchmark.OneInstrumentation"); + private readonly ActivitySource sourceWithTwoInstrumentations = new ActivitySource("Benchmark.TwoInstrumentations"); public TraceBenchmarks() { @@ -80,6 +82,21 @@ public TraceBenchmarks() .AddProcessor(new DummyActivityProcessor()) .AddProcessor(new DummyActivityProcessor()) .Build(); + + Sdk.CreateTracerProviderBuilder() + .SetSampler(new AlwaysOnSampler()) + .AddSource(this.sourceWithOneInstrumentation.Name) + .AddAspNetCoreInstrumentation() + .AddProcessor(new DummyActivityProcessor()) + .Build(); + + Sdk.CreateTracerProviderBuilder() + .SetSampler(new AlwaysOnSampler()) + .AddSource(this.sourceWithTwoInstrumentations.Name) + .AddAspNetCoreInstrumentation() + .AddHttpClientInstrumentation() + .AddProcessor(new DummyActivityProcessor()) + .Build(); } [Benchmark] @@ -142,6 +159,22 @@ public void ThreeProcessors() } } + [Benchmark] + public void OneInstrumentation() + { + using (var activity = this.sourceWithOneInstrumentation.StartActivity("Benchmark")) + { + } + } + + [Benchmark] + public void TwoInstrumentations() + { + using (var activity = this.sourceWithTwoInstrumentations.StartActivity("Benchmark")) + { + } + } + internal class DummyActivityProcessor : BaseProcessor { } From 6cd0cedf97751cfb11be02f3d6d3882c5a4d3f4c Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 17 Feb 2021 19:41:00 -0800 Subject: [PATCH 05/22] Update TracerProviderSdk --- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 26 +++++++------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index 432744d665d..2f2fd57d133 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -36,6 +36,7 @@ internal class TracerProviderSdk : TracerProvider private readonly HashSet legacyActivityOperationNames; private BaseProcessor processor; private Action getRequestedDataAction; + private bool needInstrumentations; internal TracerProviderSdk( Resource resource, @@ -48,6 +49,7 @@ internal TracerProviderSdk( this.Resource = resource; this.sampler = sampler; this.legacyActivityOperationNames = legacyActivityOperationNames; + this.needInstrumentations = legacyActivityOperationNames.Count > 0; foreach (var processor in processors) { @@ -69,21 +71,8 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStarted(activity); - if (legacyActivityOperationNames.Contains(activity.OperationName)) + if (this.needInstrumentations && legacyActivityOperationNames.Contains(activity.OperationName)) { - if (this.sampler is AlwaysOnSampler) - { - this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOnSampler; - } - else if (this.sampler is AlwaysOffSampler) - { - this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOffSampler; - } - else - { - this.getRequestedDataAction = this.RunGetRequestedDataOtherSampler; - } - this.getRequestedDataAction(activity); } @@ -127,17 +116,20 @@ internal TracerProviderSdk( { listener.Sample = (ref ActivityCreationOptions options) => !Sdk.SuppressInstrumentation ? ActivitySamplingResult.AllDataAndRecorded : ActivitySamplingResult.None; + this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOnSampler; } else if (sampler is AlwaysOffSampler) { listener.Sample = (ref ActivityCreationOptions options) => !Sdk.SuppressInstrumentation ? PropagateOrIgnoreData(options.Parent.TraceId) : ActivitySamplingResult.None; + this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOffSampler; } else { // This delegate informs ActivitySource about sampling decision when the parent context is an ActivityContext. listener.Sample = (ref ActivityCreationOptions options) => !Sdk.SuppressInstrumentation ? ComputeActivitySamplingResult(options, sampler) : ActivitySamplingResult.None; + this.getRequestedDataAction = this.RunGetRequestedDataOtherSampler; } if (sources.Any()) @@ -165,7 +157,7 @@ internal TracerProviderSdk( // Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to // or not. listener.ShouldListenTo = (activitySource) => - (legacyActivityOperationNames.Count > 0) ? + this.needInstrumentations ? string.IsNullOrEmpty(activitySource.Name) || regex.IsMatch(activitySource.Name) : regex.IsMatch(activitySource.Name); } @@ -178,7 +170,7 @@ internal TracerProviderSdk( activitySources[name] = true; } - if (legacyActivityOperationNames.Count > 0) + if (this.needInstrumentations) { activitySources[string.Empty] = true; } @@ -190,7 +182,7 @@ internal TracerProviderSdk( } else { - if (legacyActivityOperationNames.Count > 0) + if (this.needInstrumentations) { listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name); } From 9291faa593331b0cb43c1da12f53dac4f313f8e4 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Wed, 17 Feb 2021 22:57:28 -0800 Subject: [PATCH 06/22] Address PR comments --- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index 2f2fd57d133..6d6cd62dc3b 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -36,7 +36,7 @@ internal class TracerProviderSdk : TracerProvider private readonly HashSet legacyActivityOperationNames; private BaseProcessor processor; private Action getRequestedDataAction; - private bool needInstrumentations; + private bool supportLegacyActivity; internal TracerProviderSdk( Resource resource, @@ -49,7 +49,7 @@ internal TracerProviderSdk( this.Resource = resource; this.sampler = sampler; this.legacyActivityOperationNames = legacyActivityOperationNames; - this.needInstrumentations = legacyActivityOperationNames.Count > 0; + this.supportLegacyActivity = legacyActivityOperationNames.Count > 0; foreach (var processor in processors) { @@ -71,7 +71,7 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStarted(activity); - if (this.needInstrumentations && legacyActivityOperationNames.Contains(activity.OperationName)) + if (this.supportLegacyActivity && legacyActivityOperationNames.Contains(activity.OperationName) && string.IsNullOrEmpty(activity.Source.Name)) { this.getRequestedDataAction(activity); } @@ -157,7 +157,7 @@ internal TracerProviderSdk( // Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to // or not. listener.ShouldListenTo = (activitySource) => - this.needInstrumentations ? + this.supportLegacyActivity ? string.IsNullOrEmpty(activitySource.Name) || regex.IsMatch(activitySource.Name) : regex.IsMatch(activitySource.Name); } @@ -170,7 +170,7 @@ internal TracerProviderSdk( activitySources[name] = true; } - if (this.needInstrumentations) + if (this.supportLegacyActivity) { activitySources[string.Empty] = true; } @@ -182,7 +182,7 @@ internal TracerProviderSdk( } else { - if (this.needInstrumentations) + if (this.supportLegacyActivity) { listener.ShouldListenTo = (activitySource) => string.IsNullOrEmpty(activitySource.Name); } From 808a25be5a3eeea5990f3968709cf816c8e0a02f Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 18 Feb 2021 12:40:36 -0800 Subject: [PATCH 07/22] Address PR comments --- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 23 ++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index 6d6cd62dc3b..f6bb631b376 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -71,9 +71,18 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStarted(activity); - if (this.supportLegacyActivity && legacyActivityOperationNames.Contains(activity.OperationName) && string.IsNullOrEmpty(activity.Source.Name)) + if (this.supportLegacyActivity && string.IsNullOrEmpty(activity.Source.Name)) { - this.getRequestedDataAction(activity); + // We have a legacy activity in hand now and it's name matches the one user is interested in + if (legacyActivityOperationNames.Contains(activity.OperationName)) + { + this.getRequestedDataAction(activity); + } + else + { + // Legacy activity doesn't match the user configured list. No need to proceed further. + return; + } } if (!activity.IsAllDataRequested) @@ -92,6 +101,16 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStopped(activity); + if (this.supportLegacyActivity && string.IsNullOrEmpty(activity.Source.Name)) + { + // We have a legacy activity in hand now but not something user expressed interest in. + // Do not proceed any further. + if (!legacyActivityOperationNames.Contains(activity.OperationName)) + { + return; + } + } + if (!activity.IsAllDataRequested) { return; From 2a3fac7f0245262e17eda26344032d1f620d3bfd Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 18 Feb 2021 13:26:52 -0800 Subject: [PATCH 08/22] Update ASP.NET and gRPC client instrumentation to work without ActivitySourceAdapater --- .../AspNetInstrumentation.cs | 5 ++--- .../Implementation/HttpInListener.cs | 9 +++------ .../TracerProviderBuilderExtensions.cs | 4 +++- .../TracerProviderBuilderExtensions.cs | 1 - .../GrpcClientInstrumentation.cs | 10 ++-------- .../Implementation/GrpcClientDiagnosticListener.cs | 14 +++----------- .../TracerProviderBuilderExtensions.cs | 5 ++++- src/OpenTelemetry/Trace/TracerProviderSdk.cs | 1 - 8 files changed, 17 insertions(+), 32 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs index e2c16edf7e1..8e5fdc2a272 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/AspNetInstrumentation.cs @@ -31,12 +31,11 @@ internal class AspNetInstrumentation : IDisposable /// /// Initializes a new instance of the class. /// - /// ActivitySource adapter instance. /// Configuration options for ASP.NET instrumentation. - 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(); diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs index a9ce11dd19c..6ad16c0dc71 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs @@ -37,13 +37,11 @@ internal class HttpInListener : ListenerHandler private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route"); private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("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.")] @@ -111,7 +109,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) { @@ -244,8 +243,6 @@ public override void OnStopActivity(Activity activity, object payload) Activity.Current = activity; } } - - this.activitySource.Stop(activityToEnrich); } } } diff --git a/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs index 4c75a68c466..25e48119ee7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/TracerProviderBuilderExtensions.cs @@ -42,7 +42,9 @@ public static TracerProviderBuilder AddAspNetInstrumentation( 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); + builder.AddLegacyActivityOperationName("Microsoft.AspNet.HttpReqIn"); return builder; } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index 3046580d629..90826769ed7 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -15,7 +15,6 @@ // using System; -using System.Reflection; using OpenTelemetry.Instrumentation.AspNetCore; namespace OpenTelemetry.Trace diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentation.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentation.cs index b19b8d0a99c..2d442839b49 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentation.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentation.cs @@ -29,16 +29,10 @@ internal class GrpcClientInstrumentation : IDisposable /// /// Initializes a new instance of the class. /// - /// ActivitySource adapter instance. /// Configuration options for Grpc client instrumentation. - 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(); } diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs index 15db8f94d1b..c95912eff50 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs @@ -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 startRequestFetcher = new PropertyFetcher("Request"); private readonly PropertyFetcher stopRequestFetcher = new PropertyFetcher("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) @@ -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.Server); if (activity.IsAllDataRequested) { @@ -158,8 +152,6 @@ public override void OnStopActivity(Activity activity, object payload) } } } - - this.activitySource.Stop(activity); } } } diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/TracerProviderBuilderExtensions.cs index 5af86a7f41a..078a95f0ede 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/TracerProviderBuilderExtensions.cs @@ -43,7 +43,10 @@ public static TracerProviderBuilder AddGrpcClientInstrumentation( 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; } } diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index f6bb631b376..ec0c7afb40c 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -20,7 +20,6 @@ using System.Linq; using System.Runtime.CompilerServices; using System.Text.RegularExpressions; -using System.Threading; using OpenTelemetry.Internal; using OpenTelemetry.Resources; From 6d71638922b8f6ccc2687f0fe81ae0b0b329d1ab Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 18 Feb 2021 22:25:55 -0800 Subject: [PATCH 09/22] Remove ActivitySourceAdapter from ASP.NET and gRPC client instrumentations; Fixed unit tests; Added unit tests for testing legacy activity processing --- .../TracerProviderBuilderExtensions.cs | 3 +- .../GrpcClientDiagnosticListener.cs | 2 +- .../.publicApi/net452/PublicAPI.Unshipped.txt | 1 + .../.publicApi/net46/PublicAPI.Unshipped.txt | 1 + .../.publicApi/net461/PublicAPI.Unshipped.txt | 1 + src/OpenTelemetry/Trace/TracerProviderSdk.cs | 11 +- .../HttpInListenerTests.cs | 4 +- .../BasicTests.cs | 57 +++- .../HttpClientTests.Basic.netcore31.cs | 6 +- .../ActivityInstrumentationHelperTest.cs | 53 ++++ .../Trace/ActivitySourceAdapterTest.cs | 267 ------------------ .../Trace/TracerProviderSdkTest.cs | 222 ++++++++++++--- 12 files changed, 288 insertions(+), 340 deletions(-) create mode 100644 test/OpenTelemetry.Tests/Instrumentation/ActivityInstrumentationHelperTest.cs delete mode 100644 test/OpenTelemetry.Tests/Trace/ActivitySourceAdapterTest.cs diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs index 90826769ed7..59d5a403594 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs @@ -43,7 +43,8 @@ public static TracerProviderBuilder AddAspNetCoreInstrumentation( configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions); builder.AddInstrumentation(() => new AspNetCoreInstrumentation(aspnetCoreOptions)); builder.AddSource(typeof(AspNetCoreInstrumentation).Assembly.GetName().Name); - builder.AddLegacyActivityOperationName("Microsoft.AspNetCore.Hosting.HttpRequestIn"); + 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; } diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs index c95912eff50..d741cf05100 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs @@ -83,7 +83,7 @@ public override void OnStartActivity(Activity activity, object payload) activity.DisplayName = grpcMethod?.Trim('/'); ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource); - ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Server); + ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client); if (activity.IsAllDataRequested) { diff --git a/src/OpenTelemetry/.publicApi/net452/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net452/PublicAPI.Unshipped.txt index e69de29bb2d..e12fe668bbb 100644 --- a/src/OpenTelemetry/.publicApi/net452/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net452/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder diff --git a/src/OpenTelemetry/.publicApi/net46/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net46/PublicAPI.Unshipped.txt index e69de29bb2d..e12fe668bbb 100644 --- a/src/OpenTelemetry/.publicApi/net46/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net46/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder diff --git a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt index e69de29bb2d..e12fe668bbb 100644 --- a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddLegacyActivityOperationName(this OpenTelemetry.Trace.TracerProviderBuilder tracerProviderBuilder, string operationName) -> OpenTelemetry.Trace.TracerProviderBuilder diff --git a/src/OpenTelemetry/Trace/TracerProviderSdk.cs b/src/OpenTelemetry/Trace/TracerProviderSdk.cs index ec0c7afb40c..7db557f6007 100644 --- a/src/OpenTelemetry/Trace/TracerProviderSdk.cs +++ b/src/OpenTelemetry/Trace/TracerProviderSdk.cs @@ -100,14 +100,11 @@ internal TracerProviderSdk( { OpenTelemetrySdkEventSource.Log.ActivityStopped(activity); - if (this.supportLegacyActivity && string.IsNullOrEmpty(activity.Source.Name)) + if (string.IsNullOrEmpty(activity.Source.Name)) { - // We have a legacy activity in hand now but not something user expressed interest in. - // Do not proceed any further. - if (!legacyActivityOperationNames.Contains(activity.OperationName)) - { - return; - } + // We have a legacy activity which was not picked up by the instrumentation library. + // Most likely, the instrumentation library created a sibling activity and populated its ActivitySource correctly + return; } if (!activity.IsAllDataRequested) diff --git a/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs b/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs index a8c602f86fc..357788f5de9 100644 --- a/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs @@ -202,8 +202,8 @@ public void AspNetRequestsAreCollectedSuccessfully( if (HttpContext.Current.Request.Path == filter || filter == "{ThrowException}") { - // only SetParentProvider/Shutdown/Dispose are called because request was filtered. - Assert.Equal(3, activityProcessor.Invocations.Count); + // only SetParentProvider/Shutdown/Dispose/OnStart are called because request was filtered. + Assert.Equal(4, activityProcessor.Invocations.Count); return; } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 9671b6cc357..09d21ce9516 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -88,7 +88,10 @@ void ConfigureTestServices(IServiceCollection services) } Assert.Equal(3, activityProcessor.Invocations.Count); // begin and end was called - var activity = (Activity)activityProcessor.Invocations[2].Arguments[0]; + + // we should only call Processor.OnEnd for the "/api/values" request + Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); + var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; Assert.Equal(200, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode)); @@ -168,8 +171,15 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext() WaitForProcessorInvocations(activityProcessor, 3); } - Assert.Equal(3, activityProcessor.Invocations.Count); // begin and end was called - var activity = (Activity)activityProcessor.Invocations[2].Arguments[0]; + // List of invocations + // 1. SetParentProvider for TracerProviderSdk + // 2. OnStart for the activity created by AspNetCore with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 3. OnStart for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener + // 4. OnEnd for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener + + // we should only call Processor.OnEnd once for the sibling activity with the OperationName ActivityCreatedByHttpInListener + Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); + var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; #if !NETCOREAPP2_1 // ASP.NET Core after 2.x is W3C aware and hence Activity created by it @@ -221,12 +231,19 @@ public async Task CustomPropagator() var response = await client.GetAsync("/api/values/2"); response.EnsureSuccessStatusCode(); // Status Code 200-299 - WaitForProcessorInvocations(activityProcessor, 3); + WaitForProcessorInvocations(activityProcessor, 4); } - // begin and end was called once each. - Assert.Equal(3, activityProcessor.Invocations.Count); - var activity = (Activity)activityProcessor.Invocations[2].Arguments[0]; + // List of invocations on the processor + // 1. SetParentProvider for TracerProviderSdk + // 2. OnStart for the activity created by AspNetCore with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 3. OnStart for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener + // 4. OnEnd for the sibling activity created by the instrumentation library with the OperationName: ActivityCreatedByHttpInListener + Assert.Equal(4, activityProcessor.Invocations.Count); + + // we should only call Processor.OnEnd once for the sibling activity with the OperationName ActivityCreatedByHttpInListener + Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); + var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; Assert.Equal("ActivityCreatedByHttpInListener", activity.OperationName); Assert.Equal(ActivityKind.Server, activity.Kind); @@ -271,12 +288,18 @@ void ConfigureTestServices(IServiceCollection services) response1.EnsureSuccessStatusCode(); // Status Code 200-299 response2.EnsureSuccessStatusCode(); // Status Code 200-299 - WaitForProcessorInvocations(activityProcessor, 3); + WaitForProcessorInvocations(activityProcessor, 4); } - // we should only create one span and never call processor with another - Assert.Equal(3, activityProcessor.Invocations.Count); // begin and end was called - var activity = (Activity)activityProcessor.Invocations[2].Arguments[0]; + // 1. SetParentProvider for TracerProviderSdk + // 2. OnStart for the activity created by AspNetCore for "/api/values" with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 3. OnEnd for the activity created by AspNetCore for "/api/values" with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 4. OnStart for the activity created by AspNetCore for "/api/values/2" with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + Assert.Equal(4, activityProcessor.Invocations.Count); + + // we should only call Processor.OnEnd for the "/api/values" request + Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); + var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; Assert.Equal(ActivityKind.Server, activity.Kind); Assert.Equal("/api/values", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); @@ -328,8 +351,16 @@ void ConfigureTestServices(IServiceCollection services) // As InstrumentationFilter threw, we continue as if the // InstrumentationFilter did not exist. - Assert.Equal(3, activityProcessor.Invocations.Count); // begin and end was called - var activity = (Activity)activityProcessor.Invocations[2].Arguments[0]; + + // List of invocations on the processor + // 1. SetParentProvider for TracerProviderSdk + // 2. OnStart for the activity created by AspNetCore for "/api/values" with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 3. OnEnd for the activity created by AspNetCore for "/api/values" with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + // 4. OnStart for the activity created by AspNetCore for "/api/values/2" with the OperationName: Microsoft.AspNetCore.Hosting.HttpRequestIn + + // we should only call Processor.OnEnd for the "/api/values" request + Assert.Single(activityProcessor.Invocations, invo => invo.Method.Name == "OnEnd"); + var activity = activityProcessor.Invocations.FirstOrDefault(invo => invo.Method.Name == "OnEnd").Arguments[0] as Activity; Assert.Equal(ActivityKind.Server, activity.Kind); Assert.Equal("/api/values", activity.GetTagValue(SpanAttributeConstants.HttpPathKey) as string); diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs index 3b216bb91d3..4cc67baecea 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.netcore31.cs @@ -232,7 +232,7 @@ public async Task HttpClientInstrumentationBacksOffIfAlreadyInstrumented() await c.SendAsync(request); } - Assert.Equal(3, processor.Invocations.Count); // SetParentProvider/OnShutdown/Dispose called. + Assert.Equal(4, processor.Invocations.Count); // SetParentProvider/OnShutdown/Dispose/OnStart called. } [Fact] @@ -249,7 +249,7 @@ public async void RequestNotCollectedWhenInstrumentationFilterApplied() await c.GetAsync(this.url); } - Assert.Equal(3, processor.Invocations.Count); // SetParentProvider/OnShutdown/Dispose called. + Assert.Equal(4, processor.Invocations.Count); // SetParentProvider/OnShutdown/Dispose/OnStart called. } [Fact] @@ -270,7 +270,7 @@ public async void RequestNotCollectedWhenInstrumentationFilterThrowsException() } } - Assert.Equal(3, processor.Invocations.Count); // SetParentProvider/OnShutdown/Dispose called. + Assert.Equal(4, processor.Invocations.Count); // SetParentProvider/OnShutdown/Dispose/OnStart called. } [Fact] diff --git a/test/OpenTelemetry.Tests/Instrumentation/ActivityInstrumentationHelperTest.cs b/test/OpenTelemetry.Tests/Instrumentation/ActivityInstrumentationHelperTest.cs new file mode 100644 index 00000000000..c7bf6782265 --- /dev/null +++ b/test/OpenTelemetry.Tests/Instrumentation/ActivityInstrumentationHelperTest.cs @@ -0,0 +1,53 @@ +// +// 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. +// + +using System.Diagnostics; +using Xunit; + +namespace OpenTelemetry.Instrumentation.Tests +{ + public class ActivityInstrumentationHelperTest + { + [Theory] + [InlineData("TestActivitySource", null)] + [InlineData("TestActivitySource", "1.0.0")] + public void SetActivitySource(string name, string version) + { + var activity = new Activity("Test"); + var activitySource = new ActivitySource(name, version); + + activity.Start(); + ActivityInstrumentationHelper.SetActivitySourceProperty(activity, activitySource); + Assert.Equal(activitySource.Name, activity.Source.Name); + Assert.Equal(activitySource.Version, activity.Source.Version); + activity.Stop(); + } + + [Theory] + [InlineData(ActivityKind.Client)] + [InlineData(ActivityKind.Consumer)] + [InlineData(ActivityKind.Internal)] + [InlineData(ActivityKind.Producer)] + [InlineData(ActivityKind.Server)] + public void SetActivityKind(ActivityKind activityKind) + { + var activity = new Activity("Test"); + activity.Start(); + ActivityInstrumentationHelper.SetKindProperty(activity, activityKind); + Assert.Equal(activityKind, activity.Kind); + } + } +} diff --git a/test/OpenTelemetry.Tests/Trace/ActivitySourceAdapterTest.cs b/test/OpenTelemetry.Tests/Trace/ActivitySourceAdapterTest.cs deleted file mode 100644 index 87f52e15a63..00000000000 --- a/test/OpenTelemetry.Tests/Trace/ActivitySourceAdapterTest.cs +++ /dev/null @@ -1,267 +0,0 @@ -// -// 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. -// - -using System; -using System.Collections.Generic; -using System.Diagnostics; -using OpenTelemetry.Tests; -using Xunit; - -namespace OpenTelemetry.Trace.Tests -{ - public class ActivitySourceAdapterTest : IDisposable - { - private TestSampler testSampler; - private TestActivityProcessor testProcessor; - private ActivitySourceAdapter activitySourceAdapter; - - static ActivitySourceAdapterTest() - { - Activity.DefaultIdFormat = ActivityIdFormat.W3C; - Activity.ForceDefaultIdFormat = true; - } - - public ActivitySourceAdapterTest() - { - this.testSampler = new TestSampler(); - this.testProcessor = new TestActivityProcessor(); - this.activitySourceAdapter = new ActivitySourceAdapter(this.testSampler, this.testProcessor); - } - - [Fact] - public void ActivitySourceAdapterValidatesConstructor() - { - // Sampler null - Assert.Throws(() => new ActivitySourceAdapter(null, this.testProcessor)); - - // Processor null. This is not expected to throw as processor can - // be null and can be later added. - var adapter = new ActivitySourceAdapter(this.testSampler, null); - } - - [Theory] - [InlineData(ActivityKind.Client)] - [InlineData(ActivityKind.Consumer)] - [InlineData(ActivityKind.Internal)] - [InlineData(ActivityKind.Producer)] - [InlineData(ActivityKind.Server)] - public void ActivitySourceAdapterSetsKind(ActivityKind kind) - { - var activity = new Activity("test"); - activity.Start(); - this.activitySourceAdapter.Start(activity, kind, new ActivitySource("test", "1.0.0")); - - Assert.Equal(kind, activity.Kind); - } - - [Theory] - [InlineData(SamplingDecision.Drop)] - [InlineData(SamplingDecision.RecordOnly)] - [InlineData(SamplingDecision.RecordAndSample)] - public void ActivitySourceAdapterCallsStartStopActivityProcessor1(SamplingDecision decision) - { - this.testSampler.SamplingAction = (samplingParameters) => - { - return new SamplingResult(decision); - }; - - bool startCalled = false; - bool endCalled = false; - this.testProcessor.StartAction = - (a) => - { - startCalled = true; - - // If start is called, that means activity is sampled, - // and TraceFlag is set to Recorded. - Assert.Equal(decision == SamplingDecision.RecordOnly || decision == SamplingDecision.RecordAndSample, a.IsAllDataRequested); - Assert.Equal(decision == SamplingDecision.RecordAndSample ? ActivityTraceFlags.Recorded : ActivityTraceFlags.None, a.ActivityTraceFlags); - Assert.Equal(decision == SamplingDecision.RecordAndSample, a.Recorded); - }; - - this.testProcessor.EndAction = - (a) => - { - endCalled = true; - }; - - var activity = new Activity("test"); - activity.Start(); - this.activitySourceAdapter.Start(activity, ActivityKind.Producer, new ActivitySource("test", "1.0.0")); - activity.Stop(); - this.activitySourceAdapter.Stop(activity); - - Assert.Equal(ActivityKind.Producer, activity.Kind); - Assert.Equal(activity.IsAllDataRequested, startCalled); - Assert.Equal(activity.IsAllDataRequested, endCalled); - } - - [Theory] - [InlineData(true)] - [InlineData(false)] - public void ActivitySourceAdapterCallsStartStopActivityProcessor2(bool isSampled) - { - this.testSampler.SamplingAction = (samplingParameters) => - { - return new SamplingResult(isSampled); - }; - - bool startCalled = false; - bool endCalled = false; - this.testProcessor.StartAction = - (a) => - { - startCalled = true; - - // If start is called, that means activity is sampled, - // and TraceFlag is set to Recorded. - Assert.Equal(isSampled, a.IsAllDataRequested); - Assert.Equal(isSampled ? ActivityTraceFlags.Recorded : ActivityTraceFlags.None, a.ActivityTraceFlags); - Assert.Equal(isSampled, a.Recorded); - }; - - this.testProcessor.EndAction = - (a) => - { - endCalled = true; - }; - - var activity = new Activity("test"); - activity.Start(); - this.activitySourceAdapter.Start(activity, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - activity.Stop(); - this.activitySourceAdapter.Stop(activity); - - Assert.Equal(isSampled, startCalled); - Assert.Equal(isSampled, endCalled); - } - - [Theory] - [InlineData(SamplingDecision.Drop)] - [InlineData(SamplingDecision.RecordOnly)] - [InlineData(SamplingDecision.RecordAndSample)] - public void ActivitySourceAdapterPopulatesSamplingAttributesToActivity(SamplingDecision sampling) - { - this.testSampler.SamplingAction = (samplingParams) => - { - var attributes = new Dictionary(); - attributes.Add("tagkeybysampler", "tagvalueaddedbysampler"); - return new SamplingResult(sampling, attributes); - }; - - var activity = new Activity("test"); - activity.Start(); - this.activitySourceAdapter.Start(activity, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - if (sampling != SamplingDecision.Drop) - { - Assert.Contains(new KeyValuePair("tagkeybysampler", "tagvalueaddedbysampler"), activity.TagObjects); - } - - activity.Stop(); - } - - [Fact] - public void ActivitySourceAdapterPopulatesSamplingParamsCorrectlyForRootActivity() - { - this.testSampler.SamplingAction = (samplingParameters) => - { - Assert.Equal(default, samplingParameters.ParentContext); - return new SamplingResult(SamplingDecision.RecordAndSample); - }; - - // Start activity without setting parent. i.e it'll have null parent - // and becomes root activity - var activity = new Activity("test"); - activity.Start(); - this.activitySourceAdapter.Start(activity, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - activity.Stop(); - this.activitySourceAdapter.Stop(activity); - } - - [Theory] - [InlineData(ActivityTraceFlags.None)] - [InlineData(ActivityTraceFlags.Recorded)] - public void ActivitySourceAdapterPopulatesSamplingParamsCorrectlyForActivityWithRemoteParent(ActivityTraceFlags traceFlags) - { - var parentTraceId = ActivityTraceId.CreateRandom(); - var parentSpanId = ActivitySpanId.CreateRandom(); - var parentTraceFlag = (traceFlags == ActivityTraceFlags.Recorded) ? "01" : "00"; - string remoteParentId = $"00-{parentTraceId}-{parentSpanId}-{parentTraceFlag}"; - string tracestate = "a=b;c=d"; - - this.testSampler.SamplingAction = (samplingParameters) => - { - Assert.Equal(parentTraceId, samplingParameters.ParentContext.TraceId); - Assert.Equal(parentSpanId, samplingParameters.ParentContext.SpanId); - Assert.Equal(traceFlags, samplingParameters.ParentContext.TraceFlags); - Assert.Equal(tracestate, samplingParameters.ParentContext.TraceState); - return new SamplingResult(SamplingDecision.RecordAndSample); - }; - - // Create an activity with remote parent id. - // The sampling parameters are expected to be that of the - // parent context i.e the remote parent. - var activity = new Activity("test").SetParentId(remoteParentId); - activity.TraceStateString = tracestate; - activity.Start(); - this.activitySourceAdapter.Start(activity, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - activity.Stop(); - this.activitySourceAdapter.Stop(activity); - } - - [Theory] - [InlineData(ActivityTraceFlags.None)] - [InlineData(ActivityTraceFlags.Recorded)] - public void ActivitySourceAdapterPopulatesSamplingParamsCorrectlyForActivityWithInProcParent(ActivityTraceFlags traceFlags) - { - // Create some parent activity. - string tracestate = "a=b;c=d"; - var activityLocalParent = new Activity("testParent"); - activityLocalParent.ActivityTraceFlags = traceFlags; - activityLocalParent.TraceStateString = tracestate; - activityLocalParent.Start(); - - this.testSampler.SamplingAction = (samplingParameters) => - { - Assert.Equal(activityLocalParent.TraceId, samplingParameters.ParentContext.TraceId); - Assert.Equal(activityLocalParent.SpanId, samplingParameters.ParentContext.SpanId); - Assert.Equal(activityLocalParent.ActivityTraceFlags, samplingParameters.ParentContext.TraceFlags); - Assert.Equal(tracestate, samplingParameters.ParentContext.TraceState); - Assert.Equal(ActivityKind.Client, samplingParameters.Kind); - return new SamplingResult(SamplingDecision.RecordAndSample); - }; - - // This activity will have a inproc parent. - // activity.Parent will be equal to the activity created at the beginning of this test. - // Sampling parameters are expected to be that of the parentContext. - // i.e of the parent Activity - var activity = new Activity("test"); - activity.Start(); - this.activitySourceAdapter.Start(activity, ActivityKind.Client, new ActivitySource("test", "1.0.0")); - activity.Stop(); - this.activitySourceAdapter.Stop(activity); - - activityLocalParent.Stop(); - } - - public void Dispose() - { - Activity.Current = null; - this.testProcessor.Dispose(); - GC.SuppressFinalize(this); - } - } -} diff --git a/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs b/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs index e193cb9b5b6..b091e787d26 100644 --- a/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs +++ b/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs @@ -18,6 +18,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using OpenTelemetry.Instrumentation; using OpenTelemetry.Resources; using OpenTelemetry.Tests; using Xunit; @@ -252,8 +253,10 @@ public void ProcessorDoesNotReceiveNotRecordDecisionSpan() Assert.False(endCalled); } + // Test to check that TracerProvider does not call Processor.Start or Processor.End for a legacy activity when no legacy OperationName is + // provided to TracerProviderBuilder. [Fact] - public void TracerProvideSdkCreatesActivitySource() + public void SdkDoesNotProcessLegacyActivityWithNoAdditionalConfig() { using TestActivityProcessor testActivityProcessor = new TestActivityProcessor(); @@ -272,21 +275,175 @@ public void TracerProvideSdkCreatesActivitySource() endCalled = true; }; - TestInstrumentation testInstrumentation = null; + // No AddLegacyOperationName chained to TracerProviderBuilder using var tracerProvider = Sdk.CreateTracerProviderBuilder() .AddProcessor(testActivityProcessor) - .AddDiagnosticSourceInstrumentation((adapter) => - { - testInstrumentation = new TestInstrumentation(adapter); - return testInstrumentation; - }) .Build(); - var adapter = testInstrumentation.Adapter; - Activity activity = new Activity("test"); + Activity activity = new Activity("Test"); + activity.Start(); + activity.Stop(); + + Assert.False(startCalled); + Assert.False(endCalled); + } + + // Test to check that TracerProvider does not call Processor.End for a legacy activity which did not have its ActivitySource updated before Activity.Stop + [Fact] + public void SdkProcessLegacyActivityWithNoUpdateToActivitySource() + { + using TestActivityProcessor testActivityProcessor = new TestActivityProcessor(); + + bool startCalled = false; + bool endCalled = false; + + testActivityProcessor.StartAction = + (a) => + { + startCalled = true; + }; + + testActivityProcessor.EndAction = + (a) => + { + endCalled = true; + }; + + var operationNameForLegacyActivity = "TestOperationName"; + + // Only AddLegacyActivityOperationName is chained which ensures that a legacy activity with empty ActivitySource is sent to Processor.OnStart. + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddLegacyActivityOperationName(operationNameForLegacyActivity) + .AddProcessor(testActivityProcessor) + .Build(); + + Activity activity = new Activity(operationNameForLegacyActivity); + activity.Start(); + + // The legacy activity's ActivitySource is not set before activity.Stop. The activity will not be sent to Processor.OnEnd. + activity.Stop(); + + Assert.True(startCalled); + Assert.False(endCalled); + } + + // Test to check that TracerProvider does not call Processor.End for a legacy activity which had its ActivitySource updated but the + // updated ActivitySource was not added to the TracerProvider + [Fact] + public void SdkProcessLegacyActivityWithPartialConfig() + { + using TestActivityProcessor testActivityProcessor = new TestActivityProcessor(); + + bool startCalled = false; + bool endCalled = false; + + testActivityProcessor.StartAction = + (a) => + { + startCalled = true; + }; + + testActivityProcessor.EndAction = + (a) => + { + endCalled = true; + }; + + var activitySourceForLegacyActvity = new ActivitySource("TestActivitySource", "1.0.0"); + var operationNameForLegacyActivity = "TestOperationName"; + + // Only AddLegacyActivityOperationName is chained which ensures that a legacy activity with empty ActivitySource is sent to Processor.OnStart. + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddLegacyActivityOperationName(operationNameForLegacyActivity) + .AddProcessor(testActivityProcessor) + .Build(); + + Activity activity = new Activity(operationNameForLegacyActivity); + activity.Start(); + + // The legacy activity's ActivitySource is not set before activity.Stop. The activity should not be sent to Processor.OnEnd. + activity.Stop(); + + Assert.True(startCalled); + Assert.False(endCalled); + } + + // Test to check that TracerProvider processes legacy activities when a legacy OperationName and the required Source is provided to the TracerProvider + [Fact] + public void SdkProcessesLegacyActivityWithRightConfig() + { + using TestActivityProcessor testActivityProcessor = new TestActivityProcessor(); + + bool startCalled = false; + bool endCalled = false; + + testActivityProcessor.StartAction = + (a) => + { + startCalled = true; + }; + + testActivityProcessor.EndAction = + (a) => + { + endCalled = true; + }; + + var activitySourceForLegacyActvity = new ActivitySource("TestActivitySource", "1.0.0"); + var operationNameForLegacyActivity = "TestOperationName"; + + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddSource(activitySourceForLegacyActvity.Name) + .AddLegacyActivityOperationName(operationNameForLegacyActivity) + .AddProcessor(testActivityProcessor) + .Build(); + + Activity activity = new Activity(operationNameForLegacyActivity); + activity.Start(); + + // Set legacy activity's ActvitySource before Activity.Stop + ActivityInstrumentationHelper.SetActivitySourceProperty(activity, activitySourceForLegacyActvity); + activity.Stop(); + + Assert.True(startCalled); + Assert.True(endCalled); + } + + // Test to check that TracerProvider continues to process legacy activities even after a new Processor is added after the building the provider. + [Fact] + public void SdkProcessesLegacyActivityAfterAddingNewProcessor() + { + using TestActivityProcessor testActivityProcessor = new TestActivityProcessor(); + + bool startCalled = false; + bool endCalled = false; + + testActivityProcessor.StartAction = + (a) => + { + startCalled = true; + }; + + testActivityProcessor.EndAction = + (a) => + { + endCalled = true; + }; + + var activitySourceForLegacyActvity = new ActivitySource("TestActivitySource", "1.0.0"); + var operationNameForLegacyActivity = "TestOperationName"; + + using var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddSource(activitySourceForLegacyActvity.Name) + .AddLegacyActivityOperationName(operationNameForLegacyActivity) + .AddProcessor(testActivityProcessor) + .Build(); + + Activity activity = new Activity(operationNameForLegacyActivity); activity.Start(); - adapter.Start(activity, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - adapter.Stop(activity); + + // Set legacy activity's ActvitySource + ActivityInstrumentationHelper.SetActivitySourceProperty(activity, activitySourceForLegacyActvity); activity.Stop(); Assert.True(startCalled); @@ -313,56 +470,31 @@ public void TracerProvideSdkCreatesActivitySource() }; tracerProvider.AddProcessor(testActivityProcessorNew); - Activity activityNew = new Activity("test"); + + Activity activityNew = new Activity(operationNameForLegacyActivity); // Create a new Activity with the same operation name activityNew.Start(); - adapter.Start(activityNew, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - adapter.Stop(activityNew); + + // Set new legacy activity's ActvitySource + ActivityInstrumentationHelper.SetActivitySourceProperty(activityNew, activitySourceForLegacyActvity); activityNew.Stop(); Assert.True(startCalledNew); Assert.True(endCalledNew); } - [Fact] - public void TracerProvideSdkCreatesActivitySourceWhenNoProcessor() - { - TestInstrumentation testInstrumentation = null; - using var tracerProvider = Sdk.CreateTracerProviderBuilder() - .AddDiagnosticSourceInstrumentation((adapter) => - { - testInstrumentation = new TestInstrumentation(adapter); - return testInstrumentation; - }) - .Build(); - - var adapter = testInstrumentation.Adapter; - Activity activity = new Activity("test"); - activity.Start(); - adapter.Start(activity, ActivityKind.Internal, new ActivitySource("test", "1.0.0")); - adapter.Stop(activity); - activity.Stop(); - - // No asserts here. Validates that no exception - // gets thrown when processors are not added, - // TODO: Refactor to have more proper unit test - // to target each individual classes. - } - [Fact] public void TracerProvideSdkCreatesAndDiposesInstrumentation() { TestInstrumentation testInstrumentation = null; var tracerProvider = Sdk.CreateTracerProviderBuilder() - .AddDiagnosticSourceInstrumentation((adapter) => + .AddInstrumentation(() => { - testInstrumentation = new TestInstrumentation(adapter); + testInstrumentation = new TestInstrumentation(); return testInstrumentation; }) .Build(); Assert.NotNull(testInstrumentation); - var adapter = testInstrumentation.Adapter; - Assert.NotNull(adapter); Assert.False(testInstrumentation.IsDisposed); tracerProvider.Dispose(); Assert.True(testInstrumentation.IsDisposed); @@ -406,11 +538,9 @@ public void Dispose() private class TestInstrumentation : IDisposable { public bool IsDisposed; - public ActivitySourceAdapter Adapter; - public TestInstrumentation(ActivitySourceAdapter adapter) + public TestInstrumentation() { - this.Adapter = adapter; this.IsDisposed = false; } From 1f32aa6f5ac4a6f7b06245e04a8b8b5f7e388f9f Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 18 Feb 2021 22:29:00 -0800 Subject: [PATCH 10/22] Remove the incorrectly added file --- .../getting-started/ActivitySourceDemo.cs | 26 ------------------- .../getting-started/getting-started.csproj | 2 +- 2 files changed, 1 insertion(+), 27 deletions(-) delete mode 100644 docs/trace/getting-started/ActivitySourceDemo.cs diff --git a/docs/trace/getting-started/ActivitySourceDemo.cs b/docs/trace/getting-started/ActivitySourceDemo.cs deleted file mode 100644 index 432118f819e..00000000000 --- a/docs/trace/getting-started/ActivitySourceDemo.cs +++ /dev/null @@ -1,26 +0,0 @@ -// -// 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. -// - -using System.Diagnostics; - -namespace Demo -{ - internal static class ActivitySourceDemo - { - internal static readonly ActivitySource MyActivitySource = new ActivitySource( - "MyCompany.MyProduct.MyLibrary"); - } -} diff --git a/docs/trace/getting-started/getting-started.csproj b/docs/trace/getting-started/getting-started.csproj index 2e58890af03..afb37b73c59 100644 --- a/docs/trace/getting-started/getting-started.csproj +++ b/docs/trace/getting-started/getting-started.csproj @@ -1,4 +1,4 @@ - +