Conversation
Remove the [Experimental(EXTEXP0013)] attribute from HttpLatencyTelemetryServiceCollectionExtensions and its AddHttpLatencyTelemetry method, and regenerate the public API baseline so the type and method move to Stage: Stable. This mirrors the already-stable outgoing-request equivalent HttpClientLatencyTelemetryExtensions.AddHttpClientLatencyTelemetry. Pending API review approval of #7729. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 544c8b44-3b80-41b2-b9be-e60561adaa97
EasyL0ver
force-pushed
the
users/pkudlacik/http-latency-enricher-followup
branch
from
September 3, 2026 12:50
73b3732 to
ad6cec5
Compare
Update the AddHttpLatencyTelemetry XML remarks and the package README example to include the required AddLatencyContext call (and name the UseRequestCheckpoint/UseRequestLatencyTelemetry middleware), so the documented setup actually produces an ILatencyContext instead of throwing or silently no-op'ing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 373d48a1-1c50-4f3b-b1ae-555139892620
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1597709&view=codecoverage-tab |
The incoming latency enricher escaped only the list-item separator '/' and left the section separator ',' and the client application name field unescaped, so a client name (or tag value) containing ',' or '/' could shift the positional LatencyInfo layout. Escape both ',' and '/' to '_' across every string field, including the client name, keeping the v1.0 format. Fix is scoped to the incoming enricher; the outgoing HttpClient enricher is tracked in #7757. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 373d48a1-1c50-4f3b-b1ae-555139892620
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Delimiter-escaping coverage and README setup corrections remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Promotes AddHttpLatencyTelemetry to stable and fixes delimiter escaping in serialized latency data.
Changes:
- Removes experimental API annotations and updates the API baseline.
- Documents required latency services and usage examples.
- Escapes commas and slashes across latency string fields.
- Adds regression coverage for delimiter escaping.
File summaries
| File | Summary |
|---|---|
test/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware.Tests/Latency/Internal/HttpLatencyLogEnricherTests.cs |
Adds client-name delimiter coverage; broader latency-field coverage remains requested. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/README.md |
Updates setup documentation; example corrections remain requested. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Microsoft.AspNetCore.Diagnostics.Middleware.json |
Updates the API baseline to stable. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Latency/Internal/HttpLatencyLogEnricher.cs |
Escapes delimiters across serialized string fields. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Latency/HttpLatencyTelemetryServiceCollectionExtensions.cs |
Promotes the API and documents prerequisites. |
Review details
Suppressed comments (2)
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Latency/Internal/HttpLatencyLogEnricher.cs:64
- The new helper changes serialization for tag names/values, checkpoint names, and measure names, but the added test only puts delimiters in the client header;
MockLatencyDatacovers/but no,in those fields. A regression in any of these new comma-escaping paths would therefore pass the suite. Add comma-containing names/values (or assert the complete nine-section output) for the latency data fields as well.
AppendSpanEscapingDelimiters(sb, latencyData.Tags, a => a.Name);
_ = sb.Append(',');
AppendSpanEscapingDelimiters(sb, latencyData.Tags, a => a.Value);
test/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware.Tests/Latency/Internal/HttpLatencyLogEnricherTests.cs:81
- This test only puts delimiters in the client header. The updated serializer also routes tag names/values, checkpoint names, and measure names through the new escaping path, while
MockLatencyDatacurrently exercises only/in those fields. Please add comma-containing latency-data fields and assert the positional layout so a regression at any of these call sites cannot pass unnoticed.
public void HttpLatencyLogEnricher_EscapesDelimiters_InClientName()
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| ```csharp | ||
| var builder = WebApplication.CreateBuilder(args); | ||
|
|
||
| builder.Services.AddLatencyContext(); |
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1597895&view=codecoverage-tab |
evgenyfedorov2
requested changes
Sep 16, 2026
| /// Extensions for enriching incoming HTTP request logs with latency telemetry. | ||
| /// </summary> | ||
| [Experimental(diagnosticId: DiagnosticIds.Experiments.HttpLogging, UrlFormat = DiagnosticIds.UrlFormat)] | ||
| public static class HttpLatencyTelemetryServiceCollectionExtensions |
Member
There was a problem hiding this comment.
please rename as approved #7729 (comment)
Suggested change
| public static class HttpLatencyTelemetryServiceCollectionExtensions | |
| public static class HttpLoggingServiceCollectionExtensions |
as well as the .cs file name
| /// that an <see cref="Microsoft.Extensions.Diagnostics.Latency.ILatencyContext"/> is available for each request. | ||
| /// </remarks> | ||
| [Experimental(diagnosticId: DiagnosticIds.Experiments.HttpLogging, UrlFormat = DiagnosticIds.UrlFormat)] | ||
| public static IServiceCollection AddHttpLatencyTelemetry(this IServiceCollection services) |
Member
There was a problem hiding this comment.
Suggested change
| public static IServiceCollection AddHttpLatencyTelemetry(this IServiceCollection services) | |
| public static IServiceCollection AddHttpServerLatencyTelemetry(this IServiceCollection services) |
rename as approved
| /// <summary> | ||
| /// Extensions for enriching incoming HTTP request logs with latency telemetry. | ||
| /// </summary> | ||
| [Experimental(diagnosticId: DiagnosticIds.Experiments.HttpLogging, UrlFormat = DiagnosticIds.UrlFormat)] |
Member
There was a problem hiding this comment.
Let's not remove the attribute for now. Since the changes were requested at the API review and pre-approved, the right course of action is to rename stuff to the API approved shape, release it in that shape at least once, and then remove the attribute.
Member
There was a problem hiding this comment.
so in 10.11.0 version we release it with approved changes but in experimental form, and then in 11.0.0 version we release it in production form.
dotnet-policy-service
Bot
added
the
waiting-author-feedback 📭
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes
AddHttpLatencyTelemetry(incoming HTTP request latency log enricher, packageMicrosoft.AspNetCore.Diagnostics.Middleware) to stable public API, documents the required companion services, and fixes a delimiter-escaping bug in the enricher.Changes
[Experimental(EXTEXP0013)]attribute fromHttpLatencyTelemetryServiceCollectionExtensionsand itsAddHttpLatencyTelemetrymethod, plus the now-unused usings.Stage: Stable.AddLatencyContext,AddRequestLatencyTelemetry,AddRequestCheckpoint) in the XML remarks and the package README example.HttpLatencyLogEnricher: theLatencyInfovalue escaped only the list-item separator/and left the section separator,and the client application name field unescaped, so a client name (or tag value) containing,or/could shift the positional layout and corrupt the value. Both,and/are now escaped to_across every string field, including the client name, keeping thev1.0format. Scoped to the incoming enricher; the outgoing HttpClient enricher has the same class of defect and is tracked in Outgoing HttpClient LatencyInfo enricher value has incomplete escaping #7757.This mirrors the already-stable outgoing-request equivalent
HttpClientLatencyTelemetryExtensions.AddHttpClientLatencyTelemetry.Validation
Microsoft.AspNetCore.Diagnostics.Middleware.Testspass, including a new test asserting the client name delimiters are escaped.Closes #7729
Note
Draft pending API review approval of #7729.