Add support for logging/throwing exceptions in RDG #47657
Conversation
captainsafia
requested review from
Tratcher,
BrennanConroy and
halter73
as code owners
ghost
added
the
|
I had a look at the default parameter handling. Looks good, did you happen to confirm whether it will work for things like ints etc? It looks like you handle that scenario but I haven't run the code to confirm. |
|
This approach currently always allocates no matter what. We're also losing the Name part of the EventID. And, we're losing the structured logging part of the logs (the named parameters). |
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
| (false, {{RequestDelegateCreationLogging.InvalidFormRequestBodyEventId}}) => {{SymbolDisplay.FormatLiteral(RequestDelegateCreationLogging.InvalidFormRequestBodyLogMessage, true)}}, | ||
| _ => throw new UnreachableException("Encountered unsupported logging EventId.") | ||
| }; | ||
| void logOrThrowException(int eventId, string eventName, Exception? exception = null, int statusCode = StatusCodes.Status400BadRequest, params object?[] messageFormatArgs) |
There was a problem hiding this comment.
params object?[] that's an allocation 😢
There was a problem hiding this comment.
Fixed. params object?[] ➡️ params string?[[]
There was a problem hiding this comment.
That's not the allocation I was thinking of. The array part of params blah[] is an allocation.
But that is a good point about the difference between this code and what RDF uses. RDF uses LoggerMessage.Define which is generic so it can avoid boxing of arguments. Although this now forces a string allocation for anything passed in. e.g. StatusCodes.Status400BadRequest when ThrowOnBadRequest is false and log level is not debug.
I think the PR is fine for now, but we should probably file an issue to consider using LoggerMessage.Define and generics to avoid the boxing and array allocations.
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
| @@ -768,48 +768,6 @@ public async Task RequestDelegateHandlesNullableStringValuesFromExplicitQueryStr | |||
| Assert.Equal(new StringValues(new[] { "7", "8", "9" }), httpContext.Items["form"]!); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
I know there has been stuff going on with testing relating to RDG. Does that affect the RDF tests? Is RDF tested with the same code as RDG somewhere else now so we can share tests?
|
Merging. @BrennanConroy and I synced offline about the changes in 739d3f0. |
* Add support for logging/throwing exceptions in RDG * Fix event names, structured logs, and excessive allocation * Remove unused vars, fix allocation, and update logger * Use LogOrThrowExceptionHelper class
amcasey
added
area-minimal
Addresses #46362 and #47266.
TryParse-able elementsNote: I added some logic to the emitter context to avoid emitting the logging-related code unnecessarily but this will be more clearly handled for all scenarios by #47147