chore: WPB-28096 Enable JVM metrics and add business metrics

[bbaarriiss]

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

Solutions

• Enable JVM metrics
• Add business metrics that will help us to make further plans for the app itself

How to test

• On Staging WebApp, create a channel and add the app.
• Add the given webhook url to the repo you want to test.
• Enable all events to be able to receive also unsupported events from Github
• Verify the existence of the business metrics on Grafana.
• Verify the existence of JVM metrics on Grafana

[claude]

Code review: Prometheus metrics integration

Noting the PR body says "TEST, DO NOT REVIEW" — I was explicitly asked to review, so here it is. Ignore freely if this branch is a throwaway.

Overall this is a well-shaped change. The UsageMetrics facade keeps Micrometer out of the handlers, the funnel design (received → sent / unsupported) is the right instrumentation shape for this app, and the KDoc explaining why each counter exists rather than what it counts is genuinely above average. The unsupported_events counter as a prioritisation signal for which templates to write next is a nice idea.

There is one issue I'd want fixed before merge (label cardinality), one deployment question, and a handful of smaller things.

---

1. Unbounded label cardinality — client-controlled tag values 🔴

src/main/kotlin/com/wire/github/metrics/UsageMetrics.kt

Both tag values are caller-controlled:

• event comes from the X-GitHub-Event header, and the HMAC in SignatureValidator covers only the payload — headers are not authenticated at all.
• action comes from the deserialized JSON body, i.e. also whatever the sender put there.

Micrometer registers a new Counter per distinct tag combination and never evicts meters. So anything that can pass signature validation once (a conversation owner, a leaked or rotated-away secret, a buggy integration) can grow the registry without bound by varying the header/action: steady heap growth plus a /metrics scrape body that gets larger and slower forever. With a 512Mi memory limit (helm/githubapp/values.yaml) this is a realistic OOM path, and it is also the classic Prometheus cardinality blowup that takes the scrape target down with it.

githubapp_unsupported_events_total is the worst case: it is unbounded × unbounded, and it is specifically the counter for values you haven't modelled.

Suggested fix — clamp to a known set:

private fun eventTag(event: String) =
    if (event in KNOWN_EVENTS) event else OTHER

private fun actionTag(action: String?) = when {
    action == null -> NO_ACTION
    action in KNOWN_ACTIONS -> action
    else -> OTHER
}

KNOWN_EVENTS can be a literal set of the GitHub events you support, or derived from the templates/en/ resource names so it stays in sync with TemplateHandler by construction. Keeping an other bucket means you still see that something unknown arrived, you just don't let the sender name your label values.

2. /metrics is unauthenticated on the same port as the public webhook 🟠

src/main/kotlin/com/wire/github/metrics/Metrics.kt registers /metrics in the same routing tree as the webhook endpoint, and values.yaml defaults ingress.hosts[].paths to path: / with pathType: Prefix. So wherever the ingress is enabled, /metrics is internet-reachable with no auth. Nothing requires that — helm/githubapp/templates/servicemonitor.yaml scrapes the ClusterIP service directly.

Options, roughly in order of preference:

• run the metrics endpoint on a second port not routed by the ingress (and point the ServiceMonitor at it),
• or add an ingress rule denying /metrics,
• or gate it behind a bearer token / ServiceMonitor.bearerTokenSecret.

At minimum this deserves a deliberate decision plus a line in helm/githubapp/DEPLOYMENT.md. The leak itself is modest (event-type mix, request counts, URI templates, JVM internals) but it is free to avoid.

3. The hard-coded 3.4.0 pin papers over a version misalignment 🟠

gradle/libs.versions.toml:

# Pinned to the version koin-ktor already forces onto ktor-server-core; 3.2.3 is binary incompatible with it.
ktor-server-metrics-micrometer = { module = "io.ktor:ktor-server-metrics-micrometer", version = "3.4.0" }

The comment is doing useful archaeology — but the conclusion it points to is that ktor-version = "3.2.3" is already a fiction: everything resolves to 3.4.0 at runtime via koin-ktor, including ktor-server-core. Pinning one artifact leaves a trap: the next bump of ktor-version (say to 3.5.0) silently leaves metrics at 3.4.0 and reintroduces exactly the incompatibility this comment is about. The Ktor Gradle plugin is also still on 3.2.3.

Cleanest is to bump ktor-version to 3.4.0 so every Ktor artifact and the plugin move in lockstep, and drop the special case. If you would rather not touch the other deps in this PR, at least give it a named ref (ktor-metrics-version = "3.4.0") so it is visible in [versions] where someone doing a bump will actually see it.

4. The missing-secret → 403 change is unrelated, untested, and uncounted 🟡

src/main/kotlin/com/wire/github/Routing.kt. The reasoning in the comment is correct and the behaviour is an improvement over a redelivery-triggering 500 — but three things:

• Scope: this is a behaviour change to signature handling inside an "integrate prometheus metrics" PR. Worth splitting, or at least calling out in the description, since it changes what GitHub sees.
• Signalling via IOException is fragile. SignatureValidator.isValid is declared @Throws(Exception::class) and throws a generic IOException("Missing secret..."). A dedicated MissingSecretException (or returning a sealed result / Boolean?) makes the contract explicit and will not quietly start swallowing an unrelated IOException after a refactor of the storage layer.
• No test. This is a new branch with a new status code and there is no case for it. every { mockRedisCommands.get(any()) } returns null → assert 403 is a three-line test.

Related gap: the funnel currently starts at "accepted", so rejected deliveries are invisible to metrics entirely — which is the thing you would most want to alert on ("this conversation's webhook has been 403ing for an hour"). A low-cardinality onRejectedDelivery(reason = "missing_secret" | "invalid_signature") would close that.

5. No counter for send failures 🟡

onNotificationSent only fires after wireAppSdk.getApplicationManager().sendMessage(...) returns. If the SDK throws, the request 500s and the metrics show a received with no matching sent or unsupported — indistinguishable from a request still in flight. A try/catch around the send incrementing onNotificationFailed(event) makes the funnel actually sum to the total.

6. Metrics setup details 🟡

src/main/kotlin/com/wire/github/metrics/Metrics.kt / Application.kt:

• Install order: module() calls configureRouting() then configureMetrics(). Ktor 3's MicrometerMetrics is hook/event-based so this should work, but installing observability plugins before routing is the conventional order and removes the need to reason about it.
• No JVM binders: PrometheusConfig.DEFAULT on its own gives you HTTP timers plus your custom counters. The standard binders (JvmMemoryMetrics, JvmGcMetrics, JvmThreadMetrics, ProcessorMetrics, ClassLoaderMetrics, UptimeMetrics) are cheap and are what you will want the first time this pod gets OOMKilled at 512Mi. The plugin config accepts them via meterBinders, and commonTags is a good place for an app/env label.
• Content type: call.respond(prometheusRegistry.scrape()) will emit text/plain; charset=UTF-8 (ContentNegotiation ignores String by default, so there is no accidental-JSON risk), but the type Prometheus expects is text/plain; version=0.0.4. call.respondText(registry.scrape(), ContentType.Text.Plain) is more explicit, and registry.scrape(contentType) lets you honour OpenMetrics negotiation if you ever want exemplars.

7. Counter re-registration on every increment 🔵

Each increment() builds a fresh Counter.Builder, re-declares the description, allocates Tags, and calls register just to get the already-cached meter back. Functionally correct, and irrelevant at webhook volume — but registry.counter(NAME, TAG_EVENT, event) is the idiomatic one-liner for the dynamic case, or cache in a ConcurrentHashMap<String, Counter> if you want the builder's description preserved.

---

Test coverage

Good instinct testing through the real /metrics scrape output rather than mocking the registry — that catches naming and _total-suffix surprises. Some nits:

• assertTrue(metrics.contains("githubapp_notifications_sent_total") == false, ...) → assertFalse(metrics.contains(...), ...).
• The exact-string tag assertions ({action="created",event="$DUMMY_EVENT"} 1.0) depend on Micrometer's label ordering, escaping, and float formatting. They pass today and will break confusingly on a library bump. Either a small regex, or separate contains checks for the metric name and each label="value", would be more durable.
• Missing cases: 403 on missing secret (see chore: Add Fixtures and Templates #4); counters not incremented on an invalid signature; /metrics returning 200 against an empty registry.
• The two new funnel tests use the non-expression testApplication { } form with loadKoinModules outside it, matching the existing pull_request test — good, consistent.

Docs

README.md has an endpoint table (lines 28–29) listing /health and the webhook path; /metrics should be added there. helm/githubapp/DEPLOYMENT.md is the right home for the exposure note from #2.

Small adjacent thing

This PR reasons carefully about returning 403 instead of a 500 that GitHub would redeliver — while requireNotNull(event) / requireNotNull(signature) / requireNotNull(delivery) a few lines above still throw IllegalArgumentException → 500 on a missing header. Converting those to 400 responses is a two-minute change and fits the same intent, if you want it in scope.

---

Summary: #1 (cardinality) is the one I would block on — it is a memory/availability issue reachable by anyone holding a webhook secret. #2 and #3 are decisions worth making explicitly now rather than discovering later. Everything else is polish.

[bbaarriiss]

Hey @claude , I can understand your concern about "Unbounded label cardinality — client-controlled tag values" but the values are actually limited. These event are the event Github sends us. So, no worries about it.

[claude]

Claude finished @bbaarriiss's task in 2m 21s —— View job

---

Re: label cardinality — you're mostly right, downgrading it

Thanks for pushing back. I went back through SignatureValidator, Routing.kt and TemplateHandler — you're right on the substance, and I was wrong to treat the two tags as equivalent. Dropping this from a blocker.

Where you're right

The benign cardinality is genuinely fine. GitHub has ~75 webhook event types, most with a handful of actions; templates/en/ covers 6 event types across 11 templates. Even if a conversation subscribes to "send me everything", githubapp_unsupported_events_total tops out around a few hundred series — low single-digit MB in the registry, nothing against a 512Mi limit. Not a memory problem, and not a scrape problem.

I was also wrong to lump action in with event. action comes from the deserialized body (Routing.kt:128), and the HMAC does cover the payload — so varying it requires the secret and a re-sign. That tag is bounded by GitHub's schema in the way you describe. My original point #1 shouldn't have included it.

The one narrow thing that isn't quite convention-free

event comes from the X-GitHub-Event header, and SignatureValidator.isValid HMACs payload only:

// SignatureValidator.kt:28
val generatedHmacSha1: String = generateHmacSha1(payload, secret)

The header isn't in the signed material, and there's no delivery-ID dedup or timestamp check — X-GitHub-Delivery is read at Routing.kt:55 and only used for logging. So a single valid (payload, signature) pair replays indefinitely with a different X-GitHub-Event each time and still passes validation.

That said, the realism caveat matters and cuts your way: getting a valid signed payload means being a repo admin, or reading one out of GitHub's webhook delivery UI — i.e. someone who already holds the secret. It's not an anonymous-internet path, it's "a party you already trust can make a shared service's registry grow." That's a much smaller claim than the one I made.

So: the values are the ones GitHub sends, in practice. They just aren't constrained to that by the code — it's a convention, not an invariant.

If you want the cheap version

Not asking for the KNOWN_EVENTS machinery I sketched. A clamp on the one tag is ~5 lines in UsageMetrics.kt and ends the conversation:

private fun eventTag(event: String) = if (event in KNOWN_EVENTS) event else "other"

Worth noting it'd close an adjacent thing for free: event is also interpolated straight into a classpath resource path (TemplateHandler.kt:43), so the same unvalidated header picks which *.template resource gets compiled and rendered into a Wire conversation. The .template suffix constrains that to near-nothing in practice — I'd call it a curiosity, not a finding — but one clamp covers both. Your call; happy to leave it as-is.

Add the event tag clamp →

---

The rest of the review (#2 metrics endpoint exposure, #3 the 3.4.0 pin, #4 the 403 change, #5 send failures, #6 setup, #7 re-registration) is unaffected by this and still stands as written.

Original review, for reference

Code review: Prometheus metrics integration

Noting the PR body says "TEST, DO NOT REVIEW" — I was explicitly asked to review, so here it is. Ignore freely if this branch is a throwaway.

Overall this is a well-shaped change. The UsageMetrics facade keeps Micrometer out of the handlers, the funnel design (received → sent / unsupported) is the right instrumentation shape for this app, and the KDoc explaining why each counter exists rather than what it counts is genuinely above average. The unsupported_events counter as a prioritisation signal for which templates to write next is a nice idea.

There is one issue I'd want fixed before merge (label cardinality), one deployment question, and a handful of smaller things.

---

1. Unbounded label cardinality — client-controlled tag values 🔴

(superseded — see reply above)

src/main/kotlin/com/wire/github/metrics/UsageMetrics.kt

Both tag values are caller-controlled:

• event comes from the X-GitHub-Event header, and the HMAC in SignatureValidator covers only the payload — headers are not authenticated at all.
• action comes from the deserialized JSON body, i.e. also whatever the sender put there.

Micrometer registers a new Counter per distinct tag combination and never evicts meters. So anything that can pass signature validation once (a conversation owner, a leaked or rotated-away secret, a buggy integration) can grow the registry without bound by varying the header/action: steady heap growth plus a /metrics scrape body that gets larger and slower forever. With a 512Mi memory limit (helm/githubapp/values.yaml) this is a realistic OOM path, and it is also the classic Prometheus cardinality blowup that takes the scrape target down with it.

githubapp_unsupported_events_total is the worst case: it is unbounded × unbounded, and it is specifically the counter for values you haven't modelled.

Suggested fix — clamp to a known set:

private fun eventTag(event: String) =
    if (event in KNOWN_EVENTS) event else OTHER

private fun actionTag(action: String?) = when {
    action == null -> NO_ACTION
    action in KNOWN_ACTIONS -> action
    else -> OTHER
}

KNOWN_EVENTS can be a literal set of the GitHub events you support, or derived from the templates/en/ resource names so it stays in sync with TemplateHandler by construction. Keeping an other bucket means you still see that something unknown arrived, you just don't let the sender name your label values.

2. /metrics is unauthenticated on the same port as the public webhook 🟠

src/main/kotlin/com/wire/github/metrics/Metrics.kt registers /metrics in the same routing tree as the webhook endpoint, and values.yaml defaults ingress.hosts[].paths to path: / with pathType: Prefix. So wherever the ingress is enabled, /metrics is internet-reachable with no auth. Nothing requires that — helm/githubapp/templates/servicemonitor.yaml scrapes the ClusterIP service directly.

Options, roughly in order of preference:

• run the metrics endpoint on a second port not routed by the ingress (and point the ServiceMonitor at it),
• or add an ingress rule denying /metrics,
• or gate it behind a bearer token / ServiceMonitor.bearerTokenSecret.

At minimum this deserves a deliberate decision plus a line in helm/githubapp/DEPLOYMENT.md. The leak itself is modest (event-type mix, request counts, URI templates, JVM internals) but it is free to avoid.

3. The hard-coded 3.4.0 pin papers over a version misalignment 🟠

gradle/libs.versions.toml:

# Pinned to the version koin-ktor already forces onto ktor-server-core; 3.2.3 is binary incompatible with it.
ktor-server-metrics-micrometer = { module = "io.ktor:ktor-server-metrics-micrometer", version = "3.4.0" }

The comment is doing useful archaeology — but the conclusion it points to is that ktor-version = "3.2.3" is already a fiction: everything resolves to 3.4.0 at runtime via koin-ktor, including ktor-server-core. Pinning one artifact leaves a trap: the next bump of ktor-version (say to 3.5.0) silently leaves metrics at 3.4.0 and reintroduces exactly the incompatibility this comment is about. The Ktor Gradle plugin is also still on 3.2.3.

Cleanest is to bump ktor-version to 3.4.0 so every Ktor artifact and the plugin move in lockstep, and drop the special case. If you would rather not touch the other deps in this PR, at least give it a named ref (ktor-metrics-version = "3.4.0") so it is visible in [versions] where someone doing a bump will actually see it.

4. The missing-secret → 403 change is unrelated, untested, and uncounted 🟡

src/main/kotlin/com/wire/github/Routing.kt. The reasoning in the comment is correct and the behaviour is an improvement over a redelivery-triggering 500 — but three things:

• Scope: this is a behaviour change to signature handling inside an "integrate prometheus metrics" PR. Worth splitting, or at least calling out in the description, since it changes what GitHub sees.
• Signalling via IOException is fragile. SignatureValidator.isValid is declared @Throws(Exception::class) and throws a generic IOException("Missing secret..."). A dedicated MissingSecretException (or returning a sealed result / Boolean?) makes the contract explicit and will not quietly start swallowing an unrelated IOException after a refactor of the storage layer.
• No test. This is a new branch with a new status code and there is no case for it. every { mockRedisCommands.get(any()) } returns null → assert 403 is a three-line test.

Related gap: the funnel currently starts at "accepted", so rejected deliveries are invisible to metrics entirely — which is the thing you would most want to alert on ("this conversation's webhook has been 403ing for an hour"). A low-cardinality onRejectedDelivery(reason = "missing_secret" | "invalid_signature") would close that.

5. No counter for send failures 🟡

onNotificationSent only fires after wireAppSdk.getApplicationManager().sendMessage(...) returns. If the SDK throws, the request 500s and the metrics show a received with no matching sent or unsupported — indistinguishable from a request still in flight. A try/catch around the send incrementing onNotificationFailed(event) makes the funnel actually sum to the total.

6. Metrics setup details 🟡

src/main/kotlin/com/wire/github/metrics/Metrics.kt / Application.kt:

• Install order: module() calls configureRouting() then configureMetrics(). Ktor 3's MicrometerMetrics is hook/event-based so this should work, but installing observability plugins before routing is the conventional order and removes the need to reason about it.
• No JVM binders: PrometheusConfig.DEFAULT on its own gives you HTTP timers plus your custom counters. The standard binders (JvmMemoryMetrics, JvmGcMetrics, JvmThreadMetrics, ProcessorMetrics, ClassLoaderMetrics, UptimeMetrics) are cheap and are what you will want the first time this pod gets OOMKilled at 512Mi. The plugin config accepts them via meterBinders, and commonTags is a good place for an app/env label.
• Content type: call.respond(prometheusRegistry.scrape()) will emit text/plain; charset=UTF-8 (ContentNegotiation ignores String by default, so there is no accidental-JSON risk), but the type Prometheus expects is text/plain; version=0.0.4. call.respondText(registry.scrape(), ContentType.Text.Plain) is more explicit, and registry.scrape(contentType) lets you honour OpenMetrics negotiation if you ever want exemplars.

7. Counter re-registration on every increment 🔵

Each increment() builds a fresh Counter.Builder, re-declares the description, allocates Tags, and calls register just to get the already-cached meter back. Functionally correct, and irrelevant at webhook volume — but registry.counter(NAME, TAG_EVENT, event) is the idiomatic one-liner for the dynamic case, or cache in a ConcurrentHashMap<String, Counter> if you want the builder's description preserved.

---

Test coverage

Good instinct testing through the real /metrics scrape output rather than mocking the registry — that catches naming and _total-suffix surprises. Some nits:

• assertTrue(metrics.contains("githubapp_notifications_sent_total") == false, ...) → assertFalse(metrics.contains(...), ...).
• The exact-string tag assertions ({action="created",event="$DUMMY_EVENT"} 1.0) depend on Micrometer's label ordering, escaping, and float formatting. They pass today and will break confusingly on a library bump. Either a small regex, or separate contains checks for the metric name and each label="value", would be more durable.
• Missing cases: 403 on missing secret (see chore: Add Fixtures and Templates #4); counters not incremented on an invalid signature; /metrics returning 200 against an empty registry.
• The two new funnel tests use the non-expression testApplication { } form with loadKoinModules outside it, matching the existing pull_request test — good, consistent.

Docs

README.md has an endpoint table (lines 28–29) listing /health and the webhook path; /metrics should be added there. helm/githubapp/DEPLOYMENT.md is the right home for the exposure note from #2.

Small adjacent thing

This PR reasons carefully about returning 403 instead of a 500 that GitHub would redeliver — while requireNotNull(event) / requireNotNull(signature) / requireNotNull(delivery) a few lines above still throw IllegalArgumentException → 500 on a missing header. Converting those to 400 responses is a two-minute change and fits the same intent, if you want it in scope.

---

Revised summary: no blockers. #2 and #3 are decisions worth making explicitly now rather than discovering later; everything else is polish.

No code changes made — this was a discussion, not an implementation request.
· branch chore/WPB-28096-integrate-prometheus-metrics

[bbaarriiss]

📣 Now the PR is ready for review.

Note: I deleted the comments I've added in this PR to test certain cases about Github events and verify them on Grafana.