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

Only determine success of request based on statuscode when the Activity Status is Unset #43594

Merged

Conversation

rvanheest
Copy link
Contributor

Fixes #41993

When a non-2xx response from a web request should not be marked as an error in OpenTelemetry, you can set the Activity's status to ActivityStatusCode.Ok manually.

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

However, even though the status was set to Ok, the ApplicationInsights exporter still treats it like an error because of the response code being >= 400. This should only happen in cases where activity.Status == ActivityStatusCode.Unset.

Added to the RequestData.IsSuccess function is this extra check. Also, inside the if now the expression activity.Status != ActivityStatusCode.Error becomes always true and can hence be removed.

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor - Exporter Monitor OpenTelemetry Exporter labels Apr 23, 2024
Copy link

Thank you for your contribution @rvanheest! We will review the pull request and get back to you soon.

@rvanheest
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Contributor

@TimothyMothra TimothyMothra left a comment

Choose a reason for hiding this comment

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

LGTM. Need to update changelog.

@rvanheest
Copy link
Contributor Author

LGTM. Need to update changelog.

@TimothyMothra updated changelog

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

Thanks @rvanheest!

Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com>
@TimothyMothra TimothyMothra enabled auto-merge (squash) April 24, 2024 19:59
@TimothyMothra TimothyMothra merged commit f906727 into Azure:main Apr 24, 2024
17 checks passed
@rvanheest rvanheest deleted the feature/determine-success-with-status branch April 25, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor - Exporter Monitor OpenTelemetry Exporter
Projects
None yet
4 participants