Skip to content

Add is_whitelisted tag to captcha metrics #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add is_whitelisted tag to captcha metrics #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...vice-backend/src/main/java/de/muenchen/captchaservice/service/metrics/MetricsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,29 @@ public MetricsService(MeterRegistry meterRegistry, DifficultyService difficultyS
}

public void recordChallengeRequest(String siteKey, long difficulty, SourceAddress sourceAddress) {
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();
}
Comment on lines +36 to 47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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();
}


public void recordVerifySuccess(String siteKey, SourceAddress sourceAddress) {
final long difficulty = difficultyService.getDifficultyForSourceAddress(siteKey, sourceAddress);
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")
Comment on lines +51 to 59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

.register(meterRegistry)
.increment();
Expand All @@ -64,12 +68,14 @@ public void recordClientSolveTime(String siteKey, SourceAddress sourceAddress, L
}

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")
Comment on lines 70 to 79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

.baseUnit("milliseconds")
.register(meterRegistry)
Expand Down
32 changes: 29 additions & 3 deletions ...nd/src/test/java/de/muenchen/captchaservice/controller/captcha/CaptchaControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

private static final ExtendedPayload TEST_PAYLOAD;

static {
Expand Down Expand Up @@ -236,6 +237,28 @@ void postVerify_expired() {
}
}

@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")));
}
Comment on lines +240 to +260
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


@Test
@SneakyThrows
void testChallengeMetricsIncrement() {
Expand All @@ -256,7 +279,8 @@ void testChallengeMetricsIncrement() {
.andExpect(jsonPath("$.measurements[0].value", is((double) i)))
.andExpect(jsonPath("$.availableTags[?(@.tag=='difficulty')].values[0]", hasItem("1000")))
.andExpect(jsonPath("$.availableTags[?(@.tag=='site_key')].values[0]", hasItem("test_site")))
.andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')].values[*]", hasItem(String.valueOf(i))));
.andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')].values[*]", hasItem(String.valueOf(i))))
.andExpect(jsonPath("$.availableTags[?(@.tag=='is_whitelisted')].values[0]", hasItem("false")));
}
}

Expand Down Expand Up @@ -287,7 +311,8 @@ void testVerifyMetricsIncrement() {
.andExpect(jsonPath("$.measurements[0].value", is((double) i)))
.andExpect(jsonPath("$.availableTags[?(@.tag=='difficulty')].values[0]", hasItem("1000")))
.andExpect(jsonPath("$.availableTags[?(@.tag=='site_key')].values[0]", hasItem("test_site")))
.andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')]").exists());
.andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')]").exists())
.andExpect(jsonPath("$.availableTags[?(@.tag=='is_whitelisted')].values[0]", hasItem("false")));

Comment on lines +314 to 316
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

mockMvc.perform(get("/actuator/metrics/captcha.client.solve.time"))
.andExpect(status().isOk())
Expand All @@ -296,7 +321,8 @@ void testVerifyMetricsIncrement() {
.andExpect(jsonPath("$.measurements[?(@.statistic=='MAX')].value", hasItem((double) TEST_PAYLOAD.getTook())))
.andExpect(jsonPath("$.availableTags[?(@.tag=='difficulty')].values[0]", hasItem("1000")))
.andExpect(jsonPath("$.availableTags[?(@.tag=='site_key')].values[0]", hasItem("test_site")))
.andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')]").exists());
.andExpect(jsonPath("$.availableTags[?(@.tag=='same_source_address_request_count')]").exists())
.andExpect(jsonPath("$.availableTags[?(@.tag=='is_whitelisted')].values[0]", hasItem("false")));

databaseTestUtil.clearDatabase();
}
Expand Down