Skip to content

fix(scale): sanitize the configured Prometheus namespace, not just the Kubernetes fallback (#8688) - #8691

Merged
kugesan1105 merged 13 commits into
jaseci-labs:mainfrom
akindu-k:fix/sanitize-configured-prometheus-namespace-8688
Sep 7, 2026
Merged

fix(scale): sanitize the configured Prometheus namespace, not just the Kubernetes fallback (#8688)#8691
kugesan1105 merged 13 commits into
jaseci-labs:mainfrom
akindu-k:fix/sanitize-configured-prometheus-namespace-8688

Conversation

@akindu-k

@akindu-k akindu-k commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Description

image

Fixes #8688.

Every metric this server exports starts with a prefix, for example jac_shop_http_requests_total. That prefix comes from jac.toml, and it can come from two places: [scale.monitoring] namespace if you set it, otherwise the [scale.kubernetes] namespace fallback.

Prometheus only allows letters, digits and underscores in a prefix, so sanitize_prometheus_namespace exists to clean the value. The bug: that call sat inside the fallback branch, so it only ever ran on the fallback. A prefix you configured yourself was used exactly as typed.

Nothing crashed, because prometheus_client also cleans illegal names on the way out. But it cleans them by different rules. So the same value produced two different metric names depending on which setting it came from.

Same value, two prefixes, on main:

namespace via [scale.monitoring] via [scale.kubernetes] fallback
jac--shop jac__shop_http_requests_total jac_shop_http_requests_total
2024app _024app_... (leading digit lost) _2024app_... (digit kept)
--- ____... jaclang_scale_...

Dashboards and alert rules written against one path go blank on the other, and nothing reports an error.

The fix

_resolve_prometheus_namespace now owns the decision for both paths, so there is no longer a route around the cleanup:

impl _resolve_prometheus_namespace(configured: str | None, k8s_namespace: str) -> str {
    if not configured {
        return sanitize_prometheus_namespace(k8s_namespace);
    }
    sanitized = sanitize_prometheus_namespace(configured);
    if sanitized != configured and not _prometheus_namespace_warned {
        _prometheus_namespace_warned = True;
        ...
    }
    return sanitized;
}

The call site in get_monitoring_config() loses its branch entirely. Both values are handed to one function, and that function decides.

Renaming a metric breaks dashboards silently, so a rewritten value logs a warning naming both the old and the new prefix. A module flag stops it repeating on every config read, and reset_scale_config() clears that flag, so a second project loaded in the same process still gets its own warning instead of being renamed in silence.

Validation

All checks run against real config files, a real server and real Prometheus output. No mocks.

  • End to end, same project on both revisions. A project with [scale.monitoring] enabled = true and namespace = "jac--shop", served with jac run --serve and scraped at /metrics with admin basic auth:

    exported families warning
    main jac__shop_http_requests_total, jac__shop_http_request_duration_seconds, ... none
    this branch jac_shop_http_requests_total, jac_shop_http_request_duration_seconds, ... logged at startup

    Zero jac__shop occurrences in the scrape after the fix, with live samples such as jac_shop_http_requests_total{method="GET",path="/healthz",status_code="200"} 1.0.

  • One new test, one extended. A table-driven test in test_config_loader.jac writes five real jac.toml projects and reads each back through get_scale_config(): a dashed value is sanitized, an already valid value is left alone and stays silent, "---" falls back to jaclang_scale, a non-string value such as 2024 is coerced to _2024, and an empty value still derives from [scale.kubernetes]. Every rewritten value must warn about itself through the process logging stack, naming the old and new prefix and what to update, which also pins down the per-project flag reset. The existing custom-namespace test in test_metrics.jac now records a request and asserts the namespace reaches the real expose() output.

  • Failure check. Reverting only the two source files fails the behaviour tests and leaves the guard tests passing, so the guards encode behaviour that must not change rather than the new behaviour.

  • Suites. 47 passed across test_config_loader.jac and test_metrics.jac. jac check clean on all changed files, jac fmt produces no diff.

Note on the release note

Filed under breaking rather than bugfix. The fix is corrective, but anyone whose configured namespace contains characters outside [a-zA-Z0-9_] sees exported metric names change once, which breaks dashboards and alert rules that reference the old prefix. The startup warning exists so that change is visible rather than silent.

Out of scope

The sanitizer's own rules are unchanged. This PR only decides which inputs it is applied to.

Maintainer update (2026-09-07)

Validated on top of current main by running the loader, the app collector, the gateway middleware, and the admin traffic query builder against the same projects, plus a real jac run --serve scrape and a run against prometheus_client 0.21.0, the capability pin floor. All three consumers now agree on jac_shop_*, the admin PromQL parses, the pin-floor client no longer raises Invalid metric name at startup, and the warning appears once in the structured server log.

Changes added on top of the original branch:

  • Merged main (the glob block and reset_scale_config had drifted).
  • A non-string TOML namespace was hitting the sanitizer's regex as an int and raising TypeError at config read; it is now coerced to str first.
  • The subprocess test ran jac run with the temp project as cwd, which in a dev checkout resolves the installed jaclang rather than the branch under test and fails with "namespace was not sanitized". The seven single-case tests are folded into one table-driven loader test that asserts the namespace and the warning per project, and the exposition check moves onto the existing custom-namespace collector test.
  • The monitoring reference table and the config schema description now state that the value is sanitized and that a rewrite warns at startup.

…e fallback

sanitize_prometheus_namespace only ran on the Kubernetes fallback, so
[scale.monitoring] namespace reached prometheus_client untouched and the
client normalized it differently: "jac--shop" exported jac__shop_* through
the configured path and jac_shop_* through the derived one, and "2024app"
lost its leading digit through the client but kept it through the sanitizer.

resolve_prometheus_namespace now owns that decision for both paths and warns
once when a configured value is rewritten, since exported metric names change
and dashboards reference them.

@MusabMahmoodh MusabMahmoodh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct, and the tests are the right shape - real jac.toml on disk, a real subprocess for the warning, real expose() output, no mocks anywhere.

Worth naming: this also fixes the admin panel. _metrics_namespace at jac/jaclang/scale/admin/impl/workloads.impl.jac:68 builds PromQL prefixes off the same key, so with namespace = "jac--shop" it was querying jac--shop_http_requests_total while the exporter emitted jac__shop_http_requests_total. The traffic panel was silently empty for anyone with a dashed namespace. Might be worth a line in the PR body, since it makes the breaking label easier to justify.

Two things, neither blocking:

1. The warning is one-shot per process, but the condition it reports is per-project. reset_scale_config clears _scale_config_instance and nothing else, so once _prometheus_namespace_warned is set, the next project loaded in the same process gets its metrics renamed silently - which is the exact outcome the warning exists to prevent. Your own test had to shell out to a subprocess to observe it, which is the tell. One line in reset_scale_config:

impl reset_scale_config -> None {
    _scale_config_instance = None;
    _prometheus_namespace_warned = False;
}

I know _ms_enabled_deprecation_warned has the same shape, so this is following the local idiom - but a deprecation notice that fires once per process is fine, and "your metric names just changed" is not the same kind of message.

2. resolve_prometheus_namespace is public. It goes into config_loader.jac as an exported def next to _ws_positive, which is private in the same glob block, and nothing outside this module calls it. _resolve_prometheus_namespace keeps the module surface where it was.

Checked and fine: import logging; inside the impl body matches how get_config_schema and _metrics_namespace do it here, so no complaint there. Also checked every other reader of get_monitoring_config()["namespace"] - microservice_gateway.impl.jac:698, serve.core.impl.jac:95 and :632 - none of them feed it to a Kubernetes object name, where the underscores would have been illegal. That was the regression I went looking for and it is not there.

akindu-k and others added 3 commits August 28, 2026 17:45
…-8688

Conflict in test_metrics.jac: main added read-tier retry tests where this branch added the sanitized-namespace exposition test. Kept both.
…lver private

The rewritten-namespace warning was one-shot per process while the condition
it reports is per project, so a second project loaded in the same process had
its metric names changed silently. reset_scale_config now clears the flag.

resolve_prometheus_namespace has no caller outside this module, so it is
private now.
@akindu-k

akindu-k commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Explanation

image image

Test Coverage

image

Summary

Before, get_monitoring_config() only sanitized the namespace on the fallback branch, so a namespace you configured yourself reached prometheus_client raw and got normalised by its rules instead of ours — two different prefixes for the same input, and the admin panel’s queries stopped matching the exported names. Now both sources go through one private resolver that always sanitizes, plus a one-per-load warning naming the old and new prefix so nobody’s dashboard breaks silently.

kugesan1105 and others added 4 commits September 7, 2026 10:51
…ess, document the rewrite

A non-string TOML value such as `namespace = 2024` reached the sanitizer's
regex as an int and raised TypeError at config read; it is now coerced to
str first and sanitizes to `_2024`.

The subprocess test ran `jac run` with the temp project as cwd, which in a
dev checkout resolves the installed jaclang rather than the branch under
test and fails with "namespace was not sanitized". The warning body is now
asserted through the in-process logging helper instead, alongside a check
that a valid namespace stays silent.

The monitoring reference table and the config schema description now say
the value is sanitized and that a rewrite warns at startup.
… test

Seven single-case tests and two helpers collapse into one test that
loads five real jac.toml projects through the loader and asserts the
namespace and the warning for each. The exposition check moves onto the
existing custom-namespace collector test instead of a new one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scale: the Prometheus namespace sanitizer skips the configured namespace, so metric names differ by path

4 participants