[Question] Why does AspNetCore instrumentation documentation suggests adding custom attributes through Enrich hook instead of directly updating activity in middleware?

[jundayin]

Component

OpenTelemetry.Instrumentation.AspNetCore

Question details

In the documentation we were told to add custom attributes through enrich hook in the following format

services.AddOpenTelemetry()
    .WithTracing(builder => builder
        .AddAspNetCoreInstrumentation(o =>
        {
            o.EnrichWithHttpRequest = (activity, httpRequest) =>
            {
                activity.SetTag("requestProtocol", httpRequest.Protocol);
            };
            o.EnrichWithHttpResponse = (activity, httpResponse) =>
            {
                activity.SetTag("responseLength", httpResponse.ContentLength);
            };
            o.EnrichWithException = (activity, exception) =>
            {
                if (exception.Source != null)
                {
                    activity.SetTag("exception.source", exception.Source);
                }
            };
        }));

In my code I need to read the request body and set one of the fields as part of the custom attributes. Basically I do something like

o.EnrichWithHttpRequest = (activity, httpRequest) =>
{
    httpRequest.EnableBuffering();
    try
    {
        var buffer = new byte[Convert.ToInt32(httpRequest.ContentLength)];
        httpRequest.Body.ReadAsync(buffer, 0, buffer.Length);
        var requestContent = Encoding.UTF8.GetString(buffer);
        var policyRequest = JsonConvert.DeserializeObject<PolicyRequest>(requestContent);
        string policyNamespace = policyRequest.Namespace;
        activity.SetTag(TelemetryConstants.PolicyNamespaceKey, policyNamespace);
    }
    catch (Exception e)
    {
        Console.WriteLine($"Failed to process policy namespace. Exception: {e.Message}");
    }
    finally
    {
        httpRequest.Body.Position = 0;
    }
};

This is not a thread-safe approach and apparently it breaks a lot of inbound requests. Later I realize since we only need to update the acitivity tag directly, maybe I can get the Acitivity directly in the middleware

public class TestMiddleware
{
    public async Task InvokeAsync(HttpContext httpContext) 
    {
        --- get the namespace from request body ---
        Activity.Current?.AddTag(TelemetryConstants.PolicyNamespaceKey, policyNamespace);
}

Although it works, I don't know if this has any issue that I'm not aware. That makes me curious why the documentation only mentions the hook approach

[cijothomas]

Not sure what is the thread-safety issue. However, if your goal is to do http logging, its better to do it via ILogger, and is already natively supported by asp.net core.

Using Activity.Current works equally well, no harm using it. A lot of people don't like to mix up their middlewares/controllers with Telemetry logic and instead prefer to do it in central place. But no issues if you want to do it in middleware directly.