Add exception metric emission filter#5590
Open
feordin wants to merge 7 commits into
Open
Conversation
Geneva-side filtering of auth-failure metrics is not supported, so we must stop the flood at the source. ApiNotificationMiddleware now skips publishing ApiResponseNotification for 401/403 responses, preventing auth-failure bursts (e.g., expired-token retry storms) from overwhelming the shared metric account. ADR adr-2605 is rewritten to reflect this decision and document why the earlier rate-limiting designs were superseded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5590 +/- ##
==========================================
+ Coverage 77.28% 77.36% +0.08%
==========================================
Files 995 998 +3
Lines 36467 36553 +86
Branches 5520 5545 +25
==========================================
+ Hits 28184 28280 +96
+ Misses 6924 6902 -22
- Partials 1359 1371 +12 🚀 New features to boost your workflow:
|
Reverts the broad 401/403 suppression in ApiNotificationMiddleware and replaces it with a targeted, pluggable IExceptionMetricEmissionFilter applied at the AzureMonitorOpenTelemetryLogEnricher chokepoint where the fhir/failures/exceptions metric is actually emitted. The default filter (AuthenticationFailureExceptionMetricEmissionFilter) suppresses emission only when both an auth exception (SecurityToken Exception or one in the inner chain) AND a 401 response are present — the exact signature of the incident. 403s, validation errors, and other failures continue to emit metrics. Downstream consumers (fhir-paas) can compose additional filters via DI, subclass the default filter (IsAuthenticationException is virtual), or replace it entirely with RemoveAll. ADR-2605 rewritten to capture the new design and the extension patterns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
feordin
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+89
to
+95
| foreach (var filter in _exceptionMetricEmissionFilters) | ||
| { | ||
| if (!filter.ShouldEmit(data.Exception, httpContext)) | ||
| { | ||
| return; | ||
| } | ||
| } |
… docs Auth filter: The existing SecurityTokenException inner-chain match already covers SecurityTokenExpiredException, SecurityTokenInvalid AudienceException, and SecurityTokenInvalidIssuerException (all derive from SecurityTokenException). XML doc updated to call this out. New SecurityAbuseExceptionMetricEmissionFilter suppresses metric emission for ServerSideRequestForgeryException (defined in this repo). Match is exception-type-only — no status-code condition — because these exceptions are constructed only inside FHIR's abuse-rejection paths and always produce 4xx responses. Downstream consumers add paas-only exception types (AntiSSRFException, S2SAuthenticationException) by subclassing the relevant default filter and overriding the protected virtual Is...Exception method, then re-registering via RemoveAll + AddSingleton. Documented in the ADR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
feordin
changed the title
feordin
added
Area-Operations
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
3 participants
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
Adds a pluggable
IExceptionMetricEmissionFilterinterface and a defaultAuthenticationFailureExceptionMetricEmissionFilterthat suppresses thefhir/failures/exceptionsmetric for authentication exceptions (SecurityTokenExceptionor one in the inner-exception chain) that produced a 401 response.This addresses the auth-failure flood incident that caused the shared Geneva metric account to throttle.
Extensibility for fhir-paas
The filter chain is designed so that fhir-paas can extend or override without forking fhir-server:
IExceptionMetricEmissionFilter. The enricher AND-combines all registered filters; the default registration usesAddSingleton(notTryAddSingleton) so additions compose.AuthenticationFailureExceptionMetricEmissionFilterand override theprotected virtual IsAuthenticationExceptionmethod, then replace the registration withRemoveAll+AddSingleton.services.RemoveAll<IExceptionMetricEmissionFilter>().Documented in detail in ADR-2605.
Testing
dotnet test src/Microsoft.Health.Fhir.Api.UnitTests/Microsoft.Health.Fhir.R4.Api.UnitTests.csproj— 1315 passed, 0 failed (net8.0 and net9.0).