[BUG] AzureMonitorExporter marks 404 response as errors, even if explicitly marked as successful in OpenTelemetry

[rvanheest]

Library name and version

Azure.Monitor.OpenTelemetry.Exporter

Describe the bug

My webapi yields a 404 response when the requested data cannot be found. In my case, this is not supposed to be marked as an error when exporting the OpenTelemetry data to ApplicationInsights.

As suggested in various other places, I have added an EnrichWithHttpResponse to the AddAspNetCoreInstrumentation to manually change the activity's status to ActivityStatusCode.Ok.

builder
    .AddAspNetCoreInstrumentation(options =>
    {
        options.EnrichWithHttpResponse = (activity, response) =>
        {
            if (response.StatusCode == 404)
            {
                activity.SetStatus(ActivityStatusCode.Ok);
            }
        };
    })
    .AddConsoleExporter()
    .AddAzureMonitorTraceExporter(options =>
    {
        options.ConnectionString = "<my connection string>";
    });

However, I'm still getting my 404 responses marked as errors in ApplicationInsights, even though the console exporter clearly shows the status of the activity to be marked as successful.

Expected behavior

I would expect the manually set activity.Status to be respected here, rather than it being overridden by interpreting the response code. It would still make sense to look at the response code when the value of activity.Status is ActivityStatusCode.Unset, but not when the activity.Status is set to AcitivityStatusCode.Ok.

Actual behavior

In the ApplicationInsights exporter, the Activity containing the 404 response is transformed into a RequestData, where the Success property is determined by the IsSuccess function.

azure-sdk-for-net/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RequestData.cs

Lines 54 to 67 in a18e701

	internal static bool IsSuccess(Activity activity, string? responseCode, OperationType operationType)
	{
	if (operationType.HasFlag(OperationType.Http)
	&& responseCode != null
	&& int.TryParse(responseCode, out int statusCode))
	{
	bool isSuccessStatusCode = statusCode != 0 && statusCode < 400;
	return activity.Status != ActivityStatusCode.Error && isSuccessStatusCode;
	}
	else
	{
	return activity.Status != ActivityStatusCode.Error;
	}
	}

Since the operationType is an http call and the response code is set to 404, the if-branch applies, evaluating the value for isSuccessStatusCode to false and making the function return false, even though activity.Status != ActivityStatusCode.Error evaluates to true.

In this IsSuccess function is where I would expect the predetermined activity.Status to be respected. Perhaps the function should be something like

internal static bool IsSuccess(Activity activity, string? responseCode, OperationType operationType)
{
    if (activity.Status == ActivityStatusCode.Unset
        && operationType.HasFlag(OperationType.Http)
        && responseCode != null
        && int.TryParse(responseCode, out int statusCode))
    {
        return statusCode != 0 && statusCode < 400;
    }
    else
    {
        return activity.Status != ActivityStatusCode.Error;
    }
}

Reproduction Steps

See bug description

Environment

No response

[github-actions]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @cijothomas @rajkumar-rangaraj @reyang @TimothyMothra @vishweshbankwar.

[github-actions]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @cijothomas @rajkumar-rangaraj @reyang @TimothyMothra @vishweshbankwar.

[rvanheest]

Any updates on this? Not sure what the process/next step is. Do you want me to create a PR for this with my proposed solution?

[vishweshbankwar]

@rvanheest - Apologies for the delay. I think the change you have suggested is reasonable. Would you be able to create the PR for this?

[rvanheest]

@vishweshbankwar sure, here you go - #43594