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

Log outgoing HTTP path parameters in Structured mode #4845

Merged
merged 4 commits into from
Jan 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Buffers;
using System.Collections.Frozen;
using System.Collections.Generic;
using System.Net.Http;
Expand All @@ -19,6 +20,7 @@ namespace Microsoft.Extensions.Http.Logging.Internal;
internal sealed class HttpRequestReader : IHttpRequestReader
{
private readonly IHttpRouteFormatter _routeFormatter;
private readonly IHttpRouteParser _httpRouteParser;
private readonly IHttpHeadersReader _httpHeadersReader;
private readonly FrozenDictionary<string, DataClassification> _defaultSensitiveParameters;

Expand All @@ -44,12 +46,14 @@ public HttpRequestReader(
IServiceProvider serviceProvider,
IOptionsMonitor<LoggingOptions> optionsMonitor,
IHttpRouteFormatter routeFormatter,
IHttpRouteParser httpRouteParser,
IOutgoingRequestContext requestMetadataContext,
IDownstreamDependencyMetadataManager? downstreamDependencyMetadataManager = null,
[ServiceKey] string? serviceKey = null)
: this(
optionsMonitor.GetKeyedOrCurrent(serviceKey),
routeFormatter,
httpRouteParser,
serviceProvider.GetRequiredOrKeyedRequiredService<IHttpHeadersReader>(serviceKey),
requestMetadataContext,
downstreamDependencyMetadataManager)
Expand All @@ -59,6 +63,7 @@ public HttpRequestReader(
internal HttpRequestReader(
LoggingOptions options,
IHttpRouteFormatter routeFormatter,
IHttpRouteParser httpRouteParser,
IHttpHeadersReader httpHeadersReader,
IOutgoingRequestContext requestMetadataContext,
IDownstreamDependencyMetadataManager? downstreamDependencyMetadataManager = null)
Expand All @@ -67,6 +72,7 @@ internal HttpRequestReader(
_httpHeadersReader = httpHeadersReader;

_routeFormatter = routeFormatter;
_httpRouteParser = httpRouteParser;
_requestMetadataContext = requestMetadataContext;
_downstreamDependencyMetadataManager = downstreamDependencyMetadataManager;

Expand All @@ -92,7 +98,7 @@ public async Task ReadRequestAsync(LogRecord logRecord, HttpRequestMessage reque
{
logRecord.Host = request.RequestUri?.Host ?? TelemetryConstants.Unknown;
logRecord.Method = request.Method;
logRecord.Path = GetRedactedPath(request);
GetRedactedPathAndParameters(request, logRecord);

if (_logRequestHeaders)
{
Expand Down Expand Up @@ -125,16 +131,19 @@ public async Task ReadResponseAsync(LogRecord logRecord, HttpResponseMessage res
logRecord.StatusCode = (int)response.StatusCode;
}

private string GetRedactedPath(HttpRequestMessage request)
private void GetRedactedPathAndParameters(HttpRequestMessage request, LogRecord logRecord)
{
logRecord.PathParameters = null;
if (request.RequestUri is null)
{
return TelemetryConstants.Unknown;
logRecord.Path = TelemetryConstants.Unknown;
return;
}

if (_routeParameterRedactionMode == HttpRouteParameterRedactionMode.None)
{
return request.RequestUri.AbsolutePath;
logRecord.Path = request.RequestUri.AbsolutePath;
return;
}

var requestMetadata = request.GetRequestMetadata() ??
Expand All @@ -143,19 +152,36 @@ private string GetRedactedPath(HttpRequestMessage request)

if (requestMetadata == null)
{
return TelemetryConstants.Redacted;
logRecord.Path = TelemetryConstants.Redacted;
return;
}

var route = requestMetadata.RequestRoute;
if (route == TelemetryConstants.Unknown)
{
return requestMetadata.RequestName;
logRecord.Path = requestMetadata.RequestName;
return;
}

return _outgoingPathLogMode switch
var routeSegments = _httpRouteParser.ParseRoute(route);

if (_outgoingPathLogMode == OutgoingPathLoggingMode.Formatted)
{
logRecord.Path = _routeFormatter.Format(in routeSegments, request.RequestUri.AbsolutePath, _routeParameterRedactionMode, _defaultSensitiveParameters);
logRecord.PathParameters = null;
}
else
{
OutgoingPathLoggingMode.Formatted => _routeFormatter.Format(route, request.RequestUri.AbsolutePath, _routeParameterRedactionMode, _defaultSensitiveParameters),
_ => route
};
// Case when logging mode is "OutgoingPathLoggingMode.Structured"
logRecord.Path = route;
var routeParams = ArrayPool<HttpRouteParameter>.Shared.Rent(routeSegments.ParameterCount);

// Setting this value right away to be able to return it back to pool in a callee's "finally" block:
logRecord.PathParameters = routeParams;
if (_httpRouteParser.TryExtractParameters(request.RequestUri.AbsolutePath, in routeSegments, _routeParameterRedactionMode, _defaultSensitiveParameters, ref routeParams))
{
logRecord.PathParametersCount = routeSegments.ParameterCount;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ internal static class Log
"An error occurred in enricher '{Enricher}' while enriching the logger context for request: " +
$"{{{HttpClientLoggingTagNames.Method}}} {{{HttpClientLoggingTagNames.Host}}}/{{{HttpClientLoggingTagNames.Path}}}";

private static readonly Func<LoggerMessageState, Exception?, string> _originalFormatValueFMTFunc = OriginalFormatValueFMT;
private static readonly Func<LoggerMessageState, Exception?, string> _originalFormatValueFMTFunc = OriginalFormatValueFmt;

public static void OutgoingRequest(ILogger logger, LogLevel level, LogRecord record)
{
Expand Down Expand Up @@ -115,7 +115,7 @@ public static void LoggerContextMissing(ILogger logger, Exception? exception, st
new(0, nameof(LoggerContextMissing)),
state,
exception,
(s, _) =>
static (s, _) =>
{
var requestState = s.TagArray[3].Value ?? NullString;
var method = s.TagArray[2].Value ?? NullString;
Expand All @@ -142,7 +142,7 @@ public static void EnrichmentError(ILogger logger, Exception exception, string?
new(0, nameof(EnrichmentError)),
state,
exception,
(s, _) =>
static (s, _) =>
{
var enricher = s.TagArray[4].Value ?? NullString;
var method = s.TagArray[3].Value ?? NullString;
Expand Down Expand Up @@ -171,7 +171,10 @@ private static void OutgoingRequest(
var requestHeadersCount = record.RequestHeaders?.Count ?? 0;
var responseHeadersCount = record.ResponseHeaders?.Count ?? 0;

var index = loggerMessageState.ReserveTagSpace(MinimalPropertyCount + statusCodePropertyCount + requestHeadersCount + responseHeadersCount);
var spaceToReserve = MinimalPropertyCount + statusCodePropertyCount + requestHeadersCount + responseHeadersCount +
record.PathParametersCount + (record.RequestBody is null ? 0 : 1) + (record.ResponseBody is null ? 0 : 1);
xakep139 marked this conversation as resolved.
Show resolved Hide resolved

var index = loggerMessageState.ReserveTagSpace(spaceToReserve);
loggerMessageState.TagArray[index++] = new(HttpClientLoggingTagNames.Method, record.Method);
loggerMessageState.TagArray[index++] = new(HttpClientLoggingTagNames.Host, record.Host);
loggerMessageState.TagArray[index++] = new(HttpClientLoggingTagNames.Path, record.Path);
Expand All @@ -192,14 +195,19 @@ private static void OutgoingRequest(
loggerMessageState.AddResponseHeaders(record.ResponseHeaders!, ref index);
}

if (record.PathParameters is not null)
{
loggerMessageState.AddPathParameters(record.PathParameters, record.PathParametersCount, ref index);
}

if (record.RequestBody is not null)
{
loggerMessageState.AddTag(HttpClientLoggingTagNames.RequestBody, record.RequestBody);
loggerMessageState.TagArray[index++] = new(HttpClientLoggingTagNames.RequestBody, record.RequestBody);
}

if (record.ResponseBody is not null)
{
loggerMessageState.AddTag(HttpClientLoggingTagNames.ResponseBody, record.ResponseBody);
loggerMessageState.TagArray[index] = new(HttpClientLoggingTagNames.ResponseBody, record.ResponseBody);
}

logger.Log(
Expand All @@ -215,7 +223,7 @@ private static void OutgoingRequest(
}
}

private static string OriginalFormatValueFMT(LoggerMessageState request, Exception? _)
private static string OriginalFormatValueFmt(LoggerMessageState request, Exception? _)
{
int startIndex = FindStartIndex(request);
var httpMethod = request[startIndex].Value;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers;
using System.Collections.Generic;
using System.Net.Http;
using Microsoft.Extensions.Http.Diagnostics;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.ObjectPool;

Expand Down Expand Up @@ -63,8 +65,24 @@ internal sealed class LogRecord : IResettable
/// </summary>
public LoggerMessageState? EnrichmentTags { get; set; }

/// <summary>
/// Gets or sets request path parameters.
/// </summary>
public HttpRouteParameter[]? PathParameters { get; set; }

/// <summary>
/// Gets or sets request path parameters count for <see cref="PathParameters"/>.
/// </summary>
public int PathParametersCount { get; set; }

public bool TryReset()
{
if (PathParameters != null)
{
ArrayPool<HttpRouteParameter>.Shared.Return(PathParameters);
PathParameters = null;
}

Host = string.Empty;
Method = null;
Path = string.Empty;
Expand All @@ -75,6 +93,7 @@ public bool TryReset()
EnrichmentTags = null;
RequestHeaders = null;
ResponseHeaders = null;
PathParametersCount = 0;
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.Http.Diagnostics;
using Microsoft.Extensions.Logging;

namespace Microsoft.Extensions.Http.Logging.Internal;
Expand All @@ -26,8 +27,7 @@ public static void AddRequestHeaders(this LoggerMessageState state, List<KeyValu
{
var key = _requestPrefixedNamesCache.GetOrAdd(
items[i].Key,
static (x, p) => p + Normalize(x),
HttpClientLoggingTagNames.RequestHeaderPrefix);
static x => HttpClientLoggingTagNames.RequestHeaderPrefix + Normalize(x));
xakep139 marked this conversation as resolved.
Show resolved Hide resolved

state.TagArray[index++] = new(key, items[i].Value);
}
Expand All @@ -46,17 +46,30 @@ public static void AddResponseHeaders(this LoggerMessageState state, List<KeyVal
{
var key = _responsePrefixedNamesCache.GetOrAdd(
items[i].Key,
static (x, p) => p + Normalize(x),
HttpClientLoggingTagNames.ResponseHeaderPrefix);
static x => HttpClientLoggingTagNames.ResponseHeaderPrefix + Normalize(x));

state.TagArray[index++] = new(key, items[i].Value);
}
}

/// <summary>
/// Adds path parameters to <see cref="LoggerMessageState"/>.
/// </summary>
/// <param name="state">A <see cref="LoggerMessageState"/> to be filled.</param>
/// <param name="parameters">An array with path parameters.</param>
/// <param name="paramsCount">A number of path parameters.</param>
/// <param name="index">Represents an index to be used when writing tags into <paramref name="state"/>.</param>
/// <remarks><paramref name="index"/> will be mutated to point to the next <paramref name="state"/> item.</remarks>
public static void AddPathParameters(this LoggerMessageState state, HttpRouteParameter[] parameters, int paramsCount, ref int index)
{
for (var i = 0; i < paramsCount; i++)
{
state.TagArray[index++] = new(parameters[i].Name, parameters[i].Value);
}
}

[SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase",
Justification = "Normalization to lower case is required by OTel's semantic conventions")]
private static string Normalize(string header)
{
return header.ToLowerInvariant();
}
=> header.ToLowerInvariant();
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
<PackageReference Include="Microsoft.Extensions.Http" />
<PackageReference Include="System.Text.Json" />
<PackageReference Include="System.Collections.Immutable" Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))" />
<PackageReference Include="System.Net.Http" Condition="'$(TargetFramework)' == 'net462'" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to update the implicit reference of 4.3.0 which is vulnerable

</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ internal sealed class HttpRouteFormatter : IHttpRouteFormatter
{
private const char ForwardSlashSymbol = '/';

#if NETCOREAPP3_1_OR_GREATER
#if NET6_0_OR_GREATER
private const char ForwardSlash = ForwardSlashSymbol;
#else
#pragma warning disable IDE1006 // Naming Styles
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,10 @@ internal interface IHttpRouteParser
/// <param name="parametersToRedact">Dictionary of parameters with their data classification that needs to be redacted.</param>
/// <param name="httpRouteParameters">Output array where parameters will be stored. Caller must provide the array with enough capacity to hold all parameters in route segment.</param>
/// <returns>Returns true if parameters were extracted successfully, return false otherwise.</returns>
#pragma warning disable CA1045 // Do not pass types by reference
bool TryExtractParameters(
string httpPath,
in ParsedRouteSegments routeSegments,
HttpRouteParameterRedactionMode redactionMode,
IReadOnlyDictionary<string, DataClassification> parametersToRedact,
ref HttpRouteParameter[] httpRouteParameters);
#pragma warning restore CA1045 // Do not pass types by reference
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
</PropertyGroup>

<ItemGroup>
<ProjectReference Include="..\..\Libraries\Microsoft.Extensions.DependencyInjection.AutoActivation\Microsoft.Extensions.DependencyInjection.AutoActivation.csproj" />
<ProjectReference Include="..\Microsoft.Extensions.DependencyInjection.AutoActivation\Microsoft.Extensions.DependencyInjection.AutoActivation.csproj" />
<ProjectReference Include="..\Microsoft.Extensions.AmbientMetadata.Application\Microsoft.Extensions.AmbientMetadata.Application.csproj" />
<ProjectReference Include="..\Microsoft.Extensions.Compliance.Abstractions\Microsoft.Extensions.Compliance.Abstractions.csproj" />
<ProjectReference Include="..\Microsoft.Extensions.Telemetry.Abstractions\Microsoft.Extensions.Telemetry.Abstractions.csproj" />
Expand Down
Loading