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

CustomProperty improvements #1261

Merged
merged 10 commits into from
Oct 6, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

using System;
using System.Diagnostics;
using System.Web;
using OpenTelemetry.Context.Propagation;

Expand All @@ -41,5 +42,10 @@ public class AspNetInstrumentationOptions
/// If Filter returns false or throw exception, the request is filtered out.
/// </summary>
public Func<HttpContext, bool> Filter { get; set; }

/// <summary>
/// Gets or sets an action to enrich an Activity.
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
public Action<Activity, string, object> Enrich { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ public override void OnStartActivity(Activity activity, object payload)

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
this.options.Enrich?.Invoke(activity, "OnStartActivity", request);

if (request.Url.Port == 80 || request.Url.Port == 443)
{
activity.SetTag(SemanticConventions.AttributeHttpHost, request.Url.Host);
Expand Down Expand Up @@ -159,7 +160,8 @@ public override void OnStopActivity(Activity activity, object payload)

var response = context.Response;

activityToEnrich.SetCustomProperty(ResponseCustomPropertyName, response);
this.options.Enrich?.Invoke(activity, "OnStopActivity", response);

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

activityToEnrich.SetStatus(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

using System;
using System.Diagnostics;
using Microsoft.AspNetCore.Http;
using OpenTelemetry.Context.Propagation;

Expand All @@ -41,5 +42,10 @@ public class AspNetCoreInstrumentationOptions
/// If Filter returns false or throw exception, the request is filtered out.
/// </summary>
public Func<HttpContext, bool> Filter { get; set; }

/// <summary>
/// Gets or sets an action to enrich an Activity.
/// </summary>
public Action<Activity, string, object> Enrich { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public override void OnStartActivity(Activity activity, object payload)

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
this.options.Enrich?.Invoke(activity, "OnStartActivity", request);
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Also - can you ensure that unit test validate that Enrich is called only if Activity is sampled-in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a test. Let me know what do you think before I update everything


var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
activity.DisplayName = path;
Expand Down Expand Up @@ -152,7 +152,9 @@ public override void OnStopActivity(Activity activity, object payload)
}

var response = context.Response;
activity.SetCustomProperty(ResponseCustomPropertyName, response);

this.options.Enrich?.Invoke(activity, "OnStopActivity", response);

activity.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);

if (TryGetGrpcMethod(activity, out var grpcMethod))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;

namespace OpenTelemetry.Instrumentation.GrpcNetClient
{
/// <summary>
Expand All @@ -25,5 +28,10 @@ public class GrpcClientInstrumentationOptions
/// Gets or sets a value indicating whether down stream instrumentation is suppressed (disabled).
/// </summary>
public bool SuppressDownstreamInstrumentation { get; set; }

/// <summary>
/// Gets or sets an action to enrich an Activity.
/// </summary>
public Action<Activity, string, object> Enrich { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ public override void OnStartActivity(Activity activity, object payload)

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
this.options.Enrich?.Invoke(activity, "OnStartActivity", 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 @@ -15,6 +15,7 @@
// </copyright>

using System;
using System.Diagnostics;
using System.Net.Http;
using System.Runtime.CompilerServices;
using OpenTelemetry.Context.Propagation;
Expand Down Expand Up @@ -50,6 +51,11 @@ public class HttpClientInstrumentationOptions
/// </summary>
public Func<HttpRequestMessage, bool> Filter { get; set; }

/// <summary>
/// Gets or sets an action to enrich an Activity.
/// </summary>
public Action<Activity, string, object> Enrich { get; set; }

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool EventFilter(string activityName, object arg1)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ public override void OnStartActivity(Activity activity, object payload)

if (activity.IsAllDataRequested)
{
activity.SetCustomProperty(RequestCustomPropertyName, request);
this.options.Enrich?.Invoke(activity, "OnStartActivity", 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 Expand Up @@ -139,7 +140,7 @@ public override void OnStopActivity(Activity activity, object payload)

if (this.stopResponseFetcher.Fetch(payload) is HttpResponseMessage response)
{
activity.SetCustomProperty(ResponseCustomPropertyName, response);
this.options.Enrich?.Invoke(activity, "OnStopActivity", response);

activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode);

Expand All @@ -163,7 +164,7 @@ public override void OnException(Activity activity, object payload)
return;
}

activity.SetCustomProperty(ExceptionCustomPropertyName, exc);
this.options.Enrich?.Invoke(activity, "OnException", exc);

if (exc is HttpRequestException)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ public override void OnCustom(string name, Activity activity, object payload)
var database = this.databaseFetcher.Fetch(connection);

activity.DisplayName = (string)database;
activity.SetCustomProperty(CommandCustomPropertyName, command);
this.options.Enrich?.Invoke(activity, "OnCustom", 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 @@ -13,6 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Collections.Concurrent;
using System.Data;
Expand Down Expand Up @@ -56,6 +57,11 @@ public class SqlClientInstrumentationOptions
/// </remarks>
public bool EnableConnectionLevelAttributes { get; set; }

/// <summary>
/// Gets or sets an action to enrich an Activity.
/// </summary>
public Action<Activity, string, object> Enrich { get; set; }

internal static SqlConnectionDetails ParseDataSource(string dataSource)
{
Match match = DataSourceRegex.Match(dataSource);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;

Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry/Trace/ActivityProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
// </copyright>

using System;
using System.Collections.Generic;
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
using System.Diagnostics;
using System.Threading;
using OpenTelemetry.Context;
using OpenTelemetry.Internal;

namespace OpenTelemetry.Trace
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ public void Dispose()
[InlineData("http://localhost/api/value", 0, null, "TraceContext", "/api/value")] // Request will be filtered
[InlineData("http://localhost/api/value", 0, null, "TraceContext", "{ThrowException}")] // Filter user code will throw an exception
[InlineData("http://localhost/api/value/2", 0, null, "CustomContext", "/api/value")] // Request will not be filtered
[InlineData("http://localhost/api/value/2", 0, null, "CustomContext", "/api/value", true)] // Request will not be filtered
public void AspNetRequestsAreCollectedSuccessfully(
string url,
int routeType,
Expand Down Expand Up @@ -162,6 +161,8 @@ public void AspNetRequestsAreCollectedSuccessfully(
{
options.Propagator = propagator.Object;
}

options.Enrich = ActivityEnrichment;
})
.SetResource(expectedResource)
.AddProcessor(activityProcessor.Object).Build())
Expand Down Expand Up @@ -251,13 +252,23 @@ public void AspNetRequestsAreCollectedSuccessfully(
Assert.Equal(HttpContext.Current.Request.UserAgent, span.GetTagValue(SemanticConventions.AttributeHttpUserAgent) as string);

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

private static void ActivityEnrichment(Activity activity, string method, object obj)
{
switch (method)
{
case "OnStartActivity":
Assert.True(obj is HttpRequest);
break;

var response = span.GetCustomProperty(HttpInListener.ResponseCustomPropertyName);
Assert.NotNull(response);
Assert.True(response is HttpResponse);
case "OnStopActivity":
Assert.True(obj is HttpResponse);
break;

default:
break;
}
}

private class FakeAspNetDiagnosticSource : IDisposable
Expand Down
36 changes: 27 additions & 9 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,23 @@ public void AddAspNetCoreInstrumentation_BadArgs()
Assert.Throws<ArgumentNullException>(() => builder.AddAspNetCoreInstrumentation());
}

[Fact]
public async Task SuccessfulTemplateControllerCallGeneratesASpan()
[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task SuccessfulTemplateControllerCallGeneratesASpan(bool shouldEnrich)
{
var expectedResource = Resources.Resources.CreateServiceResource("test-service");
var activityProcessor = new Mock<ActivityProcessor>();
void ConfigureTestServices(IServiceCollection services)
{
this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
.AddAspNetCoreInstrumentation()
.AddAspNetCoreInstrumentation(options =>
{
if (shouldEnrich)
{
options.Enrich = ActivityEnrichment;
}
})
.SetResource(expectedResource)
.AddProcessor(activityProcessor.Object)
.Build();
Expand Down Expand Up @@ -309,13 +317,23 @@ private static void ValidateAspNetCoreActivity(Activity activityToValidate, stri
Assert.Equal(ActivityKind.Server, activityToValidate.Kind);
Assert.Equal(expectedHttpPath, activityToValidate.GetTagValue(SpanAttributeConstants.HttpPathKey) as string);
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);
private static void ActivityEnrichment(Activity activity, string method, object obj)
{
switch (method)
{
case "OnStartActivity":
Assert.True(obj is HttpRequest);
break;

case "OnStopActivity":
Assert.True(obj is HttpResponse);
break;

default:
break;
}
}
}
}
Loading