Add OpenAPI operation and schema transformer interfaces #56395
Conversation
dotnet-issue-labeler
bot
added
the
old-area-web-frameworks-do-not-use
martincostello
added
feature-openapi
and removed
old-area-web-frameworks-do-not-use
dotnet-policy-service
bot
added
the
community-contribution
martincostello
added
the
old-area-web-frameworks-do-not-use
There was a problem hiding this comment.
I haven't run these yet.
There was a problem hiding this comment.
Having trouble running these locally. I've run restore.cmd, done . activate.ps1 and run eng\build.cmd, but trying to dotnet run these benchmarks I just get this:
❯ dotnet run --configuration Release --framework net9.0
C:\Coding\dotnet\aspnetcore\src\Servers\IIS\IIS\src\Microsoft.AspNetCore.Server.IIS.csproj(41,5): error : Required dll from ANCM has not been built. To build ANCM, you must use MSBuild.exe.
The build failed. Fix the build errors and run again.
There was a problem hiding this comment.
Ooof -- I usually develop on macOS/Linux so I don't run into the IIS-specific issues with the build. 😅
One thing I would check is if you have the C++ Components installed in Visual Studio. Then, perhaps try doing a build in the src/Servers directory?
I'll try running them locally as well to see what we can get here.
There was a problem hiding this comment.
I'll try running them from WSL on my laptop.
There was a problem hiding this comment.
I'll have to try again another day - on WSL I'm getting issues from Node trying to build Blazor, which the build fails and complains about if it's not been produced.
There was a problem hiding this comment.
Welp -- I had written up this comment alongside my earlier review and then GitHub played me and didn't post it. Let me know if you are seeing similar numbers after making the change to the ActivatedTransformer scenarios.
There was a problem hiding this comment.
No problem! I’ll re-run them tomorrow and see what happens. For whatever reason they seem to take A While™️ to run on my little-laptop-that-could.
There was a problem hiding this comment.
Here's what I get in WSL with the latest changes:
BenchmarkDotNet=v0.13.0, OS=ubuntu 22.04
12th Gen Intel Core i7-1270P, 1 CPU, 16 logical and 8 physical cores
.NET SDK=9.0.100-preview.7.24358.6
[Host] : .NET 9.0.0 (9.0.24.35702), X64 RyuJIT
Job-RWQJNC : .NET 9.0.0 (9.0.24.35702), X64 RyuJIT
Server=True Toolchain=.NET Core 9.0 RunStrategy=Throughput
| Method | TransformerCount | Mean | Error | StdDev | Median | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|
| ActivatedOperationTransformer | 10 | 6.511 μs | 0.2847 μs | 0.8167 μs | 6.520 μs | 153,594.6 | 0.1755 | 0.0381 | - | 7 KB |
| OperationTransformerAsDelegate | 10 | 5.382 μs | 0.3163 μs | 0.8871 μs | 5.228 μs | 185,804.4 | 0.0076 | - | - | 6 KB |
| ActivatedDocumentTransformer | 10 | 6.532 μs | 0.3649 μs | 1.0703 μs | 6.324 μs | 153,103.4 | - | - | - | 7 KB |
| DocumentTransformerAsDelegate | 10 | 5.602 μs | 0.2916 μs | 0.8505 μs | 5.667 μs | 178,521.0 | 0.0076 | - | - | 6 KB |
| ActivatedSchemaTransformer | 10 | 71.136 μs | 1.7172 μs | 5.0093 μs | 70.855 μs | 14,057.7 | 1.4648 | - | - | 56 KB |
| SchemaTransformerAsDelegate | 10 | 76.440 μs | 3.2091 μs | 9.2590 μs | 75.209 μs | 13,082.1 | 1.4648 | - | - | 55 KB |
| ActivatedOperationTransformer | 100 | 10.824 μs | 0.9000 μs | 2.4789 μs | 10.920 μs | 92,389.4 | 0.2289 | 0.0458 | - | 9 KB |
| OperationTransformerAsDelegate | 100 | 8.029 μs | 0.4858 μs | 1.3939 μs | 7.804 μs | 124,550.7 | - | - | - | 6 KB |
| ActivatedDocumentTransformer | 100 | 9.658 μs | 0.5128 μs | 1.4712 μs | 9.669 μs | 103,541.0 | 0.2365 | 0.0534 | - | 9 KB |
| DocumentTransformerAsDelegate | 100 | 7.948 μs | 0.3417 μs | 0.9694 μs | 8.019 μs | 125,824.2 | - | - | - | 6 KB |
| ActivatedSchemaTransformer | 100 | 77.444 μs | 1.7093 μs | 5.0132 μs | 77.709 μs | 12,912.5 | 0.4883 | - | - | 66 KB |
| SchemaTransformerAsDelegate | 100 | 71.226 μs | 1.7780 μs | 5.2425 μs | 71.005 μs | 14,039.9 | 1.4648 | - | - | 61 KB |
| ActivatedOperationTransformer | 1000 | 69.732 μs | 2.8724 μs | 8.2875 μs | 66.498 μs | 14,340.7 | 0.7935 | 0.1831 | - | 30 KB |
| OperationTransformerAsDelegate | 1000 | 16.873 μs | 0.8374 μs | 2.4691 μs | 16.685 μs | 59,267.5 | - | - | - | 6 KB |
| ActivatedDocumentTransformer | 1000 | 66.275 μs | 1.2815 μs | 2.6750 μs | 65.474 μs | 15,088.7 | 0.7324 | 0.1221 | - | 30 KB |
| DocumentTransformerAsDelegate | 1000 | 20.716 μs | 1.2154 μs | 3.5455 μs | 19.535 μs | 48,272.1 | 0.1526 | 0.0305 | - | 6 KB |
| ActivatedSchemaTransformer | 1000 | 262.090 μs | 8.0333 μs | 23.4336 μs | 257.450 μs | 3,815.5 | 3.9063 | - | - | 164 KB |
| SchemaTransformerAsDelegate | 1000 | 146.933 μs | 5.4175 μs | 15.8032 μs | 148.187 μs | 6,805.8 | 0.9766 | - | - | 117 KB |
There was a problem hiding this comment.
Did another run on my end against 28acf62. The numbers were seeing have some variance but tell the same story about the cost of activation, particularly for schema transformers which get called more frequently in the pipeline.
| Method | TransformerCount | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| ActivatedOperationTransformer | 10 | 2.466 μs | 0.0377 μs | 0.0477 μs | 405,514.3 | 0.0534 | 0.0114 | - | 7 KB |
| OperationTransformerAsDelegate | 10 | 2.561 μs | 0.0195 μs | 0.0173 μs | 390,494.5 | 0.0496 | 0.0114 | - | 6 KB |
| ActivatedDocumentTransformer | 10 | 2.462 μs | 0.0369 μs | 0.0288 μs | 406,209.9 | 0.0534 | 0.0114 | - | 7 KB |
| DocumentTransformerAsDelegate | 10 | 2.399 μs | 0.0463 μs | 0.0455 μs | 416,841.0 | 0.0496 | 0.0114 | - | 6 KB |
| ActivatedSchemaTransformer | 10 | 31.617 μs | 0.4569 μs | 0.4274 μs | 31,628.4 | 0.3662 | - | - | 56 KB |
| SchemaTransformerAsDelegate | 10 | 30.972 μs | 0.6155 μs | 0.6320 μs | 32,287.4 | 0.3662 | - | - | 55 KB |
| ActivatedOperationTransformer | 100 | 4.082 μs | 0.0557 μs | 0.0494 μs | 244,954.8 | 0.0687 | 0.0153 | - | 9 KB |
| OperationTransformerAsDelegate | 100 | 3.250 μs | 0.0595 μs | 0.0497 μs | 307,655.7 | 0.0458 | 0.0076 | - | 6 KB |
| ActivatedDocumentTransformer | 100 | 4.149 μs | 0.0428 μs | 0.0358 μs | 241,048.5 | 0.0687 | 0.0153 | - | 9 KB |
| DocumentTransformerAsDelegate | 100 | 3.241 μs | 0.0258 μs | 0.0215 μs | 308,574.1 | 0.0496 | 0.0114 | - | 6 KB |
| ActivatedSchemaTransformer | 100 | 38.633 μs | 0.2470 μs | 0.2190 μs | 25,884.8 | 0.4883 | - | - | 66 KB |
| SchemaTransformerAsDelegate | 100 | 35.940 μs | 0.4064 μs | 0.3802 μs | 27,824.2 | 0.4883 | - | - | 62 KB |
| ActivatedOperationTransformer | 1000 | 34.187 μs | 0.4856 μs | 0.4543 μs | 29,251.0 | 0.2441 | - | - | 30 KB |
| OperationTransformerAsDelegate | 1000 | 11.128 μs | 0.0788 μs | 0.0658 μs | 89,863.8 | 0.0458 | 0.0153 | - | 6 KB |
| ActivatedDocumentTransformer | 1000 | 36.648 μs | 0.6589 μs | 0.8332 μs | 27,286.5 | 0.2441 | - | - | 30 KB |
| DocumentTransformerAsDelegate | 1000 | 12.282 μs | 0.0807 μs | 0.0674 μs | 81,422.7 | 0.0458 | 0.0153 | - | 6 KB |
| ActivatedSchemaTransformer | 1000 | 145.059 μs | 1.1951 μs | 0.9980 μs | 6,893.7 | 1.2207 | 0.2441 | - | 164 KB |
| SchemaTransformerAsDelegate | 1000 | 72.551 μs | 0.4466 μs | 0.3959 μs | 13,783.4 | 0.7324 | - | - | 117 KB |
This comment was marked as outdated.
This comment was marked as outdated.
martincostello
force-pushed
the
gh-56022-operation-interceptor
branch
from
feea044 to
ed43c21
Compare
martincostello
force-pushed
the
gh-56022-operation-interceptor
branch
from
ed43c21 to
3725e71
Compare
- Add `IOpenApiOperationTransformer`. - Add `IOpenApiSchemaTransformer`. - Rename `Use*Transfomer()` methods to `Add*Transformer()`. Resolves dotnet#56022.
Add examples for DI-activated operation and schema transformers.
martincostello
force-pushed
the
gh-56022-operation-interceptor
branch
from
bc9749a to
9ee1a54
Compare
Fix mistake during rebase.
Run operation transformers before any document transformers.
Add type-based transformer for operations to its own class.
Revert accidental change to submodule during rebase.
There was a problem hiding this comment.
Looking good! Some minor comments on the docs and feedback on our benchmark test scenarios.
WRT to perf, we can reasses impact for schema transformers once this PR and #56709 are in, but figured we should establish good baselines here.
|
|
||
| /// <summary> | ||
| /// Cache of <see cref="OpenApiOperationTransformerContext"/> instances keyed by the | ||
| /// `ApiDescription.ActionDescriptor.Id` of the associated operation. ActionDescriptor IDs | ||
| /// are unique within the lifetime of an application and serve as helpful associators between | ||
| /// operations, API descriptions, and their respective transformer contexts. | ||
| /// </summary> | ||
| private readonly Dictionary<string, OpenApiOperationTransformerContext> _operationTransformerContextCache = new(); | ||
| private static readonly ApiResponseType _defaultApiResponseType = new ApiResponseType { StatusCode = StatusCodes.Status200OK }; | ||
| private readonly Dictionary<string, OpenApiOperationTransformerContext> _operationTransformerContextCache = []; |
There was a problem hiding this comment.
Super nit: although collection expressions do incidently work correctly for initializing empty dictionaries based on the current implementation, I try to avoid applying the guidance that analyzers provide here since dictionary expressions are technically not supported in the langauge at the moment.
- Remove collection expression. - Fix two typos. - Add parameter to benchmark.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Test failures seem to be an instance of #56828 that is affecting a few other PRs. The particular failrue doesn't seem like a flake but it's not impact all PRs so there might be something here... |
Add OpenAPI operation and schema transformer interfaces
Add interfaces for transforming OpenAPI operations and schemas.
Description
IOpenApiOperationTransformer.IOpenApiSchemaTransformer.Use*Transfomer()methods toAdd*Transformer().Fixes #56022