Skip to content

fix: fail Grafana namespaced metric_map targets and cover field-profile ingest - #439

Merged
shmsr merged 3 commits into
mainfrom
fix/field-profile-coverage-and-metric-map-prefix
Sep 8, 2026
Merged

fix: fail Grafana namespaced metric_map targets and cover field-profile ingest#439
shmsr merged 3 commits into
mainfrom
fix/field-profile-coverage-and-metric-map-prefix

Conversation

@shmsr

@shmsr shmsr commented Sep 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Grafana migrate now fails closed when a metric_map target already carries the active Prometheus profile prefix (metrics.*, prometheus.metrics.*, prometheus.*), so operators get a logical-name hint instead of metrics.metrics.* in emitted ES|QL. otel / passthrough are unchanged; Datadog map files stay verbatim.
  • Offline Grafana discovery warnings name the actual --field-profile layout (named Prometheus profiles no longer claim an OTel service.name fallback).
  • Always-on ingest-story tests pin Grafana (node_exporter / cAdvisor / redis / RED) and Datadog (Agent / k8s / Redis+DogStatsD) field-profile emit, plus expanded curated-pack portability coverage.

Test plan

  • pytest tests/core/metric_mapping/test_grafana_metric_map_prefix_lint.py — helper, resolver, CLI exit 1, shipped pack ratchet
  • pytest tests/core/reporting/test_print_report.py — profile-specific offline warning copy
  • pytest tests/test_grafana_field_profile_portability.py tests/test_datadog_field_profile_portability.py
  • pytest tests/test_command_contract_metric_map_examples.py
  • Confirm CI unit + lint jobs on this PR
  • Optional: grafana-migrate --field-profile prometheus_native --metric-map-file with a metrics.foo target and check stderr ERROR: + exit 1

…le ingest

Offline Grafana runs already emit named Prometheus layouts, but the discovery
warning still claimed every non-passthrough profile fell back to OTel
service.name. Metric_map targets that already include the active profile
prefix now fail closed instead of emitting metrics.metrics.*. Always-on
ingest-story tests pin Grafana and Datadog field-profile emit.
@shmsr shmsr self-assigned this Sep 7, 2026
Auto profile resolution ran inside schema discovery's broad exception path,
turning prefix-lint failures into warnings and allowing invalid artifacts.
Propagate only the dedicated lint error while preserving graceful discovery
failures, and make the profile portability assertions non-vacuous.
Comment thread tests/core/metric_mapping/test_grafana_metric_map_prefix_lint.py Fixed
The code-quality review flagged mixed parent and from-import forms. Import the
mock submodule explicitly so TestCase and mock remain available through one
consistent namespace without relying on unittest to load mock implicitly.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: fixed, then approved.

Review contract and paths exercised:

  • Compared merge base 0a5340ad64b43764d74d8ae3a64fe22a8bb23a56 through original head e34578c52ac5c40902b72cd716b7116a9d121367, then re-reviewed fixes d0dcf02a3487ba84bc356661b6a6e30fff045c62 and fab56248f0f79f9244e20877684947ccfb8c2c69.
  • Exercised helper, resolver, and CLI paths for all three prefixed Grafana profiles; root/variant/empty/multiple map entries; explicit OTel/passthrough preservation; auto named-layout detection and OTel fallback; request/JSON error paths; profile-specific offline warning copy; Grafana/Datadog portability; docs and repository conventions.
  • Confirmed no panel/widget type or baseline changes. The only external review comment was evaluated and resolved below.

Findings and fixes:

  • Important — <observability_migration/adapters/source/grafana/schema.py:291-307>: _discover_fields() swallowed the new lint when auto resolved to a named Prometheus layout, turning a required hard failure into a discovery warning and allowing invalid output. In d0dcf02, <observability_migration/adapters/source/grafana/metric_map_lint.py:26-59> now raises a dedicated GrafanaMetricMapPrefixError, and discovery re-raises only that signal while retaining graceful handling for unrelated request/JSON errors. Regression coverage is at <tests/core/metric_mapping/test_grafana_metric_map_prefix_lint.py:189-249> and <tests/core/metric_mapping/test_grafana_metric_map_prefix_lint.py:327-396>.
  • Minor — <tests/test_grafana_field_profile_portability.py:197-210> and <tests/test_datadog_field_profile_portability.py:223-236>: distinct-query tests used different indexes per profile, so they passed even if every migration used OTel. d0dcf02 makes all runs share _INDEX; mutation probes now fail when the profile argument is ignored.
  • Minor — <docs/command-contract.md:597-609>: offline documentation incorrectly included auto, although the CLI requires --es-url. d0dcf02 now states that requirement and exit behavior.
  • Minor — <tests/test_field_profile_portability.py:840-846>: d0dcf02 removed the extra EOF blank line; git diff --check is clean.
  • Minor/code-quality — <tests/core/metric_mapping/test_grafana_metric_map_prefix_lint.py:9-13> mixed import unittest and from unittest import mock. fab5624 uses explicit import unittest.mock and unittest.mock.* throughout. I verified the bot's literal import unittest-only recipe would raise AttributeError because it does not load the mock submodule, so the explicit submodule import preserves behavior while satisfying the single-style requirement. Replied in the original thread with the rationale and gates.

Artifact evidence: an identical mocked auto/native-caps control on original head exited 0 and wrote one native artifact containing metrics.metrics.already_prefixed; fixed head exits 1, writes zero native artifacts, and emits the logical-name guidance. This is a pre-translation CLI error path; valid-input dashboard output is unchanged. Kibana visual verification therefore does not apply—there is no fixed-path dashboard to upload or render.

Gate evidence on the fixes:

  • Full targeted PR set after d0dcf02: 128 passed, 58 skipped; import-style target after fab5624: 21 passed.
  • Final make test: 6,289 passed, 61 skipped, 473 subtests passed.
  • Final make lint: exit 0; source headers 505/505 and Ruff clean.
  • Final make typecheck: exit 0; mypy found 0 issues in 10 files.
  • Final manual configured pre-commit run: 5/5 hooks passed (Gitleaks, headers, Ruff, quick pytest, local-path check).
  • Final pushed-head CI: 14/14 checks passed, including Python 3.11/3.12/3.13 pytest, E2E, Ruff, mypy, Gitleaks, packaging/CLI/clean-install smoke, CodeQL, CLA, and dependency/SBOM.

Concerns probed and found unfounded: auto→OTel still allows namespaced targets; unrelated request and JSON failures still degrade gracefully; explicit OTel/passthrough behavior and Datadog verbatim mapping remain unchanged. Original-head/base checks had no pre-existing failures. Remaining optional Minors: none.

@shmsr
shmsr merged commit d80e8ec into main Sep 8, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants