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

Add additional tags to ASP.NET Core metrics #3247

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

* Added additional metric dimensions.
([3247](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3247))

* Removes net5.0 target as .NET 5.0 is going out
of support. The package keeps netstandard2.1 target, so it
can still be used with .NET5.0 apps.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
using System.Diagnostics;
using System.Diagnostics.Metrics;
using Microsoft.AspNetCore.Http;
#if NETCOREAPP
using Microsoft.AspNetCore.Routing;
#endif
using OpenTelemetry.Trace;

namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation
Expand Down Expand Up @@ -51,16 +54,60 @@ public override void OnStopActivity(Activity activity, object payload)
return;
}

string host;

if (context.Request.Host.Port is null or 80 or 443)
{
host = context.Request.Host.Host;
}
else
{
host = context.Request.Host.Host + ":" + context.Request.Host.Port;
Copy link
Member

Choose a reason for hiding this comment

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

for a follow up - see if we avoid this string alloc, by using cache/other technique?

}

TagList tags;

// We need following directive as
// RouteEndpoint is not available in netstandard2.0 and netstandard2.1
#if NETCOREAPP
Copy link
Member

Choose a reason for hiding this comment

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

We have a net6.0 and netstandard2.1 build of this instrumentation. Does this directive apply to those targets?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, now I'm wondering why we need a netstandard2.1 target given that we have a netcoreapp3.1 target.

Copy link
Member Author

@vishweshbankwar vishweshbankwar May 3, 2022

Choose a reason for hiding this comment

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

This directive only applies to netcoreapp3.1 and net6.0. It does not apply to netstandard2.1or netstandard2.0

Copy link
Member

Choose a reason for hiding this comment

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

could you add a comment why this compiler directive is relevant. ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cijothomas - Added - Please review.

Copy link
Member

Choose a reason for hiding this comment

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

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 do not have that information available during OnStop event.

var target = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;

// TODO: This is just a minimal set of attributes. See the spec for additional attributes:
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server
var tags = new TagList
if (!string.IsNullOrEmpty(target))
{
tags = new TagList
{
{ SemanticConventions.AttributeHttpFlavor, context.Request.Protocol },
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme },
{ SemanticConventions.AttributeHttpMethod, context.Request.Method },
{ SemanticConventions.AttributeHttpHost, host },
{ SemanticConventions.AttributeHttpTarget, target },
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() },
};
}
else
{
tags = new TagList
{
{ SemanticConventions.AttributeHttpFlavor, context.Request.Protocol },
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme },
{ SemanticConventions.AttributeHttpMethod, context.Request.Method },
{ SemanticConventions.AttributeHttpHost, host },
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() },
};
}
#else
tags = new TagList
{
{ SemanticConventions.AttributeHttpMethod, context.Request.Method },
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme },
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode },
{ SemanticConventions.AttributeHttpFlavor, context.Request.Protocol },
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme },
{ SemanticConventions.AttributeHttpMethod, context.Request.Method },
{ SemanticConventions.AttributeHttpHost, host },
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() },
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-dotnet/pull/3247/files#r866970771 same comment. we can follow up to fix the string alloc.

};

#endif
this.httpServerDuration.Record(activity.Duration.TotalMilliseconds, tags);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,13 +116,17 @@ public async Task RequestMetricIsCaptured()

var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, "GET");
var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, "http");
var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, 200);
var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, "200");
var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, "HTTP/1.1");
var host = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpHost, "localhost");
var target = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpTarget, "api/Values");
Assert.Contains(method, attributes);
Assert.Contains(scheme, attributes);
Assert.Contains(statusCode, attributes);
Assert.Contains(flavor, attributes);
Assert.Equal(4, attributes.Length);
Assert.Contains(host, attributes);
Assert.Contains(target, attributes);
Assert.Equal(6, attributes.Length);
}

public void Dispose()
Expand Down