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 to store context object into Activity.CustomProperty #1128

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Asp.Net Instrumentation automatically populates HttpRequest, HttpResponse in
Activity custom property

* Changed the default propagation to support W3C Baggage
([#1048](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1048))
* The default ITextFormat is now `CompositePropagator(TraceContextFormat,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation
{
internal class HttpInListener : ListenerHandler
{
public const string RequestCustomPropertyName = "OTel.AspNet.Request";
public const string ResponseCustomPropertyName = "OTel.AspNet.Response";
private const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener";
private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name);
private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route");
Expand Down Expand Up @@ -102,6 +104,7 @@ public override void OnStartActivity(Activity activity, object payload)

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
if (request.Url.Port == 80 || request.Url.Port == 443)
{
activity.SetTag(SemanticConventions.AttributeHttpHost, request.Url.Host);
Expand Down Expand Up @@ -151,6 +154,7 @@ public override void OnStopActivity(Activity activity, object payload)

var response = context.Response;

activityToEnrich.SetCustomProperty(ResponseCustomPropertyName, response);
activityToEnrich.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);

activityToEnrich.SetStatus(
Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Asp.Net Core Instrumentation automatically populates HttpRequest, HttpResponse in
Activity custom property

* Changed the default propagation to support W3C Baggage
([#1048](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1048))
* The default ITextFormat is now `CompositePropagator(TraceContextFormat,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation
{
internal class HttpInListener : ListenerHandler
{
public const string RequestCustomPropertyName = "OTel.AspNetCore.Request";
public const string ResponseCustomPropertyName = "OTel.AspNetCore.Response";
private const string UnknownHostName = "UNKNOWN-HOST";
private const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener";
private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers[name];
Expand Down Expand Up @@ -98,6 +100,8 @@ public override void OnStartActivity(Activity activity, object payload)

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);

var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
activity.DisplayName = path;

Expand Down Expand Up @@ -135,6 +139,7 @@ public override void OnStopActivity(Activity activity, object payload)
}

var response = context.Response;
activity.SetCustomProperty(ResponseCustomPropertyName, response);
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);

if (TryGetGrpcMethod(activity, out var grpcMethod))
Expand Down
5 changes: 3 additions & 2 deletions src/OpenTelemetry.Instrumentation.Grpc/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

## Unreleased

* Grpc instrumentation will now add the raw Request object to the Activity it
creates
* Grpc Instrumentation automatically populates HttpRequest in
Activity custom property
([#1099](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1099))
([#1128](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1128))

## 0.4.0-beta.2

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ public override void OnStartActivity(Activity activity, object payload)

activity.SetKind(ActivityKind.Client);
activity.DisplayName = grpcMethod?.Trim('/');
activity.SetCustomProperty(RequestCustomPropertyName, request);

this.activitySource.Start(activity);

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
activity.SetTag(SemanticConventions.AttributeRpcSystem, GrpcTagHelper.RpcSystemGrpc);

if (GrpcTagHelper.TryParseRpcServiceAndRpcMethod(grpcMethod, out var rpcService, out var rpcMethod))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,12 @@ public override void OnStartActivity(Activity activity, object payload)

activity.SetKind(ActivityKind.Client);
activity.DisplayName = HttpTagHelper.GetOperationNameForHttpMethod(request.Method);
activity.SetCustomProperty(RequestCustomPropertyName, request);

this.activitySource.Start(activity);

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
activity.SetTag(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method));
activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,9 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A

InstrumentRequest(request, activity);

activity.SetCustomProperty(RequestCustomPropertyName, request);

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);
Expand All @@ -118,10 +117,9 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void AddResponseTags(HttpWebResponse response, Activity activity)
{
activity.SetCustomProperty(ResponseCustomPropertyName, response);

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(ResponseCustomPropertyName, response);
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode);

activity.SetStatus(
Expand All @@ -134,13 +132,13 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void AddExceptionTags(Exception exception, Activity activity)
{
activity.SetCustomProperty(ExceptionCustomPropertyName, exception);

if (!activity.IsAllDataRequested)
{
return;
}

activity.SetCustomProperty(ExceptionCustomPropertyName, exception);
Copy link
Member

Choose a reason for hiding this comment

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

In HttpWebRequestActivitySource.netfx.cs there is also Request & Response set, both before activity.IsAllDataRequested check. Did you leave them alone on purpose?

I don't think that's necessarily a bad thing. I intentionally put them all outside of the check for anyone wanting to do advanced things. Something like... make a sampling decision based on the raw requests. Or maybe have their own ActivityListeners? Not saying we must do that, I just thought it was worth the perf 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed it. I prefer to keep everything inside AllDataRequested.

My general take is to follow what open telemetry spec suggests:
Samplers get a specific set of things - name, parentcontext, kind, initial attributes, links.
we don't want to provide more anything more, unless there is a strong reason to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw - samplers dont get access to raw Actiivty itself, so even if someone want to make intelligent sampling decision, they wont be able to leverage this custom obj.

Copy link
Member

Choose a reason for hiding this comment

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

Hah ok good point!


Status status;
if (exception is WebException wexc)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,13 @@ public override void OnCustom(string name, Activity activity, object payload)
return;
}

var connection = this.connectionFetcher.Fetch(command);
var database = this.databaseFetcher.Fetch(connection);

activity.DisplayName = (string)database;
activity.SetCustomProperty(CommandCustomPropertyName, command);

if (activity.IsAllDataRequested)
{
var connection = this.connectionFetcher.Fetch(command);
var database = this.databaseFetcher.Fetch(connection);

activity.DisplayName = (string)database;
activity.SetCustomProperty(CommandCustomPropertyName, command);
var dataSource = this.dataSourceFetcher.Fetch(connection);
var commandText = this.commandTextFetcher.Fetch(command);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
using System.Web.Routing;
using Moq;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Instrumentation.AspNet.Implementation;
using OpenTelemetry.Trace;
using Xunit;

Expand Down Expand Up @@ -265,6 +266,13 @@ public void AspNetRequestsAreCollectedSuccessfully(
span.Tags.FirstOrDefault(i => i.Key == SemanticConventions.AttributeHttpUserAgent).Value as string);

Assert.Equal(expectedResource, span.GetResource());
var request = span.GetCustomProperty(HttpInListener.RequestCustomPropertyName);
Assert.NotNull(request);
Assert.True(request is HttpRequest);

var response = span.GetCustomProperty(HttpInListener.ResponseCustomPropertyName);
Assert.NotNull(response);
Assert.True(response is HttpResponse);
}

private class FakeAspNetDiagnosticSource : IDisposable
Expand Down
63 changes: 38 additions & 25 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
using Microsoft.Extensions.DependencyInjection;
using Moq;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Instrumentation.AspNetCore.Implementation;
using OpenTelemetry.Trace;
#if NETCOREAPP2_1
using TestApp.AspNetCore._2._1;
Expand Down Expand Up @@ -60,13 +61,13 @@ public void AddAspNetCoreInstrumentation_BadArgs()
public async Task SuccessfulTemplateControllerCallGeneratesASpan()
{
var expectedResource = Resources.Resources.CreateServiceResource("test-service");
var spanProcessor = new Mock<ActivityProcessor>();
var activityProcessor = new Mock<ActivityProcessor>();
void ConfigureTestServices(IServiceCollection services)
{
this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation()
.SetResource(expectedResource)
.AddProcessor(spanProcessor.Object)
.AddProcessor(activityProcessor.Object)
.Build();
}

Expand All @@ -82,21 +83,19 @@ void ConfigureTestServices(IServiceCollection services)
// Assert
response.EnsureSuccessStatusCode(); // Status Code 200-299

WaitForProcessorInvocations(spanProcessor, 2);
WaitForProcessorInvocations(activityProcessor, 2);
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
var activity = (Activity)activityProcessor.Invocations[1].Arguments[0];

Assert.Equal(ActivityKind.Server, span.Kind);
Assert.Equal("/api/values", span.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
Assert.Equal(expectedResource, span.GetResource());
ValidateAspNetCoreActivity(activity, "/api/values", expectedResource);
}

[Fact]
public async Task SuccessfulTemplateControllerCallUsesParentContext()
{
var spanProcessor = new Mock<ActivityProcessor>();
var activityProcessor = new Mock<ActivityProcessor>();

var expectedTraceId = ActivityTraceId.CreateRandom();
var expectedSpanId = ActivitySpanId.CreateRandom();
Expand All @@ -107,7 +106,7 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
builder.ConfigureTestServices(services =>
{
this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder().AddAspNetCoreInstrumentation()
.AddProcessor(spanProcessor.Object)
.AddProcessor(activityProcessor.Object)
.Build();
})))
{
Expand All @@ -121,11 +120,11 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
// Assert
response.EnsureSuccessStatusCode(); // Status Code 200-299

WaitForProcessorInvocations(spanProcessor, 2);
WaitForProcessorInvocations(activityProcessor, 2);
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
var span = (Activity)activityProcessor.Invocations[1].Arguments[0];

Assert.Equal(ActivityKind.Server, span.Kind);
Assert.Equal("api/Values/{id}", span.DisplayName);
Expand All @@ -138,7 +137,7 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
[Fact]
public async Task CustomTextFormat()
{
var spanProcessor = new Mock<ActivityProcessor>();
var activityProcessor = new Mock<ActivityProcessor>();

var expectedTraceId = ActivityTraceId.CreateRandom();
var expectedSpanId = ActivitySpanId.CreateRandom();
Expand All @@ -157,20 +156,20 @@ public async Task CustomTextFormat()
{
this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation((opt) => opt.TextFormat = textFormat.Object)
.AddProcessor(spanProcessor.Object)
.AddProcessor(activityProcessor.Object)
.Build();
})))
{
using var client = testFactory.CreateClient();
var response = await client.GetAsync("/api/values/2");
response.EnsureSuccessStatusCode(); // Status Code 200-299

WaitForProcessorInvocations(spanProcessor, 2);
WaitForProcessorInvocations(activityProcessor, 2);
}

// begin and end was called once each.
Assert.Equal(2, spanProcessor.Invocations.Count);
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
Assert.Equal(2, activityProcessor.Invocations.Count);
var span = (Activity)activityProcessor.Invocations[1].Arguments[0];

Assert.Equal(ActivityKind.Server, span.Kind);
Assert.True(span.Duration != TimeSpan.Zero);
Expand All @@ -184,13 +183,13 @@ public async Task CustomTextFormat()
[Fact]
public async Task FilterOutRequest()
{
var spanProcessor = new Mock<ActivityProcessor>();
var activityProcessor = new Mock<ActivityProcessor>();

void ConfigureTestServices(IServiceCollection services)
{
this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation((opt) => opt.RequestFilter = (ctx) => ctx.Request.Path != "/api/values/2")
.AddProcessor(spanProcessor.Object)
.AddProcessor(activityProcessor.Object)
.Build();
}

Expand All @@ -209,12 +208,12 @@ void ConfigureTestServices(IServiceCollection services)
response1.EnsureSuccessStatusCode(); // Status Code 200-299
response2.EnsureSuccessStatusCode(); // Status Code 200-299

WaitForProcessorInvocations(spanProcessor, 2);
WaitForProcessorInvocations(activityProcessor, 2);
}

// we should only create one span and never call processor with another
Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
var span = (Activity)activityProcessor.Invocations[1].Arguments[0];

Assert.Equal(ActivityKind.Server, span.Kind);
Assert.Equal("/api/values", span.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
Expand All @@ -225,7 +224,7 @@ public void Dispose()
this.openTelemetrySdk?.Dispose();
}

private static void WaitForProcessorInvocations(Mock<ActivityProcessor> spanProcessor, int invocationCount)
private static void WaitForProcessorInvocations(Mock<ActivityProcessor> activityProcessor, int invocationCount)
{
// We need to let End callback execute as it is executed AFTER response was returned.
// In unit tests environment there may be a lot of parallel unit tests executed, so
Expand All @@ -234,9 +233,23 @@ private static void WaitForProcessorInvocations(Mock<ActivityProcessor> spanProc
() =>
{
Thread.Sleep(10);
return spanProcessor.Invocations.Count >= invocationCount;
return activityProcessor.Invocations.Count >= invocationCount;
},
TimeSpan.FromSeconds(1)));
}

private static void ValidateAspNetCoreActivity(Activity activityToValidate, string expectedHttpPath, Resources.Resource expectedResource)
{
Assert.Equal(ActivityKind.Server, activityToValidate.Kind);
Assert.Equal(expectedHttpPath, activityToValidate.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
Assert.Equal(expectedResource, activityToValidate.GetResource());
var request = activityToValidate.GetCustomProperty(HttpInListener.RequestCustomPropertyName);
Assert.NotNull(request);
Assert.True(request is HttpRequest);

var response = activityToValidate.GetCustomProperty(HttpInListener.ResponseCustomPropertyName);
Assert.NotNull(response);
Assert.True(response is HttpResponse);
}
}
}
Loading