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

[WIP] OWIN Instrumentation for OpenTelemetry #1472

Closed
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ charset = utf-8
# maintain DOS/Windows style line endings in md files
[*.md]
end_of_line = crlf
# MSBuild files
[*.{proj,csproj,props,targets}]
indent_size = 2
trim_trailing_whitespace = true
charset = utf-8
###############################
# .NET Coding Conventions #
###############################
Expand Down
12 changes: 12 additions & 0 deletions OpenTelemetry.sln
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "extending-the-sdk", "docs\l
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OpenTelemetry.Shared", "src\OpenTelemetry.Shared\OpenTelemetry.Shared.csproj", "{1E504265-1E32-4C61-8CC5-8FA373E16699}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OpenTelemetry.Instrumentation.Owin", "src\OpenTelemetry.Instrumentation.Owin\OpenTelemetry.Instrumentation.Owin.csproj", "{7B55B64E-609F-490E-A283-AD4D85FE2F19}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OpenTelemetry.Extensions.Owin", "src\OpenTelemetry.Extensions.Owin\OpenTelemetry.Extensions.Owin.csproj", "{EE939E55-B71A-4BB2-9483-635D92B1254A}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -402,6 +406,14 @@ Global
{1E504265-1E32-4C61-8CC5-8FA373E16699}.Debug|Any CPU.Build.0 = Debug|Any CPU
{1E504265-1E32-4C61-8CC5-8FA373E16699}.Release|Any CPU.ActiveCfg = Release|Any CPU
{1E504265-1E32-4C61-8CC5-8FA373E16699}.Release|Any CPU.Build.0 = Release|Any CPU
{7B55B64E-609F-490E-A283-AD4D85FE2F19}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{7B55B64E-609F-490E-A283-AD4D85FE2F19}.Debug|Any CPU.Build.0 = Debug|Any CPU
{7B55B64E-609F-490E-A283-AD4D85FE2F19}.Release|Any CPU.ActiveCfg = Release|Any CPU
{7B55B64E-609F-490E-A283-AD4D85FE2F19}.Release|Any CPU.Build.0 = Release|Any CPU
{EE939E55-B71A-4BB2-9483-635D92B1254A}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{EE939E55-B71A-4BB2-9483-635D92B1254A}.Debug|Any CPU.Build.0 = Debug|Any CPU
{EE939E55-B71A-4BB2-9483-635D92B1254A}.Release|Any CPU.ActiveCfg = Release|Any CPU
{EE939E55-B71A-4BB2-9483-635D92B1254A}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
2 changes: 2 additions & 0 deletions build/Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@
<MicrosoftExtensionsLoggingConfigurationPkgVer>[2.1.0,6.0)</MicrosoftExtensionsLoggingConfigurationPkgVer>
<MicrosoftNETFrameworkReferenceAssembliesPkgVer>[1.0.0,2.0)</MicrosoftNETFrameworkReferenceAssembliesPkgVer>
<MicrosoftSourceLinkGitHubPkgVer>[1.0.0,2.0)</MicrosoftSourceLinkGitHubPkgVer>
<MicrosoftOwinPkgVer>[4.1.1]</MicrosoftOwinPkgVer>
<NewtonsoftJsonPkgVer>[12.0.2,13.0)</NewtonsoftJsonPkgVer>
<OpenTracingPkgVer>[0.12.1,0.13)</OpenTracingPkgVer>
<StackExchangeRedisPkgVer>[2.1.58,3.0)</StackExchangeRedisPkgVer>
<StyleCopAnalyzersPkgVer>[1.1.118,2.0)</StyleCopAnalyzersPkgVer>
<SystemCollectionsImmutablePkgVer>[1.4.0,5.0]</SystemCollectionsImmutablePkgVer>
<SystemDiagnosticSourcePkgVer>[5.0.0-rc.2.20475.5]</SystemDiagnosticSourcePkgVer>
<SystemNetHttpPkgVer>[4.3.4]</SystemNetHttpPkgVer>
<SystemReflectionEmitLightweightPkgVer>[4.7.0,5.0)</SystemReflectionEmitLightweightPkgVer>
<SystemTextJsonPkgVer>[4.7.0,5.0)</SystemTextJsonPkgVer>
<SystemThreadingTasksExtensionsPkgVer>[4.5.3,5.0)</SystemThreadingTasksExtensionsPkgVer>
Expand Down
32 changes: 32 additions & 0 deletions src/OpenTelemetry.Extensions.Owin/AppBuilderExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// <copyright file="AppBuilderExtensions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using Owin;

namespace OpenTelemetry
{
/// <summary>
/// Provides extension methods for the <see cref="IAppBuilder"/> class.
/// </summary>
public static class AppBuilderExtensions
{
/// <summary>Adds a component to the OWIN pipeline for instrumenting incoming request with System.Diagnostics.Activity and notifying listeners with DiagnosticsSource.</summary>
/// <param name="appBuilder">The application builder.</param>
/// <returns>The application builder instance.</returns>
public static IAppBuilder UseOpenTelemetry(this IAppBuilder appBuilder)
Copy link
Member

Choose a reason for hiding this comment

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

Might want to document that this middleware should be applied as early as possible so as to time the entire middleware chain.

=> appBuilder.Use<DiagnosticsMiddleware>();
}
}
22 changes: 22 additions & 0 deletions src/OpenTelemetry.Extensions.Owin/AssemblyInfo.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// <copyright file="AssemblyInfo.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System.Runtime.CompilerServices;

#if SIGNED
[assembly: InternalsVisibleTo("OpenTelemetry.Extensions.Owin.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010051c1562a090fb0c9f391012a32198b5e5d9a60e9b80fa2d7b434c9e5ccb7259bd606e66f9660676afc6692b8cdc6793d190904551d2103b7b22fa636dcbb8208839785ba402ea08fc00c8f1500ccef28bbf599aa64ffb1e1d5dc1bf3420a3777badfe697856e9d52070a50c3ea5821c80bef17ca3acffa28f89dd413f096f898")]
#else
[assembly: InternalsVisibleTo("OpenTelemetry.Extensions.Owin.Tests")]
#endif
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Extensions.Owin/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Changelog

## Unreleased

* Initial release
159 changes: 159 additions & 0 deletions src/OpenTelemetry.Extensions.Owin/DiagnosticsMiddleware.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
// <copyright file="DiagnosticsMiddleware.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Owin;
using OpenTelemetry.Implementation;

namespace OpenTelemetry
{
/// <summary>
/// Instruments incoming request with System.Diagnostics.Activity and notifies listeners with DiagnosticsSource.
/// </summary>
public sealed class DiagnosticsMiddleware : OwinMiddleware
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public?

{
private const string ActivityName = "OpenTelemetry.Extensions.Owin.HttpRequestIn";
private const string ActivityStartKey = ActivityName + ".Start";
private const string ActivityStopKey = ActivityName + ".Stop";

private readonly DiagnosticListener diagnosticListener = new DiagnosticListener("OpenTelemetry.Extensions.Owin");
private readonly Context context = new Context();

/// <summary>
/// Initializes a new instance of the <see cref="DiagnosticsMiddleware"/> class.
/// </summary>
/// <param name="next">An optional pointer to the next component.</param>
public DiagnosticsMiddleware(OwinMiddleware next)
: base(next)
{
}

/// <inheritdoc />
public override async Task Invoke(IOwinContext owinContext)
{
try
{
this.BeginRequest(owinContext);
await this.Next.Invoke(owinContext).ConfigureAwait(false);
this.RequestEnd(owinContext, null);
}
catch (Exception ex)
{
this.RequestEnd(owinContext, ex);
}
}

// Based on https://github.com/dotnet/aspnetcore/blob/v5.0.0-rc.2.20475.17/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L37
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void BeginRequest(IOwinContext owinContext)
{
if (OwinExtensionsEventSource.Log.IsEnabled())
{
this.context.EventLogEnabled = true;
}

if (this.diagnosticListener.IsEnabled() && this.diagnosticListener.IsEnabled(ActivityName, owinContext))
{
this.context.Activity = this.StartActivity(owinContext, out var hasDiagnosticListener);
this.context.HasDiagnosticListener = hasDiagnosticListener;
}
}

// Based on https://github.com/dotnet/aspnetcore/blob/v5.0.0-rc.2.20475.17/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L89
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void RequestEnd(IOwinContext owinContext, Exception exception)
{
var activity = this.context.Activity;

// Always stop activity if it was started
if (activity != null)
{
this.StopActivity(owinContext, activity, this.context.HasDiagnosticListener);
}

if (this.context.EventLogEnabled && exception != null)
{
// Non-inline
OwinExtensionsEventSource.Log.UnhandledException();
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
private Activity StartActivity(IOwinContext owinContext, out bool hasDiagnosticListener)
Copy link
Member

Choose a reason for hiding this comment

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

The recommendation is to use ActivitySource based instrumentation for any new instrumentations. DiagnosticListener based ones are "legacy".
This instructions are being documented here (WIP):
#1467

I can help with replacing the DS -> ActivitySource ones.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @cijothomas, I'll replace DiagnosticListener with ActivitySource.

{
hasDiagnosticListener = false;

var activity = new Activity(ActivityName);

// Based on https://github.com/microsoft/ApplicationInsights-dotnet/blob/2.15.0/WEB/Src/Web/Web/Implementation/RequestTrackingExtensions.cs#L41
if (!activity.Extract(owinContext.Request.Headers))
Copy link
Member

Choose a reason for hiding this comment

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

We need to move on to Propagagators.DefaultTextMapPropagator approach.

Copy link
Author

Choose a reason for hiding this comment

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

Can you please point me to some examples?

Copy link
Member

Choose a reason for hiding this comment

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

{
// Force parsing Correlation-Context in absence of Request-Id or traceparent.
owinContext.Request.Headers.ReadActivityBaggage(activity);
}

this.diagnosticListener.OnActivityImport(activity, owinContext);

if (this.diagnosticListener.IsEnabled(ActivityStartKey))
{
hasDiagnosticListener = true;
this.StartActivity(activity, owinContext);
}
else
{
activity.Start();
}

return activity;
}

// These are versions of DiagnosticSource.Start/StopActivity that don't allocate strings per call (see https://github.com/dotnet/corefx/issues/37055)
private void StartActivity(Activity activity, IOwinContext owinContext)
{
activity.Start();
this.diagnosticListener.Write(ActivityStartKey, owinContext);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private void StopActivity(IOwinContext owinContext, Activity activity, bool hasDiagnosticListener)
{
if (hasDiagnosticListener)
{
this.StopActivity(activity, owinContext);
}
else
{
activity.Stop();
}
}

private void StopActivity(Activity activity, IOwinContext owinContext)
{
// Stop sets the end time if it was unset, but we want it set before we issue the write
// so we do it now.
if (activity.Duration == TimeSpan.Zero)
{
activity.SetEndTime(DateTime.UtcNow);
}

this.diagnosticListener.Write(ActivityStopKey, owinContext);
activity.Stop(); // Resets Activity.Current (we want this after the Write)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// <copyright file="ActivityExtensions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Diagnostics;
using System.Net.Http.Headers;
using Microsoft.Owin;

namespace OpenTelemetry.Implementation
{
internal static class ActivityExtensions
{
/// <summary>
/// Maximum length of Correlation-Context header value.
/// </summary>
private const int MaxCorrelationContextLength = 1024;

/// <summary>
/// Reads Request-Id and Correlation-Context headers and sets ParentId and Baggage on Activity.
/// </summary>
/// <param name="activity">Instance of activity that has not been started yet.</param>
/// <param name="requestHeaders">Request headers collection.</param>
/// <returns>true if request was parsed successfully, false - otherwise.</returns>
// Based on the code from https://github.com/aspnet/Microsoft.AspNet.TelemetryCorrelation/blob/master/src/Microsoft.AspNet.TelemetryCorrelation/ActivityExtensions.cs#L48.
public static bool Extract(this Activity activity, IHeaderDictionary requestHeaders)
{
if (activity == null)
{
OwinExtensionsEventSource.Log.ActivityExtractionError("activity is null");
return false;
}

if (activity.ParentId != null)
{
OwinExtensionsEventSource.Log.ActivityExtractionError("ParentId is already set on activity");
return false;
}

if (activity.Id != null)
{
OwinExtensionsEventSource.Log.ActivityExtractionError("Activity is already started");
return false;
}

var traceParents = requestHeaders.GetValues(HeaderNames.TraceParent);
if (traceParents == null || traceParents.Count == 0)
{
traceParents = requestHeaders.GetValues(HeaderNames.RequestId);
}

if (traceParents != null && traceParents.Count > 0 && !string.IsNullOrEmpty(traceParents[0]))
{
// there may be several Request-Id or traceparent headers, but we only read the first one
Copy link
Member

Choose a reason for hiding this comment

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

This would go against the W3C TraceContext spec.
Here goes the compliance test FYI https://github.com/w3c/trace-context/blob/1ea757e565f3f68f453ebd62a010e6ae9d9be5f2/test/test.py#L134.

Copy link
Member

Choose a reason for hiding this comment

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

Once Propagator API is used, this would get handled by the TraceContextPropagator, and libraries don't have to write this code.

activity.SetParentId(traceParents[0]);

var traceStates = requestHeaders.GetValues(HeaderNames.TraceState);
if (traceStates != null && traceStates.Count > 0)
{
if (traceStates.Count == 1 && !string.IsNullOrEmpty(traceStates[0]))
{
activity.TraceStateString = traceStates[0];
}
else
{
activity.TraceStateString = string.Join(",", traceStates);
}
}

// Header format - Correlation-Context: key1=value1, key2=value2
var baggages = requestHeaders.GetValues(HeaderNames.CorrelationContext);
if (baggages != null)
{
int correlationContextLength = -1;

// there may be several Correlation-Context headers
foreach (var item in baggages)
{
if (correlationContextLength >= MaxCorrelationContextLength)
{
break;
}

foreach (var pair in item.Split(','))
{
correlationContextLength += pair.Length + 1; // pair and comma

if (correlationContextLength >= MaxCorrelationContextLength)
{
break;
}

if (NameValueHeaderValue.TryParse(pair, out NameValueHeaderValue baggageItem))
{
activity.AddBaggage(baggageItem.Name, baggageItem.Value);
}
else
{
OwinExtensionsEventSource.Log.HeaderParsingError(HeaderNames.CorrelationContext, pair);
}
}
}
}

return true;
}

return false;
}
}
}
Loading