Adding ProblemDetailsService #42384

brunolins16 · 2022-06-23T22:15:55Z

Close #42212

src/Http/Http/src/Builder/ApplicationBuilder.cs

Co-authored-by: Brennan <brecon@microsoft.com>

src/Http/Http.Abstractions/src/ProblemDetails/ProblemDetailsContext.cs

src/Http/Http.Extensions/src/ProblemDetailsDefaultWriter.cs

davidfowl · 2022-07-12T14:22:29Z

What does the performance of this look like? I see context objects and state machines that can't be avoided?

brunolins16 · 2022-07-12T17:31:08Z

What does the performance of this look like? I see context objects and state machines that can't be avoided?

I was expecting this question :) ... I ran crank and the results are here:
https://gist.github.com/brunolins16/5cc2286ac500706150ea4a957c9375b2

The benchmark was against a simple app.MapGet("statuscode", () => Results.StatusCode(415)); and, in general, adding the StatusCodePages degraded considerably the bad requests performance (it does not affect success requests), also, adding ProblemDetails, we end up with better memory allocation and almost the same RPS.

Also, having the default writers (when calling AddControllers) did not affect the numbers.

ExceptionHandler middleware performance is better but using ProblemsDetails increase a little bit the memory allocation with almost the same RPS.

I will open an issue to work on improvements of those middleware.

halter73

Looks good!

If we're always going to return HasStarted from TryWriteAsync, we might as well have it not even return a bool. I think it's fine to assume WriteAsync methods did in fact write something in our IProblemDetailsWriter implementations.

src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj

src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

src/Middleware/MiddlewareAnalysis/test/MiddlewareAnalysisTests.cs

src/Mvc/Mvc.Core/src/Infrastructure/DefaultApiProblemDetailsWriter.cs

Co-authored-by: Stephen Halter <halter73@gmail.com>

BrennanConroy · 2022-07-13T23:30:36Z

src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

+        var httpContext = context.HttpContext;
+        var acceptHeader = httpContext.Request.Headers.Accept.GetList<MediaTypeHeaderValue>();
+
+        if (acceptHeader?.Any(h => _jsonMediaType.IsSubsetOf(h) || _problemDetailsJsonMediaType.IsSubsetOf(h)) == true)


This lambda probably allocates per call, we should hand write .Any

brunolins16 · 2022-07-14T06:57:15Z

/backport to release/7.0-preview7

ghost · 2022-07-14T06:57:20Z

Hi @brunolins16. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

github-actions · 2022-07-14T06:57:29Z

Started backporting to release/7.0-preview7: https://github.com/dotnet/aspnetcore/actions/runs/2668518163

github-actions · 2022-07-14T06:57:58Z

@brunolins16 backporting to release/7.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: MVC Changes
Applying: ExceptionHandler changes
.git/rebase-apply/patch:126: trailing whitespace.
    /// 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	src/Middleware/Diagnostics/src/PublicAPI.Unshipped.txt
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/Diagnostics/src/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Middleware/Diagnostics/src/PublicAPI.Unshipped.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 ExceptionHandler changes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

* MVC Changes * ExceptionHandler changes * Routing changes * Http.Extensions changes * Minimal APi draft changes * HttpResults changes * New ProblemDetails project * Using ProblemDetailsOptions in mvc * ProblemDetails project simplification * Using metadata * Using metadata * Latest version * Removing changes * Initial cleanup * Clean up * Public api clean up * Updating public API * More clean ups * Adding initial unit tests * Updating unit tests * Adding more unit tests * Cleanup * Clean up * clean up * clean up * Removing nullable * Simplifying public api * API Review feedback * API review feedback * Clean up * Clean up * clean up * Reusing Endpoint & EndpointMetadataCollection * Fix build issues * Updates based on docs/Trimming.md * Adding Functional tests * Fix unittest * Seal context * Seal ProblemMetadata * PR Feeback * API Review * Clean up * Fixing publicapi warnings * Clean up * PR Feedback * Fixing build * Fix unit test * Fix unit tests * PR review * Fixing JsonSerializationContext issues * Adding statuscode 405 * Update src/Http/Http.Extensions/src/ProblemDetailsDefaultWriter.cs Co-authored-by: Brennan <brecon@microsoft.com> * PR review * Apply suggestions from code review Co-authored-by: Stephen Halter <halter73@gmail.com> * Fixing bad merge * Fix bad merge * Adding analysis.NextMiddlewareName * PR review * Changing to CanWrite/WriteAsync Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Stephen Halter <halter73@gmail.com>

* MVC Changes * ExceptionHandler changes * Routing changes * Http.Extensions changes * Minimal APi draft changes * HttpResults changes * New ProblemDetails project * Using ProblemDetailsOptions in mvc * ProblemDetails project simplification * Using metadata * Using metadata * Latest version * Removing changes * Initial cleanup * Clean up * Public api clean up * Updating public API * More clean ups * Adding initial unit tests * Updating unit tests * Adding more unit tests * Cleanup * Clean up * clean up * clean up * Removing nullable * Simplifying public api * API Review feedback * API review feedback * Clean up * Clean up * clean up * Reusing Endpoint & EndpointMetadataCollection * Fix build issues * Updates based on docs/Trimming.md * Adding Functional tests * Fix unittest * Seal context * Seal ProblemMetadata * PR Feeback * API Review * Clean up * Fixing publicapi warnings * Clean up * PR Feedback * Fixing build * Fix unit test * Fix unit tests * PR review * Fixing JsonSerializationContext issues * Adding statuscode 405 * Update src/Http/Http.Extensions/src/ProblemDetailsDefaultWriter.cs Co-authored-by: Brennan <brecon@microsoft.com> * PR review * Apply suggestions from code review Co-authored-by: Stephen Halter <halter73@gmail.com> * Fixing bad merge * Fix bad merge * Adding analysis.NextMiddlewareName * PR review * Changing to CanWrite/WriteAsync Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Stephen Halter <halter73@gmail.com> Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Stephen Halter <halter73@gmail.com>

brunolins16 added 29 commits June 1, 2022 11:41

MVC Changes

15126b5

ExceptionHandler changes

00e7615

Routing changes

9adc269

Http.Extensions changes

c430ae2

Minimal APi draft changes

a8939e5

HttpResults changes

79690d8

New ProblemDetails project

c548e2b

Using ProblemDetailsOptions in mvc

7f368a8

ProblemDetails project simplification

acb590b

Using metadata

62eb53e

Using metadata

9618302

Merge branch 'main' into brunolins16/issues/32957

a747f04

Latest version

8e25910

Removing changes

22b0932

Initial cleanup

f6a5c37

Merge branch 'main' into brunolins16/issues/32957

b2fabfb

Clean up

c13a1bd

Public api clean up

80aa845

Updating public API

e0d4333

More clean ups

3fa6af0

Adding initial unit tests

89fd8ec

Updating unit tests

dca5c73

Adding more unit tests

e885b3a

Merge branch 'main' into brunolins16/issues/32957

a593c2b

Cleanup

9877120

Clean up

2e0c98e

clean up

e8f0e1a

clean up

6316af7

Removing nullable

0ed099e

davidfowl reviewed Jun 24, 2022

View reviewed changes

src/Http/Http/src/Builder/ApplicationBuilder.cs Outdated Show resolved Hide resolved

brunolins16 and others added 2 commits July 11, 2022 18:36

Adding statuscode 405

35de3fe

Update src/Http/Http.Extensions/src/ProblemDetailsDefaultWriter.cs

1ba1f10

Co-authored-by: Brennan <brecon@microsoft.com>

Kahbazi reviewed Jul 12, 2022

View reviewed changes

src/Http/Http.Abstractions/src/ProblemDetails/ProblemDetailsContext.cs Outdated Show resolved Hide resolved

src/Http/Http.Extensions/src/ProblemDetailsDefaultWriter.cs Outdated Show resolved Hide resolved

PR review

2455ccb

brunolins16 mentioned this pull request Jul 12, 2022

Profile and performance test Diagnostics middleware #42700

Open

halter73 approved these changes Jul 12, 2022

View reviewed changes

brunolins16 and others added 7 commits July 12, 2022 15:32

Apply suggestions from code review

cd92e30

Co-authored-by: Stephen Halter <halter73@gmail.com>

Merge branch 'main' into brunolins16/issues/32957

d35ff2b

Fixing bad merge

55cb7da

Fix bad merge

8585070

Adding analysis.NextMiddlewareName

2c12741

PR review

4b84cf3

Changing to CanWrite/WriteAsync

1fe3f3b

BrennanConroy approved these changes Jul 13, 2022

View reviewed changes

brunolins16 merged commit ac67d32 into dotnet:main Jul 14, 2022

ghost added this to the 7.0-rc1 milestone Jul 14, 2022

brunolins16 mentioned this pull request Jul 15, 2022

Backport/pr 42384 to release/7.0 preview7 #42741

Merged

10 tasks

brunolins16 mentioned this pull request Jul 19, 2022

Backport/pr 42384 to release/7.0 preview7 #42809

Merged

10 tasks

This was linked to issues Jul 19, 2022

ProblemDetails is not returned for 404NotFound and 500Exception #4953

Closed

Usage of ProblemDetails is inconsistent throughout ASP.NET Core #32957

Closed

brunolins16 mentioned this pull request Aug 2, 2022

Remove .Any lambda per call allocation from DefaultProblemDetailsWriter #43046

Closed

brunolins16 deleted the brunolins16/issues/32957 branch August 2, 2022 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ProblemDetailsService #42384

Adding ProblemDetailsService #42384

brunolins16 commented Jun 23, 2022 •

edited

Loading

davidfowl commented Jul 12, 2022

brunolins16 commented Jul 12, 2022 •

edited

Loading

halter73 left a comment

BrennanConroy Jul 13, 2022

brunolins16 commented Jul 14, 2022

ghost commented Jul 14, 2022

github-actions bot commented Jul 14, 2022

github-actions bot commented Jul 14, 2022

Adding ProblemDetailsService #42384

Adding ProblemDetailsService #42384

Conversation

brunolins16 commented Jun 23, 2022 • edited Loading

davidfowl commented Jul 12, 2022

brunolins16 commented Jul 12, 2022 • edited Loading

halter73 left a comment

Choose a reason for hiding this comment

BrennanConroy Jul 13, 2022

Choose a reason for hiding this comment

brunolins16 commented Jul 14, 2022

ghost commented Jul 14, 2022

github-actions bot commented Jul 14, 2022

github-actions bot commented Jul 14, 2022

brunolins16 commented Jun 23, 2022 •

edited

Loading

brunolins16 commented Jul 12, 2022 •

edited

Loading