-
Notifications
You must be signed in to change notification settings - Fork 25
Adding generated correlation id to response #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Adding generated correlation id to response #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements functionality to add a generated correlation ID to the HTTP response headers, addressing issue #61. The change introduces a new addCorrelationIdToResponse parameter that controls whether the correlation ID should be written back to the response.
Key Changes:
- Added a new boolean parameter
addCorrelationIdToResponseto control response header population - Refactored correlation ID resolution logic into separate methods for better maintainability
- Updated all test cases to reflect the new parameter and verify response header behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CorrelationIdEnricher.cs |
Added response header writing capability and refactored correlation ID preparation into dedicated methods |
ClientInfoLoggerConfigurationExtensions.cs |
Extended API with new addCorrelationIdToResponse parameter |
CorrelationIdEnricherTests.cs |
Updated all test constructors to use new parameter signature and added assertions for response header verification |
appsettings.json |
Enabled the new feature in sample application configuration |
SampleWebApp.csproj |
Changed from package reference to project reference for testing local changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| httpContext.Items.Add(Constants.CorrelationIdValueKey, correlationId); | ||
| } | ||
|
|
||
| private void AddCorrelationIdToResponse(HttpContext httpContext, in string correlationId) |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The in modifier on the correlationId parameter is unnecessary and provides no benefit for string types. Since strings are immutable reference types, using in doesn't prevent copying or improve performance. Remove the in modifier.
| private void AddCorrelationIdToResponse(HttpContext httpContext, in string correlationId) | |
| private void AddCorrelationIdToResponse(HttpContext httpContext, string correlationId) |
| string returnValue; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(requestHeader)) | ||
| { | ||
| returnValue = requestHeader; | ||
| } | ||
| else | ||
| { | ||
| StringValues responseHeader = httpContext.Response.Headers[_headerKey]; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(responseHeader)) | ||
| { | ||
| returnValue = responseHeader; | ||
| } | ||
| else | ||
| { | ||
| if (_addValueIfHeaderAbsence) | ||
| { | ||
| returnValue = Guid.NewGuid().ToString(); | ||
| } | ||
| else | ||
| { | ||
| returnValue = null; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return returnValue; |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested if-else statements create unnecessary complexity. The inner else block (lines 114-123) can be flattened by combining the conditions or using early returns in the method to improve readability.
| string returnValue; | |
| if (!string.IsNullOrWhiteSpace(requestHeader)) | |
| { | |
| returnValue = requestHeader; | |
| } | |
| else | |
| { | |
| StringValues responseHeader = httpContext.Response.Headers[_headerKey]; | |
| if (!string.IsNullOrWhiteSpace(responseHeader)) | |
| { | |
| returnValue = responseHeader; | |
| } | |
| else | |
| { | |
| if (_addValueIfHeaderAbsence) | |
| { | |
| returnValue = Guid.NewGuid().ToString(); | |
| } | |
| else | |
| { | |
| returnValue = null; | |
| } | |
| } | |
| } | |
| return returnValue; | |
| if (!string.IsNullOrWhiteSpace(requestHeader)) | |
| { | |
| return requestHeader; | |
| } | |
| StringValues responseHeader = httpContext.Response.Headers[_headerKey]; | |
| if (!string.IsNullOrWhiteSpace(responseHeader)) | |
| { | |
| return responseHeader; | |
| } | |
| if (_addValueIfHeaderAbsence) | |
| { | |
| return Guid.NewGuid().ToString(); | |
| } | |
| return null; |
|
@mo-esmp , do you have any news about current review? |
|
I will merge it today and publish a new version. Thanks again for the contribution. |
|
@mo-esmp , don't do it, please. I had just tolking with my colleags and they provide more usefull idea about current feature implementations. |
…or CorrelationId generation and use it by dependency injection.
…overriding the algorithm for CorrelationId generation by dependency injection.
…enerated CorrelationId to the response.
|
@mo-esmp , can you look at my changes again? I have removed the logic, that adds CorrelationId to the response, because adding headers to the response breaks at least Single Responsibility Principle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected string? CorrelationId { get; set; } | ||
|
|
||
| /// <inheritdoc/> | ||
| public string? PrepareCorrelationId( | ||
| HttpContext httpContext, | ||
| CorrelationIdPreparerOptions correlationIdPreparerOptions) | ||
| { | ||
| if (string.IsNullOrEmpty(CorrelationId)) | ||
| { | ||
| CorrelationId = PrepareValueForCorrelationId(httpContext, correlationIdPreparerOptions); | ||
| } | ||
|
|
||
| return CorrelationId; |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CorrelationId property is not thread-safe and could lead to race conditions when multiple threads access the same CorrelationIdPreparer instance. Since this class may be registered as a singleton through DI, concurrent requests could overwrite each other's correlation IDs. Consider using AsyncLocal<string> or making this class scoped/transient, or remove the caching altogether since the preparer is called once per log event.
| protected string? CorrelationId { get; set; } | |
| /// <inheritdoc/> | |
| public string? PrepareCorrelationId( | |
| HttpContext httpContext, | |
| CorrelationIdPreparerOptions correlationIdPreparerOptions) | |
| { | |
| if (string.IsNullOrEmpty(CorrelationId)) | |
| { | |
| CorrelationId = PrepareValueForCorrelationId(httpContext, correlationIdPreparerOptions); | |
| } | |
| return CorrelationId; | |
| /// <inheritdoc/> | |
| public string? PrepareCorrelationId( | |
| HttpContext httpContext, | |
| CorrelationIdPreparerOptions correlationIdPreparerOptions) | |
| { | |
| return PrepareValueForCorrelationId(httpContext, correlationIdPreparerOptions); |
| protected string? CorrelationId { get; set; } | ||
|
|
||
| /// <inheritdoc/> | ||
| public string? PrepareCorrelationId( | ||
| HttpContext httpContext, | ||
| CorrelationIdPreparerOptions correlationIdPreparerOptions) | ||
| { | ||
| if (string.IsNullOrEmpty(CorrelationId)) | ||
| { | ||
| CorrelationId = PrepareValueForCorrelationId(httpContext, correlationIdPreparerOptions); | ||
| } | ||
|
|
||
| return CorrelationId; |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caching logic preserves the first correlation ID across multiple HTTP requests. If the preparer instance is shared (e.g., registered as singleton), subsequent requests will incorrectly reuse the cached correlation ID from the first request instead of computing a new one. This defeats the purpose of per-request correlation IDs.
| protected string? CorrelationId { get; set; } | |
| /// <inheritdoc/> | |
| public string? PrepareCorrelationId( | |
| HttpContext httpContext, | |
| CorrelationIdPreparerOptions correlationIdPreparerOptions) | |
| { | |
| if (string.IsNullOrEmpty(CorrelationId)) | |
| { | |
| CorrelationId = PrepareValueForCorrelationId(httpContext, correlationIdPreparerOptions); | |
| } | |
| return CorrelationId; | |
| /// <inheritdoc/> | |
| public string? PrepareCorrelationId( | |
| HttpContext httpContext, | |
| CorrelationIdPreparerOptions correlationIdPreparerOptions) | |
| { | |
| return PrepareValueForCorrelationId(httpContext, correlationIdPreparerOptions); |
|
@Maximys sorry again for the late reply. I think it’s not the responsibility of an enricher to add a header to the HTTP response, but I understand the benefit of including the correlation ID to reduce extra work. Therefore, I prefer to keep all logic within the enricher, as in your first implementation, and avoid introducing a |
Current PR implements issue #61.
Please note that I removed one copy-pasted test that existed before in commit 0f7948d0d65702009e7a85c7228e7cc43af8dc02.