fix: correct Datadog migration defects exposed by a live cluster - #454
Conversation
…luster
A customer demo hit "first argument of [http.response.status_code == \"500\"]
is [numeric] so second argument must also be [numeric] but was [keyword]".
Chasing it end-to-end — migrate, seed, live_validate per field profile against
a real Elasticsearch — turned up a family of defects that offline gates could
not see, because none of them had ever been executed against a cluster holding
the shape they assumed.
Translation
- Tag-filter literals are rendered at the target field's type. Caps say
numeric -> bare number; caps say string -> quoted; no caps + digits ->
TO_STRING(...), valid against either mapping. Covers ==, !=, a|b, IN (...),
monitor scopes and metric_map attribute_filter.
- ES|QL LIKE wildcards are * and ?, not SQL's % and _. The metric tag renderer
translated them, so `{host:web-*}` matched nothing and `{!host:canary*}`
excluded nothing — valid ES|QL, wrong rows, no error.
- Identifier quoting is per dotted segment and idempotent. Whole-string
quoting produced nested backticks (`prometheus.labels.`client-id``) on three
profiles, and reserved words were unquoted (SUM(system.network.in.bytes)).
The word list is derived from probing every ES|QL keyword against a cluster.
- Histogram `le` on a numeric target compares numerically; RLIKE casts the
field; +Inf compares via TO_STRING. Keyword `le` keeps its "1"/"1.0"
alternation.
- Datadog log syntax `status:(error OR warn)` tokenizes as a group. It was
becoming `status == "(error" OR message LIKE "*warn*"`, which also destroyed
the surrounding AND/OR structure.
- log_stream display columns resolve through the profile instead of hardcoding
ECS names, which only worked because six of seven profiles map to them.
Seeding and mapping
- Generated index templates set `subobjects: false`. Elasticsearch rejects
`redis.keys` beside `redis.keys.evicted` only under the default object
mapping; with subobjects off the pair is accepted and composes with
index.mode: time_series. Elastic's own metrics-otel@template reaches the same
place with passthrough objects. The contract no longer drops such metrics.
- The document-generation window is clamped to the 7d TSDS look_back_time
ceiling it already applied to the index setting; 53,256 of 125,532 documents
were being generated outside the writable window and rejected.
- A telemetry-contract field name keeps its backtick-quoted segment
(`...lastContact.`95percentile``), instead of splitting into a trailing-dot
name plus an orphan. That malformed name made the whole index template
invalid, so seed-sample-data aborted the stream.
- Stream names can no longer keep a wildcard (`metrics-*.prometheus-*`), which
made prometheus_native unseedable.
Reporting
- assess_field_usage's capability-is-None branch is reachable: it sat behind
`if metric_cap:`, so a run whose caps proved a metric absent still scored the
panel a clean OK. Scope filters are assessed too, not just metrics and
group-bys.
- DATA READINESS is shared by both source reporters. It lived inline in
Grafana's print_report, so Datadog runs printed 100% green while their own
readiness contract recorded status: missing.
- seed-sample-data surfaces error_samples (captured but never read) and warns
when backfill is truncated or a field cannot be mapped.
- An `object` container is no longer reported as a confirmed field.
Gates
- New verifier.filter_semantics_gate asserts a WHERE clause selects exactly the
intended rows, per profile and per target type. live_validate classifies a
valid-but-wrongly-filtering query as ok, and the render audit calls the empty
panel a warn, so nothing caught the wildcard bug.
- run_cross_profile_corpus.py takes --source; it was grafana-only. It now fails
closed when a profile has no leakage rules rather than passing vacuously.
Verified on Elasticsearch 9.6.0: all 7 Datadog and 5 Grafana profiles migrate,
seed and validate with data_gap=0 and REAL_BUGS=0. passthrough moves from
255/261 to 261/261 runnable queries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3fc0608 to
0e43149
Compare
Found by uploading the migrated Datadog corpus into Kibana and looking at it
in a browser: "System Overview - Sample" opened with 7 of its 9 query panels
showing "No results found", and the render audit agreed. Every layer below the
browser said the migration was fine -- the ES|QL was valid, the uploaded
panels matched the IR byte for byte, and the Lens accessors mapped correctly
onto the query's columns.
The translation was fine. The seeded data was not.
A Datadog template variable with a default (`{'name': 'env', 'default':
'prod'}`) migrates faithfully into an options-list control with
`selected_options: ['prod']`, and Kibana applies that selection on first open.
But the telemetry contract is built from query *text*, and a control's
pre-selection appears in no query, so the seeder invented `production` /
`staging` / `development` instead. `env == "prod"` then matched 0 of 34,946
documents and the whole dashboard came up blank.
`_require_control_fields` exists for this exact symptom -- its docstring says
"the seeded documents then match no control selection" -- but it only asserts
the control *field* reached the contract. `env` did, so the guard passed while
the dashboard was still empty. The field is not enough; the value has to be
seeded too.
Three changes, each verified in the browser:
- `control_selection_values` reads `selected_options` / `available_options`
from the IR controls, and `merge_control_selection_values` folds them into
the streams' `required_values`. The pre-selected value is listed first so a
cardinality cap truncates the merely-offered options, never the applied one.
- Those values are merged into every stream that knows the field, not only the
stream the control names. The `env` control binds `metrics-*`, but Kibana
applies it to the `logs-*` panels too; binding by data_view alone left the
logs stream with invented values and kept both log panels blank.
- `_metric_families` gives every metric the stream's control fields. It unions
dimensions per requirement, and a dashboard control is not a query
dimension: when some other panel's query happened to mention `env`, the
family holding `system.cpu.user` excluded it, so 0 documents carried both
and the charts drew axes with `(null)` values.
Render audit over the uploaded corpus, local Kibana 9.6.0: 15 dashboards,
772 panels, 770 rendered, 0 render_error. "System Overview - Sample" goes from
14 empty panels to 22/22. The 2 remaining are one `change` widget comparing
the last 7 days against the 7 before that -- a time-series index accepts at
most 7d of backfill, which `seed-sample-data` already warns about.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stefans-elastic
left a comment
There was a problem hiding this comment.
Review findings
The overall direction is strong: identifier quoting is centralized and idempotent, typed filter handling is substantially improved, readiness reporting is shared, and the focused regression coverage is extensive. I found the following issues that should be addressed before merge.
Critical
-
The new filter-semantics verifier can delete unrelated Elasticsearch indices.
parity-rig/verifier/filter_semantics_gate.py:142deletes predictable names such asfilter-semantics-otel-longon any endpoint supplied through--es-url, then line 216 deletes them again. Concurrent runs can also erase one another's indices. The delete/create/bulk responses are ignored and cleanup is not infinally, so failures can additionally leave stale state or query stale data.Please use collision-resistant run-specific names, verify API responses and resource ownership, and clean up only resources owned by this invocation in
finally.
Important
-
The “exact selected rows” gate only verifies cardinality.
filter_semantics_gate.py:201-214ends every query withSTATS n = COUNT(*)and compares the result withlen(expected). A broken predicate that selects a different row with the same cardinality passes, despite the module and docs promising exact document-set validation. Seed a stable identity/value column and compare the normalized selected set withexpected; add a same-count/wrong-row regression test. -
Grouped Datadog log values bypass safe value parsing and typed rendering.
At
log_parser.py:597-600, grouped members are split as raw strings and passed directly to_render_attr_predicate. Reproductions against this PR head include:status:("error" OR "warn")emitting comparisons against values that literally contain quote characters.- Numeric
@http.status_code:5*emittinghttp.status_code LIKE "5*", which ES|QL rejects for a numeric field. @http.status_code:(500 OR error)emitting a barehttp.status_code == error, treatingerroras a column.- Quoted numeric group members remaining quoted against numeric fields.
Please parse grouped members as proper quoted/unquoted values and route equality and wildcard members through a shared typed operand renderer. Add numeric, string, unknown, quoted, wildcard, and mixed-member cases.
-
Explicit
--data-hoursstill bypasses the seven-day TSDS ceiling.telemetry_data.py:846-850clamps only contract-derived lookback.data_hours=240still generates timestamps 240 hours old even though the generated TSDS template accepts at most 168 hours, recreating the rejected-document failure this PR intends to fix. Clamp the effective dense window, includedata_hoursin truncation reporting, and testdata_hours > 168. -
Wildcard replacement can generate a stream that does not match its source pattern.
telemetry_data.py:47replaces a run containing?with the multi-character stringgeneric. For example,metrics-?-*becomesmetrics-generic-default, which does not match the original pattern because?matches exactly one character. Replace each?with one safe character and each*with a safe run, and test the invariant that the generated concrete name matches the input pattern. -
Datadog cross-profile coverage omits
elastic_agent.scripts/run_cross_profile_corpus.py:48-54includes both aliases of the same Prometheus layout but excludes the unique built-inelastic_agentprofile. The new fail-closed rule check therefore never sees thatelastic_agentlacks leakage rules. Add those rules and profile coverage; preferably derive unique canonical profiles from the adapter registry so new profiles cannot silently escape the gate. -
Mixed literal/template scope values escape readiness assessment while still emitting a field reference.
translate.py:1900drops every value containing a template from_scope_filter_tag_keys. A value such asservice:prod-$svcstill emitsservice LIKE "prod-*", but readiness records no dependency onservice; if live caps prove it absent, the panel receives no DATA READINESS reason and later fails with an unknown-column error. Skip readiness only when no clause is emitted or when the field key itself is dynamic, and add mixed literal/template coverage.
The current Ruff and unit-test failures appear inherited from main and occur in untouched Grafana code, so I have not counted them as PR findings. Packaging, E2E, type checking, security, and dependency checks are green.
Seeding the 47 pinned DataDog/integrations-core dashboards failed outright:
Failed to create index template telemetry-data-metrics-generic-default:
composable template [...] template after composition is invalid
That message names no cause. The leaf of Elasticsearch's caused_by chain does:
``Limit of total fields [1000] has been exceeded``. The metrics stream for that
corpus declares 1043 mapping properties against a default cap of 1000, so every
panel went unseeded -- and ``verifier.live_validate`` then reported
``ok=34 data_gap=867`` on 901 queries, which reads as "telemetry not ready"
rather than "the seeder never ran".
The in-repo corpus is small enough (306 properties) to stay under the cap,
which is why this only appears at a realistic size.
- ``plan_index_template`` sets ``index.mapping.total_fields.limit`` from the
property count it is about to declare, with headroom for the dimensions
added alongside. It knows the number; it should not leave it to a default.
- ``_raise_on_error`` flattens the whole ``caused_by`` chain, so the actionable
leaf survives instead of being replaced by its outermost wrapper.
After the fix the same corpus seeds 2,593,628 documents with 0 errors, and
live validation over it is no longer vacuous: 901 of 901 queries ok,
data_gap=0, REAL_BUGS=0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``extract_monitors_from_files`` accepted a bare monitor object only when it
carried a top-level ``id`` **and** ``type``. Datadog does not publish monitors
that way. An integration asset -- and the payload the Datadog UI exports --
nests the monitor under ``definition``:
{"version": 2, "title": ..., "tags": [...], "description": ...,
"definition": {"name": ..., "type": "query alert", "query": ..., ...}}
Sampling 60 monitor assets from DataDog/integrations-core: all 60 are that
shape, so all 60 were skipped and the run reported "No monitors found". An
operator pointing the tool at their own exported monitors would have seen the
same. 21 of the 60 also carry no ``id`` at all, so unwrapping alone would still
have dropped a third of them -- a monitor without a source id is still a
monitor, so identification now keys on ``type``.
With the fix those 60 extract cleanly (44 `query alert`, 16 `log alert`) and
translate offline as 20 automated / 10 draft / 30 manual. The existing shapes
-- bare object, top-level array, `{"monitors": [...]}` -- are unchanged, and a
payload that is not a monitor is still skipped with a warning that now names
all three accepted shapes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running 60 real DataDog/integrations-core monitors, the alerts run printed:
Total: 60
By tier: {'automated': 20, 'manual_required': 30, 'draft_requires_review': 10}
Thirty monitors need hand-rebuilding in Kibana and the run named none of them.
The dashboard path prints per-panel reasons, a not-feasible list and a DATA
READINESS section; the alerts path printed a tier histogram and stopped.
The artifact was no better for most of them: 21 of the 30 carry no `warnings`
at all, only generic `losses` about threshold-window semantics that translated
monitors carry too, so they do not explain the tiering. The largest single
cause -- Datadog anomaly detection, which has no ES|QL equivalent -- was
recorded nowhere.
`derive_manual_reason` prefers whatever the translator actually said and falls
back to naming the construct, and the pipeline groups the reasons under the
tier summary the same way the dashboard reporter does:
MONITORS NEEDING MANUAL WORK (30):
14 no ES|QL translation was produced for this monitor query; rebuild ...
7 Datadog anomaly detection has no Kibana/ES|QL equivalent; rebuild ...
6 Datadog formula monitor requires manual review; exact support ...
3 Datadog metric monitor query parse degraded; manual review required
The 14 generic ones are honestly generic: offline their real blocker is field
readiness, which cannot be known without --es-url. Run with a target they
report the specific missing field instead ("Target log measure field
`pspReference` is missing from log field capabilities").
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the live Datadog API path surfaced the weak spot in the previous
commit: a `service check` monitor reported the generic "no ES|QL translation
was produced", which tells the operator nothing about what to build instead.
`core.mapping.MANUAL_ONLY_KINDS` already enumerates the monitor kinds that can
only ever be manual, and each has a different cause: a composite monitor
references other monitors by id, a service check alerts on OK/WARN/CRITICAL
rather than a query, an SLO alert wants an Elastic SLO burn-rate rule,
Synthetics wants Elastic Synthetics, and forecast/outliers/Watchdog are
algorithmic detections with no ES|QL equivalent. Collapsing all of that into
one sentence wastes the only chance the run has to say what to do.
Every kind in that set now has its own reason, and a test asserts the mapping
covers the set so the two cannot drift -- an unknown future kind still falls
back rather than crashing.
Live Datadog API run, before and after:
1 no ES|QL translation was produced for this monitor query; rebuild ...
1 service check monitors alert on check status (OK/WARN/CRITICAL)
rather than a metric query; there is no ES|QL equivalent
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the live Datadog API path with a mistyped --dashboard-ids printed the
files-mode sentence:
ERROR: no Datadog dashboards found under .. Point --input-dir at a
directory of Datadog dashboard JSON exports (each with a top-level
'widgets' key).
Two things wrong for an API run: it interpolates an unset --input-dir as "..",
and it tells the operator to fix a flag they never passed. The actionable
advice there is the id, the selectors, or the credentials.
ERROR: no Datadog dashboards matched --dashboard-ids does-not-exist.
Check the ids against the dashboard list in Datadog (the id is the last
path segment of the dashboard URL).
ERROR: no Datadog dashboards returned by the Datadog API. Check
DD_API_KEY / DD_APP_KEY / DD_SITE reach the intended org, and that any
--select-* filters are not excluding everything.
The files-mode branch that raises FileNotFoundError keeps the original
sentence, which is correct there.
Verified against a real Datadog org for all three cases, plus the unchanged
--select-* message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running `--preflight` against the live Datadog API printed both verdicts for the same dashboard: `Preflight: issues Block: 0 Warn: 52 Info: 0` while the run was in progress, then `Preflight: pass Block: 0 Warn: 52 Info: 0` in the summary. Both lines describe one `PreflightResult` -- `cli.py` copies `preflight_result.passed` straight onto `dashboard_result.preflight_passed` -- so an operator is told the dashboard both did and did not pass, with no way to tell which line to act on. The two sites had drifted onto different predicates. The report and the manifest both key off `passed`, which `PreflightResult.add` clears only for a **block**-level issue; the in-run line additionally demanded `not preflight.issues`, so any warning read as a failure. That is the common case, not an edge one: warnings are how preflight reports fields absent from the target, and the live run raised 52 of them on one dashboard. The same expression also had an unreachable arm -- `passed` is false only when a blocking issue was added, so its `else "pass"` could never be taken. `passed` is the predicate the manifest publishes, so it is the one the printed word now follows, via a single `preflight_status_label` the two call sites share rather than each spelling out. Warnings stay visible in the `Warn` count and the listed issues beside the verdict; they no longer masquerade as a failed preflight. Verified against the live Datadog API: the 52-warning dashboard now reads `pass` in both places and `"passed": true` in `migration_report.json`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Migrating a real Datadog account with `--upload` reported three of its six dashboards as `UPLOAD FAILED`/`UPLOAD ERROR: <title>: empty`, and wrote `upload.status: "fail"` for each into `migration_report.json`. All three have zero widgets in Datadog. Nothing was wrong: accounts accumulate scratch dashboards, and the run was calling them breakages -- on the console, where they bury the failures worth chasing, and in the manifest, where they fail any CI gate keyed on it. The upload path refuses a payload with no leaf panels, which is right: a dashboard of empty collapsible sections is what silently dropped panels look like from the outside, and `_payload_has_leaf_panels` exists to catch exactly that. But two unrelated situations reach it, and it answered both the same way. So the check now separates them instead of being relaxed. A payload that carries something -- sections, controls, or a non-zero mapped/unmapped count -- and still has no leaves lost its panels on the way, and keeps failing as `empty`. A payload that carries nothing whatsoever is a `source_empty`: no dashboard is created, and the reason travels with the status. The discriminator reads the payload's own shape rather than a caller's claim, so a source that drops panels without counting them cannot reach the benign answer by staying quiet -- dropping panels leaves behind either the items that held them or a count. `source_empty` is still not `uploaded_ok`, because nothing reached Kibana. It is no longer a failure: `datadog-migrate` prints `UPLOAD SKIPPED` and records `runtime_summary.upload.status: "skipped"`, kept apart from `upload_error` so the skip cannot be mistaken for one. Grafana shares the upload path and now gets the explanation in its output line; wiring its own reporting to skip rather than fail is left for a follow-up. Verified against the live Datadog API: the three empty dashboards report `UPLOAD SKIPPED` and `"status": "skipped"`, the three with widgets still upload and pass, and a payload of empty sections still fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A migrated Datadog dashboard carried two panels whose ES|QL Elasticsearch
refuses outright:
| EVAL series_group = CONCAT(..., COALESCE(TO_STRING(series_group), ""))
| EVAL series_group = CONCAT(COALESCE(TO_STRING(deployment.environment), ""), ...)
verification_exception: Unknown column [series_group]
An EVAL cannot read the column it is defining, so both panels failed to render
-- the same hard-error class as the numeric tag filter that started this work,
reached by a different route.
Lens XY and heatmap each take one categorical field, so `_composite_y_column`
splices a synthetic CONCAT column in when a query groups by two or more tags.
The splice writes back through `result.esql_query`, and the panel builders
re-derive their dimensions from that query. Re-entering a builder with the same
`TranslationResult` therefore sees the synthetic column sitting among the
grouping dimensions and concatenates it into its own definition, emitting a
second EVAL ahead of the first. Nothing downstream can recover from it: the
query is rejected before it runs.
So the splice is idempotent now. A query that already defines the column is
returned untouched, and the column is dropped from the incoming dimensions --
it is an output of this stage, never an input to it. Both call sites (the XY
`series_group` and the heatmap `y_group`) are covered, and the Grafana side
already guards its own composite legend the same way (PR #369).
This fixes the corruption rather than the trigger. Whatever re-enters the
builder -- and it took a specific combination of run flags and target state to
provoke -- a stage that rewrites its own input should survive being run twice.
Verified: re-running the splice over the real affected query now returns it
unchanged, and that query executes against Elasticsearch (1573 documents,
7517 rows) where the emitted one raised `Unknown column`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The render audit is the gate that decides whether a migrated panel actually renders, and it passed one that could not. A Datadog dashboard emitted `EVAL series_group = CONCAT(..., TO_STRING(series_group))`; Elasticsearch rejected it with `Unknown column [series_group]`; the audit confirmed `series_group` is absent from the target's field caps and filed it as `field_gap` — warn, not fail. The run was green on a panel showing an error. The evidence rule behind that downgrade is sound for a column the panel expects to *find* in the target: if it is genuinely missing, the panel is waiting on data, not on a fix. It does not hold for a column the translator *creates*. A synthetic `EVAL`/`STATS`/`RENAME` output is supposed to be absent from the index — that is what makes it synthetic — so its absence is not evidence of anything, and an error naming it can only be a construction bug. So the classifier now takes the columns each panel's query defines for itself and keeps the verdict at `render_error` when the unknown column is one of them, saying why in `detail`. Everything else is untouched: a genuinely absent target column is still a `field_gap`, and a caller that supplies no query columns gets exactly the previous behaviour. Checked against the artifact that exposed it: the same error text and field caps classify `field_gap` without the change and `render_error` with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o an hour
A real dashboard's "compared to last month" panels came through with a
one-hour window and the warning "change widget live span was unavailable;
defaulted to 1 hour", while the sibling "compared to last week" panels kept
their week. The source is explicit in both cases -- `"time": {"live_span":
"1mo"}` -- so nothing was unavailable.
The span parser accepted only the single-letter units `s/m/h/d/w`. Datadog's
live-span vocabulary also runs to `1mo`/`3mo`/`6mo` and `1y`, so those widgets
matched nothing and fell back. `m` means *minutes* here and `mo` means month,
so the unit alternation tries `mo` first; previously `fullmatch` rejected the
whole string rather than misreading it, which is why this surfaced as a silent
default rather than a one-minute window.
Calendar months and years vary in length and an ES|QL duration is fixed, so
they are taken as 30 and 365 days -- the same approximation Datadog's own
relative ranges make, and the closest thing expressible.
The fallback warning was doing its job, so this was visible rather than silent;
it just should not have been reachable for a span Datadog considers valid.
Verified on the dashboard that exposed it: all four change panels now translate
without the fallback warning.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two review findings on the new filter-semantics gate, both mine. **It could delete an unrelated index.** `_seed` opened with an unconditional `DELETE /filter-semantics-<profile>-<type>` — a fully predictable name, on whatever endpoint `--es-url` names. Pointed at a shared cluster it would erase someone else's index of that name, and two concurrent runs would erase each other's data mid-flight. Every index now carries a per-invocation token, so there is nothing of ours to clear first and nothing of anyone else's we can collide with, and cleanup runs in a `finally` over the list this invocation actually created rather than trailing the loop where an exception skips it. Seed responses were also discarded. A failed create or a bulk that answered 200 while rejecting every document left the gate asserting against an empty index — and reporting a pass. Both are checked now and surface as a finding naming the reason. **It only verified cardinality.** Every query ended `STATS n = COUNT(*)` and the result was compared with `len(expected)`, so a predicate selecting a different document of the same cardinality passed — which is precisely the bug class this gate exists to catch, and the module docstring already claimed it compared the document set. It now keeps and sorts the seeded value column, which is each row's identity, and compares the selected set. Order is not part of the contract, so both sides are sorted; a `long` mapping returns ints and the table is written in strings, so both are compared as strings. The offline driver tests cover the lifecycle directly: nothing is deleted that was not created, two runs share no index name, cleanup survives a raising query, and a same-count/wrong-row selection is a finding. Verified against Elasticsearch 9.6.0: 140/140 shapes select the expected rows, and no `filter-semantics-*` index survives the run.
… text
`field:(a OR b)` was split on `OR` with a regex and each piece handed to the
predicate renderer as a raw substring, so both quoting and target typing were
skipped. Verified against Elasticsearch 9.6.0, every one of these was emitted
and rejected:
status:("error" OR "warn") -> status == "\"error\""
(matches nothing; the value
contains quote characters)
@http.status_code:5* -> http.status_code LIKE "5*"
argument of [...] must be [string]
@http.status_code:(500 OR error) -> http.status_code == error
Unknown column [error]
@http.status_code:("500" OR "503") -> http.status_code == "500"
is [numeric] so second argument
must also be [numeric]
The last is the error this whole branch started from, reached through the log
path rather than the tag path.
The tag path already decides this correctly, and `_tag_comparison_mode`'s
docstring even claimed to mirror the log path — but nothing shared the
implementation, so they had drifted. The three helpers (`comparison_mode`,
`comparison_operands`, `pattern_field`) move to `emit/esql_utils`, which both
paths already import; `translate.py` keeps its private names as aliases, so
the 307-case tag matrix is unchanged. `log_parser` cannot import `translate`
(the dependency runs the other way), which is why a neutral home was needed
rather than a direct import.
On top of that, group members are split with a quote-aware scanner rather than
`re.split`, so a quoted member keeps its spaces, is not cut on an `OR` inside
it, and loses the quote characters that are syntax. One comparison family is
chosen for the whole group from all its members together, the same rule the
tag path applies to an OR chain, so a mixed group keeps a single left-hand
side instead of pairing a numeric compare with a cast one.
All four now execute: `TO_STRING(http.status_code) LIKE "5*"` returns its row,
the quoted numerics compare as numbers, and the non-numeric member compares
through `TO_STRING` — valid, and correctly selecting nothing.
The contract-derived lookback was clamped to what a time-series index accepts, but `data_hours` went straight through to the generator. `--data-hours 240` produced 72 documents up to ten days old against a template that accepts seven, so Elasticsearch rejected them outright — the same failure the clamp was added to fix, reached through the flag instead of the contract. Both the dense window and the lookback it raises are now bounded by `MAX_TSDS_LOOKBACK_SECONDS`, and the truncation warning takes the wider of the contract's request and `--data-hours`, so an operator who asks for ten days is told they are getting seven rather than finding out from a short series.
`concrete_stream_name` substituted the multi-character string `generic` for a run of `*` or `?` alike. `?` matches exactly one character, so `metrics-?-*` resolved to `metrics-generic-default` — a name its own source pattern cannot match. The seeded data was then invisible to every panel reading that pattern, which looks exactly like telemetry that was never seeded. Each `*` run still collapses to one safe token; each `?` becomes one safe character. The invariant is now a test: for every pattern shape, the generated stream must be matched by the pattern it came from, and must contain no wildcard of its own.
The Datadog profile list was hand-kept, and carried both spellings of the same Prometheus layout (`prometheus` and `prometheus_metrics`) while omitting `elastic_agent` and `default` entirely. The redundancy is what hid the omission. Because the new fail-closed check only inspects the profiles that are listed, it never saw that the two missing ones had no leakage rules — the guard against a vacuous pass was itself passing vacuously. Both lists now come from the adapters: `BUILTIN_PROFILES` for Datadog and `_GRAFANA_FIELD_PROFILES` for Grafana, minus `auto`, which resolves to a concrete profile at migrate time. A new profile therefore joins the gate by existing, or trips `profiles_without_rules` on its first run. `default` and `elastic_agent` emit the same native/ECS namespaces as `otel` (`host.name`, `deployment.environment`, no metric prefix); `elastic_agent` differs only in its 18 metric renames, which these rules do not police. So they alias to the `otel` rule set rather than getting a duplicate copy, and the drift guard is a test asserting every registry profile is both covered and ruled.
Readiness skipped every scope value containing a template variable. That is right for a pure template — `service:$svc` emits no clause, so there is no field to judge — but wrong for a mixed one: `service:prod-$svc` emits `service LIKE "prod-*"`, as hard a dependency on `service` as a plain literal. If live caps proved that field absent the panel got no DATA READINESS reason and failed at query time with `Unknown column` instead. What decides assessment is now whether a clause is emitted, and the predicate that answers it is the emitter itself, so the two cannot drift apart again. A dynamic *key* is still skipped, and no longer records the literal `$k` as a dependency — naming a field that cannot exist is worse than naming none.
| code, body = request( | ||
| es_url, | ||
| "POST", | ||
| "/_query", | ||
| { | ||
| "query": ( | ||
| f"FROM {index} | WHERE {predicate} " | ||
| f"| KEEP {keep} | SORT {keep} ASC" | ||
| ) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Leaving this. FailingCreate subclasses FakeES and defines __call__, and the test passes an instance as request. The call is valid. A callable(request) guard would not change this finding.
| finally: | ||
| # Only what this invocation made, and even if a shape blew up. | ||
| for index in created: | ||
| request(es_url, "DELETE", f"/{index}") |
There was a problem hiding this comment.
Same as the note above: FailingBulk implements __call__, so the instance passed as request is callable. Not changing the gate for this.
Review findings addressed — all 7, one commit each@stefans-elastic thanks, these were good catches. Every one reproduced before
Broader than reported. #6 also omitted #3 was the most consequential. VerificationCIThe three red checks are inherited from One note for whoever picks #455 up: the Ready for another look. 🤖 Generated with Claude Code |
stefans-elastic
left a comment
There was a problem hiding this comment.
[P2] Scope the seven-day clamp to TSDS metric streams — observability_migration/core/telemetry_data.py:889
_document_timestamps is shared by every stream and timestamps are calculated before stream types are known. Consequently, --data-hours 240 now truncates logs and traces to 168 hours even though their templates do not use index.mode: time_series. Compute timestamps per stream or apply the ceiling only to metric TSDS streams.
[P2] Do not classify readable flattened roots as missing — observability_migration/core/verification/field_capabilities.py:219
Both readiness builders now treat flattened capabilities as absent. On Elasticsearch 9.6, ES|QL can read a flattened root directly; only accessing its subfields requires FIELD_EXTRACT. This produces false status: missing results for valid root-field queries. Separate non-queryable object path nodes from readable container field types. Reference: https://www.elastic.co/docs/reference/query-languages/esql/esql-flattened-fields
[P2] Establish the Datadog baseline before comparing profiles — scripts/run_cross_profile_corpus.py:63
The new Datadog profile list is sorted, placing prometheus_native last. The loop only compares profiles after baseline_count is assigned, so none of the six preceding Datadog profiles undergoes the advertised feasibility-parity check. Run the native profile first or collect all counts before comparing.
Overall assessment: the PR-targeted tests pass, as do Ruff and git diff --check, but these three issues should be addressed. The full non-e2e suite stops on two test_grafana_issue_440_vector_matching failures; both reproduce unchanged on the PR base, so they are pre-existing rather than PR regressions. Live Elasticsearch verification was not rerun during this review.
…nels.py The function was imported from promql.py at line 94 and re-defined verbatim at line 1556, producing Ruff F811. Inherited from elastic#444, tracked in elastic#455. Confirmed identical by fuzzing (PR elastic#454 review). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The 7-day lookback cap is an Elasticsearch time-series limit. Applying it before stream type was known shortened logs and traces, flattened roots were marked missing even though ES|QL can read them, and feasibility parity never checked Datadog profiles that sort before prometheus_native.
|
Second review, addressed in b12eda4. TSDS ceiling. Flattened roots. Feasibility parity. Counts are collected for every profile, then Code-quality: removed unused |
Fixes #459
Why
A user demo hit this on a migrated Datadog dashboard:
Running that to ground —
migrate → seed-sample-data → verifier.live_validate, onceper field profile, against a real cluster — turned up a family of defects sharing one
cause: these paths had never been executed against a cluster holding the shape they
assumed. Four committed assertions had pinned the buggy behaviour, so the suite was
green throughout.
What's fixed
Each bullet is one commit; the rationale and evidence are in the commit messages.
Translation
!=,a|b,IN (…), monitor scopes,metric_mapfiltersLIKEused SQL%/_; ES|QL wants*/?.{host:web-*}matched nothing and{!host:canary*}excluded nothing — valid ES|QL, wrong rows, no errorsystem.network.in.bytes) were hard parse errors on 4 profilesstatus:(error OR warn)tokenized intostatus == "(error", destroying the surrounding boolean structurelog_streamcolumns were hardcoded to ECS, so every log panel failed underpassthroughEVAL series_group = CONCAT(…, TO_STRING(series_group))→Unknown column); the splice is idempotent now1mo/1ylive spans were unparsed, silently collapsing "compared to last month" panels to a 1-hour windowSeeding / mapping
subobjects: falseon generated templates: ES rejectsredis.keysbesideredis.keys.evictedonly under the default mapping.passthroughgoes 255/261 → 261/261Alerts
{"definition": {…}}) was unreadable — 60 of 60 real monitors skipped as "No monitors found". Identification keys ontypenow, since a third carry noidmanual_requiredwith no reason; everyMANUAL_ONLY_KINDnow names the construct to build, with a drift guardReporting
assess_field_usage'scapability is Nonebranch was unreachable, so a run whose caps proved a metric absent still scored the panelOK— while its own readiness contract saidmissingDATA READINESSwas Grafana-only; Datadog has a separate reporter. Now sharedPreflightResultprintedPreflight: issuesin-run andPreflight: passin the report. Only a blocking issue fails a preflight — which the manifest already published — so both derive from thatUPLOAD FAILED; 3 of 6 dashboards on a real account have zero widgets. Nowskipped, without relaxing the check that catches silently dropped panelsGates
verifier.filter_semantics_gate— asserts aWHEREselects exactly the intended rows. Nothing existing caught the wildcard bug:live_validatecalls a wrongly-filtering queryok, and the render audit calls the empty panel a warnrun_cross_profile_corpus.pytakes--sourceand fails closed on a profile with no leakage rules, instead of passing vacuouslyVerified
Elasticsearch + Kibana 9.6.0-SNAPSHOT, clean cluster before each run.
migrate→seed→live_validateok=261/261and239/240,data_gap=0 REAL_BUGS=0integrations-coredashboards (1,291 widgets)live_validate901/901; render audit 1,280/1,282, 0render_errorIN (…), boolean scopes,p50–p99, globs,powerpackrender_errorok=27/27; render audit 84 → 88 rendered, 0render_error~600 new test cases across 24 files. The filter matrix, the new gate and the
render-audit reclassification were each mutation-tested — reverting the fix makes them
fail — so they aren't vacuous.
Elasticsearch behaviour was established by probe, not docs:
subobjects: falseisaccepted and composes with
index.mode: time_series("auto"is rejected on 9.6),index.look_back_timecaps at 7d, and 19 of 77 probed ES|QL keywords are rejected asa bare dotted segment.
Not verified here: alert-rule creation. All 30 fail HTTP 500 — but a hand-written
minimal
.es-queryrule fails identically on this stack, so it's the environment(Kibana alerting needs security enabled; the local stack runs with it off), not the
payload.
Follow-ups
comparereplays migrate-time verdicts instead of re-executing, so a staleERRORcan't be clearedlive_validatefilesCannot mix time-series aggregate …asother, so--fail-on-bugmisses itlenumeric label matchers it could now translate — corpus-wide blast radiusper_second(…)) are already handled via the formula path (parse_legacy_query). The residue is capability —top(), timeshift,autosmooth— not parsing, so there's no parser work worth doingSplitting
Natural cut if you'd prefer smaller PRs: (a) filter typing + wildcards + quoting,
(b) telemetry contract / seeding /
subobjects, (c) readiness reporting, (d) gates,(e) the live-API defects. (b) must precede (c)'s tests —
subobjectschanges whatcounts as unmappable.