Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrumentation Adapters to support Activity API #701

Merged
merged 33 commits into from
Jun 11, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
831d712
use the same activity created by existing instrumentation. Simply enh…
cijothomas May 26, 2020
737f9b6
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 1, 2020
9b840f0
remove sampling from instrumentation
cijothomas Jun 2, 2020
036a9bb
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 2, 2020
5b836a4
Merge branch 'cijothomas/activity3adapter1a_alternate' of https://git…
cijothomas Jun 2, 2020
382d85f
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 2, 2020
f1e5108
add asp.net core instrumentation
cijothomas Jun 2, 2020
170ad87
Merge branch 'cijothomas/activity3adapter1a_alternate' of https://git…
cijothomas Jun 2, 2020
0832e71
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 2, 2020
3b4cbb7
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 2, 2020
19d138f
move sampling to Instrumentation for now.
cijothomas Jun 4, 2020
121e4b5
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 4, 2020
823fee6
Add httpClient .net core instrumentation
cijothomas Jun 4, 2020
c759d8a
Ad SqlClientInstrumentation
cijothomas Jun 4, 2020
6df93de
remove sqlclientinstrumentation from previous instrumentation
cijothomas Jun 4, 2020
0d07263
Quick implementation for AzureClients - not validated as there are un…
cijothomas Jun 5, 2020
692b0dc
fix examples
cijothomas Jun 5, 2020
7185426
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 5, 2020
37071c5
fix sampling flag
cijothomas Jun 8, 2020
b889a8c
Merge branch 'cijothomas/activity3adapter1a_alternate' of https://git…
cijothomas Jun 8, 2020
8630782
made sample app work with jaeger
cijothomas Jun 8, 2020
c4834ae
Mark todos and fix AspNet tests
cijothomas Jun 8, 2020
581cd1e
Fix asp.net core tests and mark TODOs
cijothomas Jun 8, 2020
7341c59
Add TODO for httpclient .net core test and fix test
cijothomas Jun 8, 2020
d9c9fff
add todo and fix httpclient test
cijothomas Jun 9, 2020
514d89c
add todos and fil sqlclienttests
cijothomas Jun 9, 2020
f939361
Make OpenTelemetrySDK disposable and take care of disposing all ds su…
cijothomas Jun 9, 2020
a220d9e
Added OpenTelemetry.Default instead of static method
cijothomas Jun 9, 2020
99b91bf
Merge branch 'master' into cijothomas/activity3adapter1a_alternate
cijothomas Jun 9, 2020
63dc16f
conflict resolve
cijothomas Jun 9, 2020
11a175a
AspNet, AspNetCore fix Dispose issue
cijothomas Jun 9, 2020
efdb1d3
conflict
cijothomas Jun 10, 2020
3646fa1
stylecop stuff lost i merge
cijothomas Jun 10, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions samples/Exporters/Web/OpenTelemetry.Exporter.Web.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\..\src\OpenTelemetry.Exporter.Console\OpenTelemetry.Exporter.Console.csproj" />
<ProjectReference Include="..\..\..\src\OpenTelemetry.Instrumentation.AspNetCore\OpenTelemetry.Instrumentation.AspNetCore.csproj" />
<ProjectReference Include="..\..\..\src\OpenTelemetry.Instrumentation.Dependencies\OpenTelemetry.Instrumentation.Dependencies.csproj" />
<ProjectReference Include="..\..\..\src\OpenTelemetry.Exporter.Zipkin\OpenTelemetry.Exporter.Zipkin.csproj" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I will create the Activity exporter for Zipkin this week or the next.

Expand Down
6 changes: 5 additions & 1 deletion samples/Exporters/Web/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.OpenApi.Models;
using OpenTelemetry.Exporter.Console;
using OpenTelemetry.Trace.Configuration;

namespace API
Expand Down Expand Up @@ -36,6 +37,10 @@ public void ConfigureServices(IServiceCollection services)
}
});

OpenTelemetrySdk.EnableOpenTelemetry(
(builder) => builder.AddRequestInstrumentation()
.UseConsoleActivityExporter(opt => opt.DisplayAsJson = opt.DisplayAsJson));

services.AddOpenTelemetry((sp, builder) =>
{
builder
Expand All @@ -45,7 +50,6 @@ public void ConfigureServices(IServiceCollection services)
options.ServiceName = "test-zipkin";
options.Endpoint = new Uri(this.Configuration.GetValue<string>("Zipkin:Endpoint"));
})
.AddRequestInstrumentation()
.AddDependencyInstrumentation();
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
// </copyright>
using System;
using OpenTelemetry.Instrumentation.AspNetCore.Implementation;
using OpenTelemetry.Trace;

namespace OpenTelemetry.Instrumentation.AspNetCore
{
Expand All @@ -29,20 +28,18 @@ public class AspNetCoreInstrumentation : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="AspNetCoreInstrumentation"/> class.
/// </summary>
/// <param name="tracer">Tracer to record traced with.</param>
public AspNetCoreInstrumentation(Tracer tracer)
: this(tracer, new AspNetCoreInstrumentationOptions())
public AspNetCoreInstrumentation()
: this(new AspNetCoreInstrumentationOptions())
{
}

/// <summary>
/// Initializes a new instance of the <see cref="AspNetCoreInstrumentation"/> class.
/// </summary>
/// <param name="tracer">Tracer to record traced with.</param>
/// <param name="options">Configuration options for ASP.NET Core instrumentation.</param>
public AspNetCoreInstrumentation(Tracer tracer, AspNetCoreInstrumentationOptions options)
public AspNetCoreInstrumentation(AspNetCoreInstrumentationOptions options)
{
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", tracer, options), null);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInListener("Microsoft.AspNetCore", options), null);
this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ internal class HttpInListener : ListenerHandler
private readonly bool hostingSupportsW3C = false;
private readonly AspNetCoreInstrumentationOptions options;

public HttpInListener(string name, Tracer tracer, AspNetCoreInstrumentationOptions options)
: base(name, tracer)
public HttpInListener(string name, AspNetCoreInstrumentationOptions options)
: base(name, null)
{
this.hostingSupportsW3C = typeof(HttpRequest).Assembly.GetName().Version.Major >= 3;
this.options = options ?? throw new ArgumentNullException(nameof(options));
Expand All @@ -60,50 +60,57 @@ public override void OnStartActivity(Activity activity, object payload)
return;
}

var request = context.Request;
// TODO: the line below once .NET ships new Activity
// Or do reflection now.
// activity.ActivityKind = ActivityKind.Server

// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md
var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
var request = context.Request;

TelemetrySpan span;
if (this.hostingSupportsW3C && this.options.TextFormat is TraceContextFormat)
{
this.Tracer.StartActiveSpanFromActivity(path, Activity.Current, SpanKind.Server, out span);
}
else
if (!this.hostingSupportsW3C || !(this.options.TextFormat is TraceContextFormat))
{
// This requires to ignore the current activity and create a new one
// using the context extracted from w3ctraceprent header or
// using the format TextFormat supports.
// TODO: actually implement code doing the above.

/*
var ctx = this.options.TextFormat.Extract<HttpRequest>(
request,
(r, name) => r.Headers[name]);

this.Tracer.StartActiveSpan(path, ctx, SpanKind.Server, out span);
*/
}

if (span.IsRecording)
if (activity.IsAllDataRequested)
{
// Note, route is missing at this stage. It will be available later
span.PutHttpHostAttribute(request.Host.Host, request.Host.Port ?? 80);
span.PutHttpMethodAttribute(request.Method);
span.PutHttpPathAttribute(path);
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md
var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
activity.DisplayName = path;

if (request.Host.Port == 80 || request.Host.Port == 443)
{
activity.AddTag("http.host", request.Host.Host);
}
else
{
activity.AddTag("http.host", request.Host.Host + ":" + request.Host.Port);
}

activity.AddTag("http.method", request.Method);
activity.AddTag("http.path", path);

var userAgent = request.Headers["User-Agent"].FirstOrDefault();
span.PutHttpUserAgentAttribute(userAgent);
span.PutHttpRawUrlAttribute(GetUri(request));
activity.AddTag("http.user_agent", userAgent);
activity.AddTag("http.url", GetUri(request));
}
}

public override void OnStopActivity(Activity activity, object payload)
{
const string EventNameSuffix = ".OnStopActivity";
var span = this.Tracer.CurrentSpan;

if (span == null || !span.Context.IsValid)
{
InstrumentationEventSource.Log.NullOrBlankSpan(nameof(HttpInListener) + EventNameSuffix);
return;
}

if (span.IsRecording)
if (activity.IsAllDataRequested)
{
if (!(this.stopContextFetcher.Fetch(payload) is HttpContext context))
{
Expand All @@ -112,26 +119,19 @@ public override void OnStopActivity(Activity activity, object payload)
}

var response = context.Response;
activity.AddTag(SpanAttributeConstants.HttpStatusCodeKey, response.StatusCode.ToString());

span.PutHttpStatusCode(response.StatusCode, response.HttpContext.Features.Get<IHttpResponseFeature>().ReasonPhrase);
Status status = SpanHelper.ResolveSpanStatusForHttpStatusCode((int)response.StatusCode);
activity.AddTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(status.CanonicalCode));
activity.AddTag(SpanAttributeConstants.StatusDescriptionKey, response.HttpContext.Features.Get<IHttpResponseFeature>().ReasonPhrase);
}

span.End();
}

public override void OnCustom(string name, Activity activity, object payload)
{
if (name == "Microsoft.AspNetCore.Mvc.BeforeAction")
{
var span = this.Tracer.CurrentSpan;

if (span == null)
{
InstrumentationEventSource.Log.NullOrBlankSpan(name);
return;
}

if (span.IsRecording)
if (activity.IsAllDataRequested)
{
// See https://github.com/aspnet/Mvc/blob/2414db256f32a047770326d14d8b0e2afd49ba49/src/Microsoft.AspNetCore.Mvc.Core/MvcCoreDiagnosticSourceExtensions.cs#L36-L44
// Reflection accessing: ActionDescriptor.AttributeRouteInfo.Template
Expand All @@ -145,9 +145,8 @@ public override void OnCustom(string name, Activity activity, object payload)
if (!string.IsNullOrEmpty(template))
{
// override the span name that was previously set to the path part of URL.
span.UpdateName(template);

span.PutHttpRouteAttribute(template);
activity.DisplayName = template;
activity.AddTag(SpanAttributeConstants.HttpRouteKey, template);
}

// TODO: Should we get values from RouteData?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// <copyright file="OpenTelemetryBuilderExtensions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using OpenTelemetry.Instrumentation.AspNetCore;

namespace OpenTelemetry.Trace.Configuration
{
/// <summary>
/// Extension methods to simplify registering of asp.net core request instrumentation.
/// </summary>
public static class OpenTelemetryBuilderExtensions
{
/// <summary>
/// Enables the incoming requests automatic data collection for Asp.Net Core.
/// </summary>
/// <param name="builder"><see cref="OpenTelemetryBuilder"/> being configured.</param>
/// <param name="configureAspNetCoreInstrumentationOptions">ASP.NET Core Request configuration options.</param>
/// <returns>The instance of <see cref="OpenTelemetryBuilder"/> to chain the calls.</returns>
public static OpenTelemetryBuilder AddRequestInstrumentation(
this OpenTelemetryBuilder builder,
Action<AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null)
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

builder.AddActivitySource(string.Empty);
var aspnetCoreOptions = new AspNetCoreInstrumentationOptions();
configureAspNetCoreInstrumentationOptions?.Invoke(aspnetCoreOptions);

// TODO: decide who is responsible for dispose upon shutdown.
new AspNetCoreInstrumentation(aspnetCoreOptions);

return builder;
}
}
}

This file was deleted.

12 changes: 12 additions & 0 deletions src/OpenTelemetry/Trace/Export/SimpleActivityProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using System.Threading;
using System.Threading.Tasks;
using OpenTelemetry.Internal;
using OpenTelemetry.Trace.Samplers;

namespace OpenTelemetry.Trace.Export
{
Expand All @@ -27,6 +28,7 @@ namespace OpenTelemetry.Trace.Export
public class SimpleActivityProcessor : ActivityProcessor, IDisposable
{
private readonly ActivityExporter exporter;
private readonly ActivitySampler sampler = new AlwaysOnActivitySampler();
private bool disposed = false;

/// <summary>
Expand All @@ -41,6 +43,16 @@ public SimpleActivityProcessor(ActivityExporter exporter)
/// <inheritdoc />
public override void OnStart(Activity activity)
{
// Perform sampling decision if source is empty.
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not 100% correct, as the sampling decision is now made with the current ActivityKind (which would be Internal, the default value). Though none of the built-in Samplers makes use of ActivityKind, nothing prevents one from writing Sampler which takes Kind into consideration.

More better would be to "feed" this activity created outside of ActivitySource, into the ActivityListener's GetRequestedDataUsingContext pipeline. That can be done in the instrumentationadapter itself, after setting the Kind to correct one, (and possibly enrich activity with more tags/links/events etc. if needed)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @CodeBlanch for proposing the idea of "feeding" an external activity to the GetRequestedDataUsingContext pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

Did you present the idea to .NET team? Any feedback?

Copy link
Contributor

Choose a reason for hiding this comment

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

@CodeBlanch we (the .NET team) will look at that to decide what is the best way can achieve this scenario. Thanks for your feedback, it is really helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that making sampling decision in SDK piece is going to be incorrect (as we have incorrect Kind), I'll move the sampling logic into the InstrumentationAdapters. Of course this forces Instrumentation to have dependency on Sampling - but doing this to make progress and uncover any other potential issues.

If everything else is good, and the only remaining issue is Instrumentation being aware of Samplers - we'll try to get a new API in activity source to run the GetRequestedDataUsingContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that making sampling decision in SDK piece is going to be incorrect (as we have incorrect Kind)

Is this to set the Kind? reflection can work here till we expose the Kind setter.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't set the Kind to the correct one in the OT SDK as SDK doesn't know what is the actual kind. InstrumentationAdapter knows it, and it sets it.

// This occurs for activities created outside of ActivitySource
// i.e new Activity() directly.
// Note: This also can occur if one creates an ActivitySource of the name string.empty.
// In that case, the sampling logic would be run twice!
if (activity.Source.Name.Equals(string.Empty))
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
var samplingDecision = this.sampler.ShouldSample(activity.Context, activity.TraceId, default, activity.DisplayName, activity.Kind, activity.Tags, activity.Links);
activity.IsAllDataRequested = samplingDecision.IsSampled;
}
}

/// <inheritdoc />
Expand Down