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

[API Proposal]: HttpClient telemetry extensibility #103130

Open
antonfirsov opened this issue Jun 6, 2024 · 6 comments
Open

[API Proposal]: HttpClient telemetry extensibility #103130

antonfirsov opened this issue Jun 6, 2024 · 6 comments
Assignees
Milestone

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Jun 6, 2024

Background and motivation

There are 2 feature requests for .NET 9 which require configuring HttpClient telemetry and logging behavior on per-request basis:

  1. To address concerns described in [API Proposal]: Mitigate the sources of privacy and compliance issues by providing and using safe-to-log URIs and safe-to-log exceptions #93221, we need to introduce a Uri redaction mechanism that allows redacting PII from the url-s emitted by Distributed Tracing, EventSource events and HttpClientFactory loggers and potentially in Metrics.
  2. .NET 9.0 Native Trace Instrumentation Support for HttpClient as per OTel specification #93019 requires us to introduce enrichment and filtering capabilities for HttpClient's Distributed Tracing. It seems reasonable to match MetricsEnrichmentContext functionality enabling the registration of enricher and filtering callbacks per-request, enabling various extension points (eg. handlers) to add their own callbacks independently.

Besides redaction, in the future we may want to implement a url templatization mechanism so we can emit url.template for metrics and activities where the template has to be set on a per-request basis.

We could introduce distinct utility methods for each feature like we did when we added MetricsEnrichmentContext.AddCallback. However, this would lead to API friction and poor discoverability.

Instead, I'm proposing a new type, HttpRequestDiagnosticOptions to encapsulate all settings that allow customizing telemetry behavior on a per-request basis. I'm also proposing to move the existing metrics enrichment callback registration functionality to the new type.

API Proposal

namespace System.Net.Http;

// The new type to encapsulate telemetry customization options.
public sealed class HttpRequestDiagnosticOptions
{
    // Enable handlers to append new callbacks for enrichment.
    public void AddActivityEnrichmentCallback(Action<HttpActivityEnrichmentContext> callback);

    // Follow the same pattern for filters, however we may consider to get a way with a single filter.
    public void AddActivityFilter(Predicate<HttpRequestMessage> filter);
    // public Predicate<HttpRequestMessage>? ActivityFilter { get; set; } // ???

    // Fully customizable Uri redaction mechanism that might be used HttpClientFactory and 3rd party libs.
    // Shall we pass HttpRequestMessage instead of Uri?
    public Func<Uri, string?>? UriRedactorCallback { get; set; }

    // Move MetricsEnrichmentContext.AddCallback() functionality into the new type.
    public void AddMetricsEnrichmentCallback(Action<Metrics.HttpMetricsEnrichmentContext> callback);
}

// Enrichment happens when a response is received or an exception is thrown.
public sealed class HttpActivityEnrichmentContext
{
    private HttpActivityEnrichmentContext() { }

    // Expose the activity so users can add or potentially modify tags.
    public Activity Activity { get; } }
    public HttpRequestMessage Request { get; }
    public HttpResponseMessage? Response { get; }
    public Exception? Exception { get; }
}

// Expose the diagnostic options through a new property on the request object
public class HttpRequestMessage
{
    public HttpRequestDiagnosticOptions DiagnosticOptions { get; }
}

namespace System.Net.Http.Metrics;

// To harmonize the API, move the metrics enrichment callback registration functionality from HttpMetricsEnrichmentContext to HttpRequestDiagnosticOptions.
public sealed class HttpMetricsEnrichmentContext
{
    // We may even consider obsoletion.
    // [Obsolete("Use HttpRequestDiagnosticOptions.MetricsEnrichmentCallbacks.")]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public static void AddCallback(HttpRequestMessage request, Action<HttpMetricsEnrichmentContext> callback);
}

API Usage

1. Redacting Uri-s

If redirects are not expected, the application can configure a constant placeholder string to log instead of the actual Uri:

using HttpClient client = new();
using HttpRequestMessage request = new(HttpMethod.Get, "https://aa.org/u/231");
request.DiagnosticOptions.UriRedactorCallback = _ => "https://aa.org/u/REDACTED";
await client.SendAsync(request);

One possible way to handle redirects:

using HttpClient client = new();
using HttpRequestMessage request = new(HttpMethod.Get, "https://aa.org/u/321");
request.DiagnosticOptions.UriRedactorCallback = uri =>
{
    if (uri.Host == "aa.org" && uri.LocalPath.StartsWith("/u/"))
    {
        return "https://aa.org/u/REDACTED";
    }

    return "REDIRECTED";
};

2. Activity enrichment

using HttpClient client = new();
using HttpRequestMessage request = new("https://aa.org/u/321");
request.DiagnosticOptions.AddActivityEnrichmentCallback(ctx =>
{
    ctx.Activity.AddTag("url.template", "https://aa.org/u/{userid}");
});
using HttpResponseMessage response = await client.SendAsync(request);

3. Activity filtering

[TODO]

4. Metrics enrichment

[TODO]

Alternative Designs

There are several aspects where we could consider alternative apporoaches.

1. Overall API shape

  • Instead of introducing HttpRequestDiagnosticOptions expose utility methods.
  • Do not introduce strongly-typed API-s for anything but activity enrichment. Use well-known option names for the rest.

2. Expose callback collections via IList<T> properties instead of Add*Callback() methods:

public IList<Action<HttpActivityEnrichmentContext>> ActivityEnrichmentCallbacks { get; }

This would make it more difficult to expose additional enrichment callbacks registration when the request is being sent. With the current method approach we can do the following:

public enum EnrichmentTiming
{
    RequestStart,
    RequestEnd
}

public sealed class HttpRequestDiagnosticOptions
{
    // New overload to control the enrichment timing.
    public void AddActivityEnrichmentCallback(Action<HttpActivityEnrichmentContext> callback, EnrichmentTiming timing);
}

3. Instead of UriRedactorCallback, expose pre-defined redaction modes.

This might be easier to use for the most common use-cases, but less flexible. Eg. it wouldn't allow users to handle redirects.

public enum UriRedactionMode
{
    EmitWithoutQueryString,
    EmitFull,
    DoNotEmit,
    EmitPlaceholderString
}

public sealed class HttpRequestDiagnosticOptions
{
    public UriRedactionMode UriRedactionMode { get; set; }
    public string? PlaceholderString { get; set; }
}

Risks

  • UriRedactionCallback turns out to be too difficult to use
  • We would need to implement Activity enrichment also for request start
  • Users would start to use activity enrichment for general Activity manipulation tasks, so it wouldn't be "enrichment" any longer.
@antonfirsov antonfirsov added this to the 9.0.0 milestone Jun 6, 2024
@antonfirsov antonfirsov self-assigned this Jun 6, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@julealgon
Copy link

@antonfirsov instead of a "list of callbacks" or AddCallback methods, why not leverage IObservable instead?

@martincostello
Copy link
Member

I know the examples are just that, but they made me consider something, so I'm going to mention it here for something to consider towards the design.

The examples use hard-coded URLs, so the conditional filters/templates etc are also specified inline to do the redaction using the same hard-coded values. For applications where the host is configured through IConfiguration, it should be relatively simple to match a request URI against resolved configuration/options. Sometimes callback-based APIs cause friction here due to lack of access to the service provider or needing to capture state into it because of that.

@antonfirsov
Copy link
Member Author

For applications where the host is configured through IConfiguration, it should be relatively simple to match a request URI against resolved configuration/options. Sometimes callback-based APIs cause friction

Wouldn't that matching be also done by a callback? Or would you expect the HttpClientFactory middleware to implement it? Can you elaborate on your idea with some examples?

@martincostello
Copy link
Member

So with OpenTelemetry as an example, the enrichment is typically configured as part of the IoC registrations, and the IServiceProvider is available to be able to reach into the configuration to do matching, and knowledge of OpenTelemetry is kept out of the knowledge of the callsite of the HttpClient itself.

Essentially, anything regarding DiagnosticOptions would be configured via HttpClientFactory and transparent to the consuming code. Something similar to the below is what I'm thinking of:

Configuration:

services.AddHttpClient("MyClient", (client, serviceProvider) =>
{
    var configuration = serviceProvider.GetRequiredService<IConfiguration>();
    client.BaseAddress = new(configuration["MyClientBaseAddress"]);
}).ConfigureDiagnosticOptions((options, serviceProvider) =>
{
    // This example is just illustrative - as written it's not very efficient or well-named
    options.UriRedactorCallback = uri =>
    {
        // If IConfiguration supports being reloaded remotely, it needs to be retrieved per-request.
        // A better approach would be via IOptionsMonitor<T> with bound configuration, for example.
        var configuration = serviceProvider.GetRequiredService<IConfiguration>();
        var baseAddress = new(configuration["MyClientBaseAddress"])
        if (uri.Host == baseAddress.Host && uri.LocalPath.StartsWith("/u/"))
        {
            return $"{baseAddress}/u/REDACTED";
        }

        return "REDIRECTED";
    };
});

Usage:

using HttpClient client = httpClientFactory.CreateClient("MyClient");
using HttpRequestMessage request = new(HttpMethod.Get, "/u/321");
using HttpResponseMessage response = await client.SendAsync(request);

@antonfirsov
Copy link
Member Author

There was a strong pushback against the proposed API shape because the way it's tied to a request. We were not able to gather enough information on time to create a better design everyone will be happy about. Pushing this to .NET 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants