-
Notifications
You must be signed in to change notification settings - Fork 730
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
Changes from all commits
831d712
737f9b6
9b840f0
036a9bb
5b836a4
382d85f
f1e5108
170ad87
0832e71
3b4cbb7
19d138f
121e4b5
823fee6
c759d8a
6df93de
0d07263
692b0dc
7185426
37071c5
b889a8c
8630782
c4834ae
581cd1e
7341c59
d9c9fff
514d89c
f939361
a220d9e
99b91bf
63dc16f
11a175a
efdb1d3
3646fa1
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 |
---|---|---|
|
@@ -12,9 +12,10 @@ | |
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\..\..\src\OpenTelemetry.Exporter.Console\OpenTelemetry.Exporter.Console.csproj" /> | ||
<ProjectReference Include="..\..\..\src\OpenTelemetry.Exporter.Jaeger\OpenTelemetry.Exporter.Jaeger.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" /> | ||
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 will create the Activity exporter for Zipkin this week or the next. |
||
<ProjectReference Include="..\..\..\src\OpenTelemetry.Extensions.Hosting\OpenTelemetry.Extensions.Hosting.csproj" /> | ||
</ItemGroup> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -36,18 +37,15 @@ public void ConfigureServices(IServiceCollection services) | |
} | ||
}); | ||
|
||
services.AddOpenTelemetry((sp, builder) => | ||
{ | ||
builder | ||
//.SetSampler(Samplers.AlwaysSample) | ||
.UseZipkin(options => | ||
{ | ||
options.ServiceName = "test-zipkin"; | ||
options.Endpoint = new Uri(this.Configuration.GetValue<string>("Zipkin:Endpoint")); | ||
}) | ||
.AddRequestInstrumentation() | ||
.AddDependencyInstrumentation(); | ||
}); | ||
OpenTelemetrySdk.Default.EnableOpenTelemetry( | ||
(builder) => builder.AddRequestInstrumentation().AddDependencyInstrumentation() | ||
.UseJaegerActivityExporter(o => | ||
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. moved to jaeger as zipkin exporter with activity is not yet ready |
||
{ | ||
o.ServiceName = this.Configuration.GetValue<string>("Jaeger:ServiceName"); | ||
o.AgentHost = this.Configuration.GetValue<string>("Jaeger:Host"); | ||
o.AgentPort = this.Configuration.GetValue<int>("Jaeger:Port"); | ||
}) | ||
); | ||
} | ||
|
||
public void Configure(IApplicationBuilder app, IWebHostEnvironment env) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,7 +7,9 @@ | |||||||||||||
} | ||||||||||||||
}, | ||||||||||||||
"AllowedHosts": "*", | ||||||||||||||
"Zipkin": { | ||||||||||||||
"Endpoint": "http://localhost:9411/api/v2/spans" | ||||||||||||||
"Jaeger": { | ||||||||||||||
"ServiceName": "jaeger-test", | ||||||||||||||
"Host": "localhost", | ||||||||||||||
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. Side note: it feels strange that we need to set Jaeger Host and Port to what it should be default values. Anyway, to be addressed in a separate PR. 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. The defaults look like they are in there: opentelemetry-dotnet/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs Lines 35 to 40 in c7333e4
But IMO it's still worthwhile to show how you can change them via settings in the samples. 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.
Makes sense, perhaps we should comment where appropriate so the code setting (again) the default values don't get copied and pasted everywhere. |
||||||||||||||
"Port": 6831 | ||||||||||||||
} | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,17 +20,22 @@ | |
using System.Web.Routing; | ||
using OpenTelemetry.Context.Propagation; | ||
using OpenTelemetry.Trace; | ||
using OpenTelemetry.Trace.Samplers; | ||
|
||
namespace OpenTelemetry.Instrumentation.AspNet.Implementation | ||
{ | ||
internal class HttpInListener : ListenerHandler | ||
{ | ||
// hard-coded Sampler here, just to prototype. | ||
// Either .NET will provide an new API to avoid Instrumentation being aware of sampling. | ||
// or we'll move the Sampler to come from OpenTelemetryBuilder, and not hardcoded. | ||
private readonly ActivitySampler sampler = new AlwaysOnActivitySampler(); | ||
private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route"); | ||
private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate"); | ||
private readonly AspNetInstrumentationOptions options; | ||
|
||
public HttpInListener(string name, Tracer tracer, AspNetInstrumentationOptions options) | ||
: base(name, tracer) | ||
public HttpInListener(string name, AspNetInstrumentationOptions options) | ||
: base(name, null) | ||
{ | ||
this.options = options ?? throw new ArgumentNullException(nameof(options)); | ||
} | ||
|
@@ -48,53 +53,75 @@ public override void OnStartActivity(Activity activity, object payload) | |
|
||
if (this.options.RequestFilter != null && !this.options.RequestFilter(context)) | ||
{ | ||
// TODO: These filters won't prevent the activity from being tracked | ||
// as they are fired anyway. | ||
InstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName); | ||
return; | ||
} | ||
|
||
// TODO: Avoid the reflection hack once .NET ships new Activity with Kind settable. | ||
activity.GetType().GetProperty("Kind").SetValue(activity, ActivityKind.Server); | ||
|
||
var request = context.Request; | ||
var requestValues = request.Unvalidated; | ||
|
||
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md | ||
var path = requestValues.Path; | ||
|
||
TelemetrySpan span; | ||
if (this.options.TextFormat is TraceContextFormat) | ||
activity.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. Due to the risk of high-cardinality on the path OTel spec suggests using the HTTP method instead. That said the path is much more useful in general and having an option to add it (more likely the absolute path) will be useful. There are other locations also using the path as DisplayName, but other correctly use the helper that gets the name per OTel spec. BTW, the link on the comment above is broken. 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. Yes incoming requests were still using path as SpanName, hence I used it as activity.DisplayName. |
||
|
||
var samplingParameters = new ActivitySamplingParameters( | ||
activity.Context, | ||
activity.TraceId, | ||
activity.DisplayName, | ||
activity.Kind, | ||
activity.Tags, | ||
activity.Links); | ||
|
||
// TODO: Find a way to avoid Instrumentation being tied to Sampler | ||
var samplingDecision = this.sampler.ShouldSample(samplingParameters); | ||
activity.IsAllDataRequested = samplingDecision.IsSampled; | ||
if (samplingDecision.IsSampled) | ||
{ | ||
this.Tracer.StartActiveSpanFromActivity(path, Activity.Current, SpanKind.Server, out span); | ||
activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded; | ||
} | ||
else | ||
|
||
if (!(this.options.TextFormat is TraceContextFormat)) | ||
{ | ||
// This requires to ignore the current activity and create a new one | ||
// using the context extracted using the format TextFormat supports. | ||
// TODO: actually implement code doing the above. | ||
/* | ||
var ctx = this.options.TextFormat.Extract<HttpRequest>( | ||
request, | ||
(r, name) => requestValues.Headers.GetValues(name)); | ||
|
||
this.Tracer.StartActiveSpan(path, ctx, SpanKind.Server, out span); | ||
*/ | ||
} | ||
|
||
if (span.IsRecording) | ||
if (activity.IsAllDataRequested) | ||
{ | ||
span.PutHttpHostAttribute(request.Url.Host, request.Url.Port); | ||
span.PutHttpMethodAttribute(request.HttpMethod); | ||
span.PutHttpPathAttribute(path); | ||
if (request.Url.Port == 80 || request.Url.Port == 443) | ||
{ | ||
activity.AddTag(SpanAttributeConstants.HttpHostKey, request.Url.Host); | ||
} | ||
else | ||
{ | ||
activity.AddTag(SpanAttributeConstants.HttpHostKey, request.Url.Host + ":" + request.Url.Port); | ||
} | ||
|
||
span.PutHttpUserAgentAttribute(request.UserAgent); | ||
span.PutHttpRawUrlAttribute(request.Url.ToString()); | ||
activity.AddTag(SpanAttributeConstants.HttpMethodKey, request.HttpMethod); | ||
activity.AddTag(SpanAttributeConstants.HttpPathKey, path); | ||
activity.AddTag(SpanAttributeConstants.HttpUserAgentKey, request.UserAgent); | ||
activity.AddTag(SpanAttributeConstants.HttpUrlKey, request.Url.ToString()); | ||
} | ||
} | ||
|
||
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) | ||
{ | ||
var context = HttpContext.Current; | ||
if (context == null) | ||
|
@@ -104,8 +131,10 @@ public override void OnStopActivity(Activity activity, object payload) | |
} | ||
|
||
var response = context.Response; | ||
|
||
span.PutHttpStatusCode(response.StatusCode, response.StatusDescription); | ||
activity.AddTag(SpanAttributeConstants.HttpStatusCodeKey, response.StatusCode.ToString()); | ||
Status status = SpanHelper.ResolveSpanStatusForHttpStatusCode((int)response.StatusCode); | ||
activity.AddTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(status.CanonicalCode)); | ||
activity.AddTag(SpanAttributeConstants.StatusDescriptionKey, response.StatusDescription); | ||
|
||
var routeData = context.Request.RequestContext.RouteData; | ||
|
||
|
@@ -131,14 +160,11 @@ public override void OnStopActivity(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); | ||
// Override the name that was previously set to the path part of URL. | ||
activity.DisplayName = template; | ||
activity.AddTag(SpanAttributeConstants.HttpRouteKey, template); | ||
} | ||
} | ||
|
||
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.
I assume this is only for tests and not part of the final merge.
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.
Yes I can remove that. The testhttpClient example was doing something similar.
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.
To provide a little history, when I was building the .NET Framework instrumentation I added in those calls so I could do integration testing, basically. Make sure incoming & outgoing requests would show up in Jaeger/Zipkin correctly. In the end it made the samples more interesting so I left them in. 🤷♂️
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.
I see, what about adding a comment stating that it exists to also include outgoing on the sample?
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.
Yes adding a comment.