Log every API Request/Response #968
Conversation
amos-cha
changed the title
amos-cha
changed the title
| [Required] | ||
| [StringLength(15)] | ||
| [Unicode(false)] | ||
| public string ClientIP { get; set; } |
There was a problem hiding this comment.
We should be ready for IPv6 addresses, and not break when we see them. So this should probably be varchar big enough for that.
| } | ||
|
|
||
| /// <summary> | ||
| /// Invoke Async is called on all HTTP requests and are intercepted by httpContext pipelining |
There was a problem hiding this comment.
"and are" -> "and is"?
Besides explaining when it's called, please summarize what it does.
| /// <summary> | ||
| /// Invoke Async is called on all HTTP requests and are intercepted by httpContext pipelining | ||
| /// </summary> | ||
| /// <param name="httpContext"></param> |
There was a problem hiding this comment.
Just naming the argument doesn't tell us anything we can't see from the code. Explain what it's for, so we know why it's here.
| return 0; | ||
| } | ||
|
|
||
| private async Task<string> ReadBodyFromRequest(HttpRequest request) |
There was a problem hiding this comment.
Needs a comment saying what it does.
(The internal comments explaining constraints and how it works are good.)
Co-authored-by: Russ Tuck <[email protected]>
|
I did a little searching for best practices in handling .NET logs. Here are some notes on what I found (with references at the bottom).
We could adopt one of the log saving systems recommended in the references below, but then we'd also need to figure out how to get the data into a machine learning system. Or we can simply save it into the DB, where it's also easy to query it for ML. My vote is to start logging to the database, and see how it goes. If the DB shows signs of stress as a result, we can disable it and then try another option. References: |
EjPlatzer
changed the title
|
All of our requests come through a proxy, so logging the source IP address is probably not going to be super helpful in our case. You would have to do something smarter to get the source IP from the client/UI side if you wanted it. |
|
Evan prefers a different approach (and has some good reasons), and started the work in #1036. But more work is needed to make that useful:
|
Handles: #940
For code explanation: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/middleware/extensibility?view=aspnetcore-6.0
I can't explain as well as Microsoft's own documentation. The reason I've selected a factory based middleware activation is due to the requirement of dependency injection of our DbContext.
This is an incredibly hefty table (
CCT.dbo.RequestResponseLog) and I've tried to minimize unnecessary data but the table is most definitely up for change and will probably need a trigger to clear the table on occasion. Merge to develop will be necessary for testing under load.