Add is_whitelisted tag to captcha metrics #104
Conversation
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
WalkthroughAdds an is_whitelisted metric tag to three captcha-related metrics by deriving it from DifficultyService.isSourceAddressWhitelisted. Updates tests to validate the new tag for both whitelisted and non-whitelisted source addresses without changing public APIs or error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant CaptchaController
participant MetricsService
participant DifficultyService
participant MetricsRegistry
Client->>CaptchaController: POST /captcha/challenge or /verify
CaptchaController->>MetricsService: record metrics (siteKey, sourceAddress, ...)
MetricsService->>DifficultyService: isSourceAddressWhitelisted(siteKey, sourceAddress)
DifficultyService-->>MetricsService: boolean isWhitelisted
rect rgba(213,232,255,0.4)
note right of MetricsService: Tag metrics with is_whitelisted = isWhitelisted
MetricsService->>MetricsRegistry: increment/record with tags<br/>site_key, difficulty, is_whitelisted
end
MetricsRegistry-->>MetricsService: ack
MetricsService-->>CaptchaController: done
CaptchaController-->>Client: response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
captchaservice-backend/src/main/java/de/muenchen/captchaservice/service/metrics/MetricsService.java(2 hunks)captchaservice-backend/src/test/java/de/muenchen/captchaservice/controller/captcha/CaptchaControllerTest.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codeql / Analyze Java source files (./captchaservice-backend)
🔇 Additional comments (1)
captchaservice-backend/src/test/java/de/muenchen/captchaservice/controller/captcha/CaptchaControllerTest.java (1)
282-284: Assertions look right for non‑whitelisted path.LGTM; asserts both the evolving request_count and is_whitelisted=false.
Comment on lines
+36
to
47
| final boolean isWhitelisted = difficultyService.isSourceAddressWhitelisted(siteKey, sourceAddress); | ||
| long sameSourceAddressRequestCount = getSameSourceAddressRequestCount(sourceAddress); | ||
|
|
||
| Counter.builder("captcha.challenge.requests") | ||
| .tag("site_key", siteKey) | ||
| .tag("difficulty", String.valueOf(difficulty)) | ||
| .tag("same_source_address_request_count", String.valueOf(sameSourceAddressRequestCount)) | ||
| .tag("is_whitelisted", String.valueOf(isWhitelisted)) | ||
| .description("Counter for captcha challenge requests") | ||
| .register(meterRegistry) | ||
| .increment(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New tag changes meter identity; also flag high-cardinality tag risk.
- Adding is_whitelisted changes the time‑series identity for captcha.challenge.requests. Coordinate dashboard/alert updates and backfills.
- Pre‑existing tag same_source_address_request_count creates unbounded series (new time series per request count). This is a metrics anti‑pattern and can explode cardinality. Remove it or encode it as a measurement/bucketed histogram instead.
Apply this diff to drop the risky tag (not introduced by this PR, but strongly recommended):
Counter.builder("captcha.challenge.requests")
.tag("site_key", siteKey)
.tag("difficulty", String.valueOf(difficulty))
- .tag("same_source_address_request_count", String.valueOf(sameSourceAddressRequestCount))
.tag("is_whitelisted", String.valueOf(isWhitelisted))To check for dashboards/alerts using this metric:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'captcha\.challenge\.requests|captcha\.verify\.success|captcha\.client\.solve\.time'Length of output: 5720
Remove high‑cardinality tag "same_source_address_request_count" from all captcha metrics; coordinate any "is_whitelisted" identity change.
- The numeric tag same_source_address_request_count is present as a tag in MetricsService.java for: captcha.challenge.requests, captcha.verify.success, and captcha.client.solve.time (MetricsService.java ~lines 37–41, 52–56, 72–76). A count used as a tag creates effectively unbounded cardinality — remove it or record it as a measurement/histogram with bucketed ranges.
- is_whitelisted is present in the metrics and asserted in tests (CaptchaControllerTest); adding/changing it alters time‑series identity — update dashboards/alerts and backfill as needed.
Apply this removal in each metric builder:
-.tag("same_source_address_request_count", String.valueOf(sameSourceAddressRequestCount))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final boolean isWhitelisted = difficultyService.isSourceAddressWhitelisted(siteKey, sourceAddress); | |
| long sameSourceAddressRequestCount = getSameSourceAddressRequestCount(sourceAddress); | |
| Counter.builder("captcha.challenge.requests") | |
| .tag("site_key", siteKey) | |
| .tag("difficulty", String.valueOf(difficulty)) | |
| .tag("same_source_address_request_count", String.valueOf(sameSourceAddressRequestCount)) | |
| .tag("is_whitelisted", String.valueOf(isWhitelisted)) | |
| .description("Counter for captcha challenge requests") | |
| .register(meterRegistry) | |
| .increment(); | |
| } | |
| final boolean isWhitelisted = difficultyService.isSourceAddressWhitelisted(siteKey, sourceAddress); | |
| long sameSourceAddressRequestCount = getSameSourceAddressRequestCount(sourceAddress); | |
| Counter.builder("captcha.challenge.requests") | |
| .tag("site_key", siteKey) | |
| .tag("difficulty", String.valueOf(difficulty)) | |
| .tag("is_whitelisted", String.valueOf(isWhitelisted)) | |
| .description("Counter for captcha challenge requests") | |
| .register(meterRegistry) | |
| .increment(); | |
| } |
Comment on lines
+51
to
59
| final boolean isWhitelisted = difficultyService.isSourceAddressWhitelisted(siteKey, sourceAddress); | ||
| long sameSourceAddressRequestCount = getSameSourceAddressRequestCount(sourceAddress); | ||
|
|
||
| Counter.builder("captcha.verify.success") | ||
| .tag("site_key", siteKey) | ||
| .tag("difficulty", String.valueOf(difficulty)) | ||
| .tag("same_source_address_request_count", String.valueOf(sameSourceAddressRequestCount)) | ||
| .tag("is_whitelisted", String.valueOf(isWhitelisted)) | ||
| .description("Counter for captcha verify success requests") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid duplicate lookups if possible.
If DifficultyService can expose a single call returning both difficulty and whitelisted status, prefer that to reduce I/O/CPU on hot paths. Otherwise, LGTM.
🤖 Prompt for AI Agents
In
captchaservice-backend/src/main/java/de/muenchen/captchaservice/service/metrics/MetricsService.java
around lines 51 to 59, the code calls difficultyService twice to obtain
difficulty and whitelisted status which causes duplicate lookups on a hot path;
change DifficultyService to expose a single method (e.g., getDifficultyInfo or a
DTO like DifficultyInfo {int difficulty; boolean isWhitelisted}) that returns
both values in one call, update the service implementation accordingly, and
replace the two separate calls in MetricsService with a single call that unpacks
the difficulty and isWhitelisted values for the metric tags.
Comment on lines
70
to
79
| final long difficulty = difficultyService.getDifficultyForSourceAddress(siteKey, sourceAddress); | ||
| final boolean isWhitelisted = difficultyService.isSourceAddressWhitelisted(siteKey, sourceAddress); | ||
| long sameSourceAddressRequestCount = getSameSourceAddressRequestCount(sourceAddress); | ||
|
|
||
| DistributionSummary.builder("captcha.client.solve.time") | ||
| .tag("site_key", siteKey) | ||
| .tag("difficulty", String.valueOf(difficulty)) | ||
| .tag("same_source_address_request_count", String.valueOf(sameSourceAddressRequestCount)) | ||
| .tag("is_whitelisted", String.valueOf(isWhitelisted)) | ||
| .description("Summary of the time taken by clients to solve captcha challenges") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consistent tagging; please add whitelisted-path coverage.
Tagging is consistent. Add tests that assert is_whitelisted=true for verify and client.solve.time as well.
🤖 Prompt for AI Agents
In
captchaservice-backend/src/main/java/de/muenchen/captchaservice/service/metrics/MetricsService.java
around lines 70 to 79, tests and metric coverage are missing for the
whitelisted-path case; add unit/integration tests that exercise the whitelisted
verification and client.solve.time flows with a whitelisted sourceAddress so the
emitted metrics include is_whitelisted=true, and assert the metric tags for both
"verify" and "captcha.client.solve.time" contain is_whitelisted=true (and
include any whitelisted-path tag if your metric schema requires it); ensure the
test sets up difficultyService.isSourceAddressWhitelisted(...) to return true,
invokes the verify and client solve endpoints/services, and asserts the created
DistributionSummary/metric tags contain the expected whitelisted values.
| @@ -55,6 +55,7 @@ class CaptchaControllerTest { | |||
| private static final String TEST_SITE_SECRET = "test_secret"; | |||
| private static final String TEST_HMAC_KEY = "secret"; | |||
| private static final String TEST_CLIENT_ADDRESS = "1.2.3.4"; | |||
| private static final String TEST_WHITELISTED_ADDRESS = "10.0.0.0"; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use a host address, not the network address.
10.0.0.0 can be treated as a network address. Prefer 10.0.0.1 (still private/whitelisted) to avoid edge‑case parser behavior.
- private static final String TEST_WHITELISTED_ADDRESS = "10.0.0.0";
+ private static final String TEST_WHITELISTED_ADDRESS = "10.0.0.1";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static final String TEST_WHITELISTED_ADDRESS = "10.0.0.0"; | |
| private static final String TEST_WHITELISTED_ADDRESS = "10.0.0.1"; |
🤖 Prompt for AI Agents
In
captchaservice-backend/src/test/java/de/muenchen/captchaservice/controller/captcha/CaptchaControllerTest.java
around line 58, the test uses the network address "10.0.0.0" as
TEST_WHITELISTED_ADDRESS which can be interpreted as a network rather than a
host; change the constant to a valid host address such as "10.0.0.1" (or another
private host IP) so parsers treat it as a host and update any dependent
assertions or mocks if they reference the constant.
Comment on lines
+240
to
+260
| @Test | ||
| @SneakyThrows | ||
| void testChallengeMetricsWhiteListedSourceAddress() { | ||
| final PostChallengeRequest challengeRequest = new PostChallengeRequest(TEST_SITE_KEY, TEST_SITE_SECRET, TEST_WHITELISTED_ADDRESS); | ||
| final String challengeRequestBody = objectMapper.writeValueAsString(challengeRequest); | ||
|
|
||
| mockMvc.perform( | ||
| post("/api/v1/captcha/challenge") | ||
| .content(challengeRequestBody) | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andExpect(status().isOk()); | ||
|
|
||
| mockMvc.perform( | ||
| get("/actuator/metrics/captcha.challenge.requests")) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.measurements[0].value", is(1.0))) | ||
| .andExpect(jsonPath("$.availableTags[?(@.tag=='difficulty')].values[0]", hasItem("1"))) | ||
| .andExpect(jsonPath("$.availableTags[?(@.tag=='site_key')].values[0]", hasItem("test_site"))) | ||
| .andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')].values[*]", hasItem("1"))) | ||
| .andExpect(jsonPath("$.availableTags[?(@.tag=='is_whitelisted')].values[0]", hasItem("true"))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Stabilize test preconditions and whitelist source.
- Clear DB before asserting same_source_address_request_count == 1 to avoid flakiness from leftover state.
- Ensure the whitelist actually contains TEST_WHITELISTED_ADDRESS in tests (property or stub DifficultyService) so the test isn’t env‑dependent.
@Test
@SneakyThrows
void testChallengeMetricsWhiteListedSourceAddress() {
+ databaseTestUtil.clearDatabase();
final PostChallengeRequest challengeRequest = new PostChallengeRequest(TEST_SITE_KEY, TEST_SITE_SECRET, TEST_WHITELISTED_ADDRESS);If needed, I can add a @DynamicPropertySource to inject the whitelist CIDR for tests.
Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+314
to
316
| .andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')]").exists()) | ||
| .andExpect(jsonPath("$.availableTags[?(@.tag=='is_whitelisted')].values[0]", hasItem("false"))); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extend coverage to whitelisted verify/solve‑time.
Consider adding mirror tests using TEST_WHITELISTED_ADDRESS to assert is_whitelisted=true for captcha.verify.success and captcha.client.solve.time.
Also applies to: 324-326
🤖 Prompt for AI Agents
In
captchaservice-backend/src/test/java/de/muenchen/captchaservice/controller/captcha/CaptchaControllerTest.java
around lines 314-316 (and similarly 324-326), tests assert is_whitelisted=false
for certain metrics but lack mirror cases for a whitelisted address; add
parallel test flows using TEST_WHITELISTED_ADDRESS that exercise
captcha.verify.success and captcha.client.solve.time and assert the JSON
responses contain is_whitelisted=true (e.g., the availableTags entries for these
events should have is_whitelisted true and related value assertions); ensure the
test setup uses the whitelisted constant for request origin, performs the same
requests as the existing non-whitelisted tests, and adds jsonPath assertions
checking is_whitelisted equals true.
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.
Pull Request
Changes
is_whitelistedtag to the exisiting metricsChecklist
General
Code
console.log), see code quality toolingBackend
Summary by CodeRabbit