-
Notifications
You must be signed in to change notification settings - Fork 778
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
Asp.Net Core Requests Instrumentation - change to use new Activity API #692
Changes from 4 commits
24a43fe
b2e1088
58be52b
597fda7
c5a94cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -36,6 +36,9 @@ internal class HttpInListener : ListenerHandler | |||||||||||
private readonly bool hostingSupportsW3C = false; | ||||||||||||
private readonly AspNetCoreInstrumentationOptions options; | ||||||||||||
|
||||||||||||
// Create an ActivitySource here to re-create Activity. | ||||||||||||
private ActivitySource activitySource = new ActivitySource("Microsoft.AspNetCore"); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
do we usually create only one instance of HttpInListener and use it the life time of the app/service? I am asking to know if we need to explicitly dispose this ActivitySource when it is not needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes only one instance for life of app. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good. we should be ok here then. thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the existing implementation, we dispose the DiagnosticListeners when TracerFactory is disposed. Don't think we need it here. |
||||||||||||
|
||||||||||||
public HttpInListener(string name, Tracer tracer, AspNetCoreInstrumentationOptions options) | ||||||||||||
: base(name, tracer) | ||||||||||||
{ | ||||||||||||
|
@@ -60,11 +63,21 @@ public override void OnStartActivity(Activity activity, object payload) | |||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
// Create a brand new activity using ActivitySource | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this code into existing place itself, which does span population. Once we settle on approach, we'll be removing all the span populations and replace with activity. |
||||||||||||
// and populate its tags correctly. | ||||||||||||
// This is double allocation of Activity! and probably bad for performance. | ||||||||||||
var newActivity = this.activitySource.StartActivity("HttpRequestIn", ActivityKind.Server); | ||||||||||||
|
||||||||||||
var request = context.Request; | ||||||||||||
|
||||||||||||
// 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() : "/"; | ||||||||||||
|
||||||||||||
if (newActivity != null) | ||||||||||||
{ | ||||||||||||
newActivity.DisplayName = path; | ||||||||||||
} | ||||||||||||
|
||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can write this block as newActivity?.DisplayName = path; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add <LangVersion>7.3</LangVersion> to the csproj |
||||||||||||
TelemetrySpan span; | ||||||||||||
if (this.hostingSupportsW3C && this.options.TextFormat is TraceContextFormat) | ||||||||||||
{ | ||||||||||||
|
@@ -90,6 +103,28 @@ public override void OnStartActivity(Activity activity, object payload) | |||||||||||
span.PutHttpUserAgentAttribute(userAgent); | ||||||||||||
span.PutHttpRawUrlAttribute(GetUri(request)); | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (newActivity != null) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Can probably combine these two |
||||||||||||
{ | ||||||||||||
if (newActivity.IsAllDataRequested) | ||||||||||||
{ | ||||||||||||
if (request.Host.Port == 80 || request.Host.Port == 443) | ||||||||||||
{ | ||||||||||||
newActivity.AddTag("http.host", request.Host.Host); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you not include the port for 80/443? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just retaining the current behavior. Current behavior is probably because 80,443 are common and can be deducted from other info like url etc. |
||||||||||||
} | ||||||||||||
else | ||||||||||||
{ | ||||||||||||
newActivity.AddTag("http.host", request.Host.Host + ":" + request.Host.Port); | ||||||||||||
} | ||||||||||||
|
||||||||||||
newActivity.AddTag("http.method", request.Method); | ||||||||||||
newActivity.AddTag("http.path", path); | ||||||||||||
|
||||||||||||
var userAgent = request.Headers["User-Agent"].FirstOrDefault(); | ||||||||||||
newActivity.AddTag("http.user_agent", userAgent); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible for
Suggested change
|
||||||||||||
newActivity.AddTag("http.url", GetUri(request)); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
public override void OnStopActivity(Activity activity, object payload) | ||||||||||||
|
@@ -114,8 +149,15 @@ public override void OnStopActivity(Activity activity, object payload) | |||||||||||
var response = context.Response; | ||||||||||||
|
||||||||||||
span.PutHttpStatusCode(response.StatusCode, response.HttpContext.Features.Get<IHttpResponseFeature>().ReasonPhrase); | ||||||||||||
|
||||||||||||
// activity here will be the activity which we created using ActivitySource in the Start callback. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
how this magic is done? doesn't OnStopActivity is teh call back called from DiagnosticListener? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. diagnosticlistener callback itself doesn't give us activity. Instead we access it using Activity.Current. Since we started a new activity() in the OnBegin, Activity.Current will now be the one we created in OnBegin. This will have its .Parent set to the one created by "old" instrumentation code using DiagnosticSource.StartActivity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sense. thanks for the clarification. I am not expecting DiagnsoticListener will notify on the stop event of the Activity created from ActivitySource, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. (In fact diagnosticlistener will only notify for diagnosticsource.write events, never for activity.start or stop. On listening side, we get Activity.Current to get the activity) |
||||||||||||
if (activity.IsAllDataRequested) | ||||||||||||
{ | ||||||||||||
activity.AddTag("http.status_code", response.StatusCode.ToString()); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
activity.Stop(); | ||||||||||||
span.End(); | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for easily trying this out locally, i changed to use consoleexporter.
The Instrumention(Adapter) hook to OpenTelemetryBuilder is not implemented in this draft PR.