[Prometheus.AspNetCore] Support GZip#7274
Conversation
Observe client requests to abort scrape request processing, including via `X-Prometheus-Scrape-Timeout-Seconds`, and respond with an HTTP 408.
Add PR number.
- Constrain accepted `X-Prometheus-Scrape-Timeout-Seconds` values. - Check cancellation token source. - Do not try to set the status if response has started.
Extend the ASP.NET Core integration tests to include interop between ASP.NET Core and Prometheus itself to scrape metrics.
- Log exceptions calling Prometheus. - Use top-level cancellation instead of nested. - Delete temporary configuration files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7274 +/- ##
==========================================
+ Coverage 89.99% 90.04% +0.04%
==========================================
Files 276 276
Lines 14252 14271 +19
==========================================
+ Hits 12826 12850 +24
+ Misses 1426 1421 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Add support for GZip compression. Resolves open-telemetry#7213.
Use `Fastest` instead of `Optimal`.
Fix tests after merge with main.
ASP.NET Core doesn't have a synchronisation context so `ConfigureAwait(false)` is redundant.
There was a problem hiding this comment.
Pull request overview
Adds gzip response compression support to the Prometheus ASP.NET Core scrape endpoint when clients request it with Accept-Encoding: gzip.
Changes:
- Adds gzip compression path in
PrometheusExporterMiddleware. - Updates negotiation tests to pass typed request headers.
- Adds an integration test and changelog entry for gzip support.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/OpenTelemetry.Exporter.Prometheus.AspNetCore/PrometheusExporterMiddleware.cs |
Adds gzip detection and compressed response writing. |
src/OpenTelemetry.Exporter.Prometheus.AspNetCore/OpenTelemetry.Exporter.Prometheus.AspNetCore.csproj |
Suppresses CA2007 warnings for the project. |
src/OpenTelemetry.Exporter.Prometheus.AspNetCore/CHANGELOG.md |
Documents gzip scrape response support. |
test/OpenTelemetry.Exporter.Prometheus.AspNetCore.Tests/PrometheusIntegrationTests.cs |
Adds integration coverage for gzip responses. |
test/OpenTelemetry.Exporter.Prometheus.AspNetCore.Tests/PrometheusExporterMiddlewareTests.cs |
Updates negotiation tests for the new typed-header signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PackageTags>$(PackageTags);prometheus;metrics</PackageTags> | ||
| <MinVerTagPrefix>coreunstable-</MinVerTagPrefix> | ||
| <DefineConstants>$(DefineConstants);PROMETHEUS_ASPNETCORE</DefineConstants> | ||
| <NoWarn>$(NoWarn);CA2007</NoWarn> |
There was a problem hiding this comment.
AspNetCore has no synchronisation context so ConfigureAwait(false) has no effect.
There was a problem hiding this comment.
True, I would consider keeping this calls just in case we need to share with httplistener similar code
There was a problem hiding this comment.
At the moment the code reuse is one way from HttpListener to AspNetCore, so I don't think we need to worry about that in practice as if it needs to be shared it'll go in the other project which doesn't supress CA2007.
- Honour the Quality parameter for `Accept-Encoding`. - Use `HeaderNames.ContentEncoding` constant. - Specify `Vary: Accept-Encoding`. - Fix swapped xunit assertion.
Kielek
left a comment
There was a problem hiding this comment.
Pushed some direct feedback, needs to resolve conflicts arreound changelog. LGTM
Thanks for spotting I'd put the Copilot suggestion in the wrong place 👍 |
Fixes #7213
Changes
Add support for GZip compression.
Builds on top of #7252 and #7266.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)