feat(BA-7317): render prometheus query preset templates with Jinja - #13675
feat(BA-7317): render prometheus query preset templates with Jinja#13675seedspirit wants to merge 8 commits into
Conversation
bb0ab6d to
aa79509
Compare
There was a problem hiding this comment.
Pull request overview
Migrates Prometheus query presets from str.format placeholders to restricted, sandboxed Jinja templates.
Changes:
- Adds Jinja rendering and AST-based validation.
- Migrates stored presets and fixtures.
- Updates APIs, documentation, and tests.
Reviewed changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/manager/services/utilization_metric/test_container_metric.py |
Updates rendered-query expectations. |
tests/unit/manager/services/prometheus_query_preset/test_prometheus_query_preset_service.py |
Updates service fixtures. |
tests/unit/manager/services/idle_checker/test_service.py |
Updates idle-checker template. |
tests/unit/manager/repositories/prometheus_query_preset/test_prometheus_query_preset_repository.py |
Updates repository templates. |
tests/unit/manager/repositories/prometheus_query_preset/test_prometheus_query_preset_options.py |
Updates seed defaults. |
tests/unit/manager/repositories/metric/test_session_utilization.py |
Updates utilization template. |
tests/unit/manager/clients/prometheus/test_preset.py |
Tests Jinja rendering and validation. |
tests/unit/manager/clients/prometheus/test_client.py |
Updates client rendering tests. |
tests/unit/common/dto/manager/v2/prometheus_query_preset/test_request.py |
Tests request validation. |
tests/component/prometheus_query_preset/test_prometheus_query_preset_preview.py |
Updates preview scenarios. |
tests/component/manager/clients/prometheus/test_sd_relabel.py |
Updates relabel fixture. |
tests/component/manager/clients/prometheus/test_client_integration.py |
Updates integration fixture. |
src/ai/backend/manager/models/alembic/versions/4b8e2f7a91d3_convert_prometheus_query_preset_templates_to_jinja.py |
Migrates stored templates. |
src/ai/backend/manager/clients/prometheus/preset.py |
Implements cached Jinja rendering. |
src/ai/backend/manager/clients/prometheus/fixed_query_builder.py |
Converts built-in templates. |
src/ai/backend/manager/api/gql/prometheus_query_preset/types/inputs.py |
Documents GraphQL syntax. |
src/ai/backend/common/dto/manager/v2/prometheus_query_preset/validators.py |
Adds sandbox and AST validation. |
src/ai/backend/common/dto/manager/v2/prometheus_query_preset/request.py |
Documents request fields. |
src/ai/backend/common/data/idle_checker/types.py |
Updates placeholder descriptions. |
fixtures/manager/example-prometheus-query-presets.json |
Converts example presets. |
docs/manager/rest-reference/openapi.json |
Updates REST reference. |
docs/manager/graphql-reference/v2-schema.graphql |
Updates GraphQL schema reference. |
docs/manager/graphql-reference/supergraph.graphql |
Updates supergraph reference. |
changes/13675.feature.md |
Adds release-note fragment. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -0,0 +1 @@ | |||
| Switch Prometheus query preset templates to sandboxed Jinja syntax ({{ labels }}, {{ window }}, {{ group_by }}) with automatic migration of stored presets; the legacy str.format placeholder syntax is no longer accepted | |||
| try: | ||
| jinja2.Environment().parse(template) | ||
| return template |
fregataa
left a comment
There was a problem hiding this comment.
let's add breaking changelog
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the str.format-based query_template engine with a sandboxed Jinja
environment. Templates now use {{ labels }}, {{ window }}, {{ group_by }};
the legacy {placeholder} syntax is rejected at the API boundary with a
guidance message, and a data migration rewrites all stored presets
(including seeded defaults) to the Jinja form. Validation is parser-based:
an AST whitelist permits only literal text and variable substitution, and
StrictUndefined rejects unknown variables.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: octodog <mu001@lablup.com>
…igration Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… head Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… head Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
df02365 to
c3d967a
Compare
| def validate_query_template(template: str) -> None: | ||
| """Validate a Jinja PromQL template; raises ``InvalidMetricPresetTemplate``.""" | ||
| if not template.strip(): | ||
| raise InvalidMetricPresetTemplate("Template must not be empty.") | ||
| unsupported_vars = _UNSUPPORTED_TEMPLATE_VAR_RE.findall(template) | ||
| if unsupported_vars: | ||
| placeholders = ", ".join(f"{{{name}}}" for name in sorted(PLACEHOLDER_NAMES)) | ||
| placeholders = ", ".join(f"{{{{ {name} }}}}" for name in sorted(PLACEHOLDER_NAMES)) | ||
| raise InvalidMetricPresetTemplate( | ||
| f"Unsupported template variables: {unsupported_vars}. " | ||
| f"Use placeholders {placeholders} or literal PromQL values." | ||
| ) | ||
| if _LEGACY_TEMPLATE_RE.search(template): | ||
| raise InvalidMetricPresetTemplate( | ||
| "Legacy str.format template syntax is no longer supported; " | ||
| f"use {{{{ labels }}}}, {{{{ window }}}}, {{{{ group_by }}}}: {template!r}" | ||
| ) | ||
| try: | ||
| ast = PROMQL_TEMPLATE_ENV.parse(template) | ||
| except TemplateSyntaxError as e: | ||
| raise InvalidMetricPresetTemplate(f"Invalid template syntax ({e}): {template!r}") from e | ||
| for node in _walk(ast): | ||
| if not isinstance(node, _ALLOWED_NODE_TYPES): | ||
| raise InvalidMetricPresetTemplate( |
There was a problem hiding this comment.
Why is this implementation here? + Since the implementation places jinja in the global scope, the location of the implementation seems off—please adjust it properly. Even in the Notification Center and other places, values for the implementation are provided in fields, so please do not place variables in the global scope.
There was a problem hiding this comment.
The DTO package should not contain any business logic.
| def _to_jinja(template: str) -> str: | ||
| """Rewrite a legacy ``str.format`` template as Jinja; other templates unchanged.""" | ||
| try: | ||
| parsed = list(string.Formatter().parse(_escape_non_placeholders(template))) | ||
| except ValueError: | ||
| return template | ||
| has_placeholder = False | ||
| for _literal, field, _spec, _conv in parsed: | ||
| if field in ("labels", "window", "group_by"): | ||
| has_placeholder = True | ||
| break | ||
| if not has_placeholder: | ||
| try: | ||
| jinja2.Environment().parse(template) | ||
| return template |
There was a problem hiding this comment.
I'm not sure if I need iLogic for this—can't I just run the format command and replace the values with {{...}}?
There was a problem hiding this comment.
These helpers are a frozen copy of the removed legacy renderer (escape_non_placeholders(template).format(...)).
Plain substitution breaks on legacy-valid templates like metric{{mode!="idle",{labels}}}: replacing only {labels} leaves {{mode!="idle" behind, which Jinja rejects — the legacy {{/}} escapes must also be unfolded into single braces. string.Formatter().parse() does exactly that, with the same parser the legacy renderer used.
Verified the converted output renders identically to the legacy renderer for all seeded presets and the shape above.
Summary
str.format-basedquery_templateengine with a sandboxed Jinja environment (ImmutableSandboxedEnvironment+StrictUndefined); placeholders are now{{ labels }},{{ window }},{{ group_by }}.{placeholder}syntax at the API boundary with a guidance message.4b8e2f7a91d3that rewrites all stored presets (including seeded defaults) to the Jinja form; the conversion is idempotent and the legacy parsing logic survives only inside the migration. The example fixture JSON is converted as well.Test plan
pants teston the changed targets and their direct dependents (26 targets, including component tests) passesalembic/AGENTS.md: representative legacy rows (seeded + user-authored patterns) convert correctly; downgrade → re-upgrade is idempotent./bai: preview/execute succeed with Jinja templates, legacy syntax is rejected client- and server-side with a clear erroruser_utilization_metricreturns real data for GAUGE/RATE/DIFF paths)Resolves BA-7317
🤖 Generated with Claude Code
📚 Documentation preview 📚: https://sorna--13675.org.readthedocs.build/en/13675/
📚 Documentation preview 📚: https://sorna-ko--13675.org.readthedocs.build/ko/13675/