Structured HTTP Request Logging via Serilog JSON Sink - #77
Conversation
…rv/TeaPie into feature/requests-logging
…xecuteRequestStep
There was a problem hiding this comment.
Pull Request Overview
This PR adds structured HTTP request logging to TeaPie by implementing a JSON-based logging system that captures detailed information about HTTP requests, responses, retries, and authentication. The implementation uses Serilog's JSON formatter to write structured logs to a separate file when the --requests-log-file option is provided.
Key Changes:
- Added
RequestsLoggingHandleras a DelegatingHandler to intercept and log HTTP requests/responses - Introduced data structures (
RequestLogFileEntry,RequestInfo,ResponseInfo, etc.) to capture structured request information - Enhanced Serilog configuration to support separate JSON log file for HTTP requests with filtering
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TeaPie/Logging/Setup.cs | Adds sink configuration methods for console, file, and requests logging with filtering logic |
| src/TeaPie/Logging/RequestsLoggingHandler.cs | Implements the core logging handler that captures request/response data and retry attempts |
| src/TeaPie/Logging/RequestLogFileEntry.cs | Defines data structures for structured logging of requests, responses, retries, and authentication |
| src/TeaPie/Http/ExecuteRequestStep.cs | Integrates logging handler into request execution pipeline and tracks request context |
| src/TeaPie/Http/Auth/Setup.cs | Registers RequestsLoggingHandler in HTTP client pipeline |
| src/TeaPie/ApplicationBuilder.cs | Adds pathToRequestsLogFile parameter to logging configuration |
| src/TeaPie.DotnetTool/TestCommand.cs | Passes RequestsLogFile setting to application builder |
| src/TeaPie.DotnetTool/LoggingSettings.cs | Adds --requests-log-file command option |
| src/TeaPie.DotnetTool/ExploreCommand.cs | Passes RequestsLogFile setting to application builder |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (File.Exists(pathToRequestsLogFile)) | ||
| { | ||
| File.Delete(pathToRequestsLogFile); | ||
| } |
There was a problem hiding this comment.
Deleting the file without synchronization could cause issues if multiple processes or threads attempt to write to the same log file simultaneously. Consider using a lock mechanism or checking if the file is in use before deletion.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| StatusCode = (int)response.StatusCode, | ||
| ReasonPhrase = response.ReasonPhrase, | ||
| Headers = ProcessHeaders(response.Headers), | ||
| Body = await GetContentBodyAsync(response.Content), |
There was a problem hiding this comment.
Reading the HTTP response content body will consume the stream and prevent it from being read again by subsequent code that expects to process the response. This will cause failures when the response needs to be read elsewhere. Consider buffering the response content before reading it, or removing body logging for responses.
| Method = request.Method.ToString(), | ||
| Uri = request.RequestUri?.ToString() ?? string.Empty, | ||
| Headers = ProcessHeaders(request.Headers), | ||
| Body = await GetContentBodyAsync(request.Content), |
There was a problem hiding this comment.
Reading the HTTP request content body will consume the stream and prevent it from being read again later in the pipeline. This will cause the actual HTTP request to fail when it tries to send the body. Consider either:
- Removing body logging for requests, or
- Using a buffered approach that preserves the content stream for later use
There was a problem hiding this comment.
we need to read the content body only once for now
There was a problem hiding this comment.
hmm nie som si istý. My ho teraz už čítame 2x. Lebo raz ho čítaš tu pre účely logovania a druhýkrát sa číta na mieste kde sa spracováva výsledok toho responsu.
Takže by sme to mali zvážiť. Čitanie obsahu je tricky vec.
There was a problem hiding this comment.
obsah je čítaný viackrát ale skrz CloneMessage v ExecuteRequestStep, kt. implementoval ešte Maťo sa stream ukladá do StringContentu a mal by byť readable viac ako raz,
aj v .json je body requestu uložené správne:
{"Name":"AddCarRequest","Method":"POST","Uri":"http://localhost:3001/cars","Headers":{"Authorization":"Bearer authToken"},"Body":"{\"Id\":10,\"Brand\":\"Ford\",\"Model\":\"Focus\",\"EngineType\":\"Petrol\",\"TransmissionType\":\"Automatic\",\"PeopleCapacity\":2,\"Color\":\"indigo\",\"Year\":1994,\"DrivenKilometres\":124679.6246257195,\"Description\":\"Vero similique ut sed.\"}",
| { | ||
| if (File.Exists(pathToRequestsLogFile)) | ||
| { | ||
| File.Delete(pathToRequestsLogFile); |
There was a problem hiding this comment.
The file deletion logic could cause issues in concurrent scenarios or when the file is locked by another process. Consider wrapping this in a try-catch block or using a safer file handling approach that doesn't silently fail if the file cannot be deleted.
| File.Delete(pathToRequestsLogFile); | |
| try | |
| { | |
| File.Delete(pathToRequestsLogFile); | |
| } | |
| catch (IOException) | |
| { | |
| // The file is in use or cannot be deleted. Optionally log this event. | |
| } | |
| catch (UnauthorizedAccessException) | |
| { | |
| // The file cannot be deleted due to permission issues. Optionally log this event. | |
| } |
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public Exception? Exception { get; set; } |
There was a problem hiding this comment.
The exception object itself is being serialized directly into the JSON log. This could cause serialization issues or expose sensitive stack trace information. Consider logging only essential exception details like exception.GetType().Name and exception.Message, or use a custom exception serializer.
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | |
| public Exception? Exception { get; set; } | |
| [JsonIgnore] | |
| public Exception? Exception { get; set; } | |
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | |
| public string? ExceptionType => Exception?.GetType().Name; | |
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | |
| public string? ExceptionMessage => Exception?.Message; | |
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | |
| public string? ExceptionStackTrace => Exception?.StackTrace; |
| .AddHttpMessageHandler<AuthHttpMessageHandler>() | ||
| .AddHttpMessageHandler<RequestsLoggingHandler>() |
There was a problem hiding this comment.
The handler order matters for DelegatingHandlers. RequestsLoggingHandler is added after AuthHttpMessageHandler but before LoggingInterceptorHandler. This means authentication headers will be captured in the logs, which could be a security concern if tokens or credentials are logged. Consider the security implications of this ordering or add sanitization for sensitive headers.
| .AddHttpMessageHandler<AuthHttpMessageHandler>() | |
| .AddHttpMessageHandler<RequestsLoggingHandler>() | |
| .AddHttpMessageHandler<RequestsLoggingHandler>() | |
| .AddHttpMessageHandler<AuthHttpMessageHandler>() |
| Method = request.Method.ToString(), | ||
| Uri = request.RequestUri?.ToString() ?? string.Empty, | ||
| Headers = ProcessHeaders(request.Headers), | ||
| Body = await GetContentBodyAsync(request.Content), |
There was a problem hiding this comment.
hmm nie som si istý. My ho teraz už čítame 2x. Lebo raz ho čítaš tu pre účely logovania a druhýkrát sa číta na mieste kde sa spracováva výsledok toho responsu.
Takže by sme to mali zvážiť. Čitanie obsahu je tricky vec.
This pull request introduces structured logging of HTTP requests by adding a new JSON requests log file feature. It allows users to specify a separate file for detailed, structured logs of HTTP requests and responses, including retries and authentication details, while keeping the main log file and console output clean. The implementation involves updates to the logging configuration, command-line options, and HTTP pipeline.
The most important changes are:
Structured HTTP Requests Logging
--requests-log-filecommand-line option to specify a file for structured JSON logs of HTTP requests.RequestLogFileEntryand related classes to represent structured log entries for HTTP requests, responses, retries, and authentication.RequestsLoggingHandler, that creates and logs structured entries for each HTTP request, including retries and errors.Logging Pipeline
ApplicationBuilderand logging setup to accept the new requests log file path, and to configure Serilog to write structured HTTP request logs to the specified file in JSON format, filtering them out from the main log file and console output.HTTP Pipeline
RequestsLoggingHandlerin the HTTP pipeline so that all outgoing requests are logged.These changes provide users with a new way to analyze HTTP requests in a structured, machine-readable format, and lay the groundwork for further use (e.g. VS Code Extension).