fix(observability): exclude the extension's JwtMetricsCollector so it stops failing every 10s (#173) - #184
Conversation
… stops failing every 10s (#173) The token-sheriff-validation-quarkus extension's `metrics.JwtMetricsCollector` constructor-injects the `SecurityEventCounter` produced by the extension's UNQUALIFIED `TokenValidatorProducer`, which resolves issuers from the `sheriff.token.issuers.<name>.*` namespace this gateway never populates — the request path runs off the `@GatewayValidator`-qualified validator built from `gateway.yaml`. The collector carries `@Scheduled(every = "10s")`, so bean creation failed on every tick and the scheduler wrote roughly sixty stack-trace lines every ten seconds. Readiness and token validation were unaffected; the log was not, and unreadable logs are a real cost during integration work. This is the same structural mismatch ADR-0027 already decided, with one bean left outside its scope. `quarkus.arc.exclude-types` now names the extension's metrics package alongside its health package. - Broaden ADR-0027 from "unqualified health probes" to "unqualified beans": one mismatch presenting as two symptoms depending on what drives each bean, and the decision restated over beans rather than probes. Renamed accordingly, with the four link sites updated. - Stop short of a `de.cuioss.sheriff.token.quarkus.*` sweep: that tree also carries beans a future change may legitimately want, and a sweep would remove them silently. - Add `ExtensionUnqualifiedBeanExclusionTest`. Beyond pinning the specific bean, it asserts the general form — no scheduled job may name the extension's Quarkus wiring — so the next bean of this shape fails the build instead of being found in a production log. Both legs carry the paired guards, and the filter carries a matched positive/negative control because the resting state is an empty input. Publishing the QUALIFIED validator's SecurityEventCounter as meters remains absent and is separate work; the collector never published a meter either, so this removes stack traces, not JWT metrics. Closes #173 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QQM5D6d5HeMWfKta3hRQje
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe gateway now excludes the token-validation extension’s unqualified health probes and ChangesToken-validation extension exclusion
Estimated code review effort: 3 (Moderate) | ~20 minutes Assessment against linked issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍(Review updated until commit 8e7df20)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
ExtensionUnqualifiedBeanExclusionTestrelies on hard-coded trigger description strings (including IDs and formatting), which makes it brittle to internal scheduler formatting changes; consider asserting on parsed components (bean class + method) or mocking aTriggerinstead of using full literal identities. - The
extensionJobsAmongfilter usesString.contains(EXTENSION_QUARKUS_PACKAGE)on the entire trigger description, which could produce false positives if any other job description happens to include that substring; you might tighten this to match on the bean class portion specifically rather than the full description. - Both the application properties comments and the test class Javadoc are quite long and narrative; it may be easier to maintain if you move some of the background/rationale to the ADR and keep the in-code comments focused on what is being pinned and how, rather than the full history and justification.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ExtensionUnqualifiedBeanExclusionTest` relies on hard-coded trigger description strings (including IDs and formatting), which makes it brittle to internal scheduler formatting changes; consider asserting on parsed components (bean class + method) or mocking a `Trigger` instead of using full literal identities.
- The `extensionJobsAmong` filter uses `String.contains(EXTENSION_QUARKUS_PACKAGE)` on the entire trigger description, which could produce false positives if any other job description happens to include that substring; you might tighten this to match on the bean class portion specifically rather than the full description.
- Both the application properties comments and the test class Javadoc are quite long and narrative; it may be easier to maintain if you move some of the background/rationale to the ADR and keep the in-code comments focused on what is being pinned and how, rather than the full history and justification.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ef8f7aca-b970-47fa-b9af-d2588a2613ed
📒 Files selected for processing (8)
api-sheriff/src/main/resources/application.propertiesapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ExtensionUnqualifiedBeanExclusionTest.javadoc/adr/0027-The_token-validation_extensions_unqualified_beans_are_excluded_not_accommodated.adocdoc/adr/0027-The_token-validation_extensions_unqualified_health_probes_are_excluded_not_accommodated.adocdoc/adr/0032-The_shipped_artifact_declares_nothing_test-shaped_the_deployment_supplies_what_it_names.adocdoc/architecture.adocdoc/development/integration-test-topology.adocdoc/user/environment-variable-overrides.adoc
💤 Files with no reviewable changes (1)
- doc/adr/0027-The_token-validation_extensions_unqualified_health_probes_are_excluded_not_accommodated.adoc
…the metrics bean in ADR-0032 Review feedback on #184. - ADR-0032 §5 said the exclusion key removes "the unused extension health probes"; it now removes the metrics collector too, so the sentence names both. - ADR-0027 §2 records the stronger check that was tried and rejected, so it is not attempted again from scratch. Asserting the extension's unqualified TokenValidator is unresolvable looks driver-agnostic but does not work: measured, it stays resolvable with every reaching bean excluded, because the producer is retained regardless. Retained is not instantiated — nothing calls a method on it, which is why the ten-second failure is genuinely gone — but that leaves resolvability unable to tell the healthy state from the broken one. Enumerating the container's beans and diffing against the exclusion list is the other candidate, and it is a membership snapshot, which ADR-0030 forbids. What stays derivable is the symptom, not the scope. The gap is carried in the ADR's Risks rather than papered over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QQM5D6d5HeMWfKta3hRQje
|
Thanks @sourcery-ai — replying to all three points. Two are declined with reasons, one is worth being precise about. 1. Hard-coded trigger description strings are brittle. Declined, because the literals are not in the production assertion — they are the fixture of a matched positive/negative control, which is a deliberate pattern here rather than an oversight. If the scheduler's identity format changes, the control keeps passing (it tests the filter, not the format) and the production assertion keeps working (it reads Mocking a 2. Fair to raise, and the near miss you are pointing at is covered explicitly — the control's third fixture is Beyond that, the input is 3. Comments and Javadoc are long and narrative. Declined — this is house style in this repository, not incidental. The split you suggest is the one already in place. Rationale, alternatives and the residual gap live in ADR-0027 — including, as of |
|
/review |
1 participant
Fixes #173.
The defect
de.cuioss.sheriff.token.quarkus.metrics.JwtMetricsCollectorconstructor-injects theSecurityEventCounterproduced by the token-sheriff-validation extension's unqualifiedTokenValidatorProducer. That producer resolves issuers fromsheriff.token.issuers.<name>.*— a namespace this gateway never populates, because the request path runs off the@GatewayValidator-qualified validator built fromgateway.yaml.The collector carries
@Scheduled(every = "10s"). So bean creation failed on every tick, and the scheduler logged a full stack trace each time — roughly sixty lines every ten seconds. Readiness and token validation were unaffected; the log was not.This is exactly the mismatch ADR-0027 already decided, with one bean left outside its scope.
Reproduced, then fixed
The new test was written first and run against unmodified
main. It failed with the trigger identity from the issue report verbatim:Both guard legs passed in that run, so the failures were genuine rather than a container that had contributed nothing.
The change
quarkus.arc.exclude-typesnow names the extension's metrics package alongside its health package:quarkus.arc.exclude-types=de.cuioss.sheriff.token.quarkus.health.*,de.cuioss.sheriff.token.quarkus.metrics.*Deliberately not a
de.cuioss.sheriff.token.quarkus.*sweep: that tree also carries beans a future change here may legitimately want (the@BearerTokenproducer, the claim-mapper registry), and a sweep would remove them at the moment someone reached for one. Deliberately notquarkus.scheduler.enabled=falseeither — that would pre-emptively remove a mechanism this gateway has no reason to forbid.ADR-0027 broadened
The ADR's title described a subset of what it now decides, so it is restated over beans rather than probes: one structural mismatch, presenting as two symptoms depending on what drives each bean (health endpoint → aggregate readiness DOWN; scheduler → a stack trace every ten seconds). Renamed to
0027-The_token-validation_extensions_unqualified_beans_are_excluded_not_accommodated.adoc, with all four link sites updated.The ADR's own risk section now records the honest weakness the second symptom exposed: the exclusion list grows by discovery, and each entry so far was added after someone observed a symptom in a running deployment.
The test
ExtensionUnqualifiedBeanExclusionTestcarries two assertions plus a control:JwtMetricsCollectoris not a bean. Paired withSheriffMetricsresolving, so a run in which CDI contributed nothing cannot masquerade as a successful exclusion.de.cuioss.sheriff.token.validation.*— the library this gateway genuinely uses, which a prefix trimmed one segment too short would over-select.Not in scope
Publishing the qualified validator's
SecurityEventCounteras meters is genuinely absent and is separate work — the seam isSheriffMetrics.bindSecurityEventCounter, which already does this for thecui-httpcounter. The excluded collector never published a meter either (it failed inside its own dependency resolution), so this PR removes stack traces, not JWT metrics. Recorded in the ADR and in the properties comment rather than left to be rediscovered.The issue's second suggestion — that a metrics collector finding no property-based issuers should warn once and go idle rather than fail hard on every tick — is a token-sheriff-side observation and is worth raising upstream. It is noted in the ADR's alternatives as complementary to this exclusion, not a substitute.
Verification
verify -Ppre-commit— greenverify— green🤖 Generated with Claude Code
https://claude.ai/code/session_01QQM5D6d5HeMWfKta3hRQje
Summary by CodeRabbit
Bug Fixes
Documentation
Tests