Skip to content

feat(exec,query): add DQL execution guardrails + dqlbench regression harness#178

Open
danaharrison wants to merge 2 commits into
dynatrace-oss:mainfrom
danaharrison:feat/dql-cost-guardrails
Open

feat(exec,query): add DQL execution guardrails + dqlbench regression harness#178
danaharrison wants to merge 2 commits into
dynatrace-oss:mainfrom
danaharrison:feat/dql-cost-guardrails

Conversation

@danaharrison

Copy link
Copy Markdown

Summary

Adds DQL execution guardrails to pkg/exec/dql.go (500 GiB scan ceiling auto-applied by default, --no-guardrails opt-out on dtctl query) and a test/dqlbench/ regression harness for ongoing before/after cost measurement.

Based on #177 (linter + CLI wiring) — the harness imports pkg/dqlcost to verify that linted queries actually produce cleaner findings after the rewriter runs. Please merge #176#177 → this PR in order.

Refs: #175

Why

Two complementary gaps left after #176 and #177:

  1. No client-side cost ceiling. DQLExecuteOptions.DefaultScanLimitGbytes exists as an opt-in knob but is unset by default, so a runaway query (bad filter, missing from:, unexpectedly wide bucket) has no client-side brake. When the query is being built by an LLM from a user prompt, this is exactly the failure mode that hits production dashboards.
  2. No ongoing regression coverage. Without a fixture-based harness, severity calibrations and rewriter defaults drift relative to real tenant cost characteristics. (The measurement that saved us from shipping an incorrect COST005 rewrite in feat(dqlcost): DQL cost-anti-pattern linter + rewriter + CLI wiring #177 is a good example.)

What changed

pkg/exec/dql.goGuardrailDefaults

type GuardrailDefaults struct {
    ScanLimitGbytes  float64 // default 500 GiB ceiling on scanned data
    MaxResultRecords int64   // 0 = defer to server default
}

var DefaultGuardrails = GuardrailDefaults{ScanLimitGbytes: 500}
func SetDefaultGuardrails(g GuardrailDefaults) { ... }
func applyGuardrailDefaults(opts *DQLExecuteOptions) { ... }
  • Runs at the top of ExecuteQueryWithOptions.
  • Fills in DefaultScanLimitGbytes only when the caller passed 0 (preserves explicit caller values).
  • Honors opts.DisableGuardrails — opt-out for power users.
  • MaxResultRecords intentionally left at 0 so the server's own default continues to apply (matches long-standing test expectations; no behavior change for integrations that rely on server defaults).

cmd/query.go--no-guardrails

  • New boolean flag on dtctl query.
  • Help text on --default-scan-limit-gbytes updated to mention the 500 GiB auto-applied default.
  • No other flags changed; existing behavior preserved.

test/dqlbench/ — regression harness

  • Fixture YAML schema — one file per prompt/query with expect_rules, after_rewrite_no_rules, must_contain, must_not_contain, budget.max_scanned_bytes.
  • Snapshot mode (make test-dqlbench-unit, no tenant required) — loads every fixture, runs dqlcost.Lint on the raw query, asserts expected rules fire; runs dqlcost.Rewrite and re-lints, asserts specified rules no longer fire.
  • Tenant mode (make test-dqlbench, //go:build dqlbench) — executes each fixture via DQLExecutor.ExecuteQueryWithOptions(IncludeContributions=true, MetadataFields=["all"]), captures GrailMetadata.ScannedBytes for raw and rewritten forms, writes a timestamped markdown comparison report to test/dqlbench/reports/<ts>.md. Budget assertion fires when rewritten scan exceeds budget.max_scanned_bytes.
  • Three seed fixtures: errors-last-24h (missing from:/scanLimit), log-timeseries-7d (wrong shape — should use metric), cost-optimized (gold path, should produce zero findings).
  • Makefile targets: test-dqlbench-unit and test-dqlbench (the latter reuses the existing .integrationtests.env / DTCTL_INTEGRATION_* env var pattern already used by test-integration).
  • .gitignore: test/dqlbench/reports/ excluded (generated artifacts).

Tests in place

Benefits

For Dynatrace customers:

  • 500 GiB auto-applied scan ceiling protects against runaway DPS charges from mistyped filters, forgotten from: clauses, and LLM-generated DQL with bad shape — without requiring every dtctl user to remember the existing opt-in flag.
  • --no-guardrails preserves power-user workflows (exports, backfills, ad-hoc analytics) — no regression for existing scripts.
  • The harness makes it possible for customer teams to capture their own tenant's before/after cost profile and validate internally before deploying dashboard changes.

For Dynatrace:

Test plan

  • go test ./pkg/exec/... — existing + new guardrail tests pass
  • make test-dqlbench-unit — 3 fixtures green
  • dtctl query 'fetch logs, from:now()-5m | limit 1' --agent — scanned bytes metadata visible
  • dtctl query 'fetch logs, from:now()-5m | limit 1' --no-guardrails --agent — flag accepted; no guardrail in request body
  • Reviewers decide whether 500 GiB is the right default ceiling or should be tenant/config-driven
  • Reviewers review test/dqlbench/ layout, naming conventions, and whether the harness should live under test/e2e/ instead

Adds pkg/dqlcost, a pure-Go DQL cost linter with eight regex-based
rules (COST001-COST009; COST007 intentionally omitted after tenant
measurements showed column pruning does not reduce billable scan),
plus a conservative rewriter that applies safe mechanical fixes.

Wired into three CLI entry points:

  dtctl verify query --cost-lint [--strict-cost] [--rewrite-cost]
    Runs the linter on the canonical query; --strict-cost exits non-zero
    on warn-or-higher findings (CI-friendly); --rewrite-cost prints a
    rewritten version to stdout for pipe/diff inspection.

  dtctl exec copilot nl2dql --cost-lint [--rewrite-cost]
    Lints CoPilot-generated DQL before printing; optionally rewrites.
    Catches expensive shapes at generation time rather than runtime.

  dtctl apply --cost-lint [--strict-cost] [--rewrite-cost]
    Walks dashboard/notebook YAML, extracts every tile `query:` string
    via yaml.Node (preserving formatting and comments), and lints each.
    --rewrite-cost writes a sibling `.rewritten.<ext>` file and exits
    without submitting — never mutates the original.

Rule severities and rewriter defaults are calibrated from live tenant
measurements. Two notable calibrations:

  - COST003 (transform-wrapped filter, e.g. `lower(x) == "..."`) is
    SeverityError, not warn, because it was measured at ~9x scan
    amplification and ~15x latency penalty.
  - COST005 (`limit` before `summarize`) is SeverityInfo and NOT in the
    default rewriter: tenant data showed Grail short-circuits the scan
    when `limit` appears early, so `limit | summarize` actually scans
    less than `summarize | limit`. Semantics differ (partial sample vs
    full aggregate), so the linter flags for intent review only.

Refs: dynatrace-oss#175
…harness

Adds two complementary cost-safety features:

## pkg/exec/dql.go — GuardrailDefaults

A package-level GuardrailDefaults struct with a 500 GiB scan ceiling
auto-applied when the caller leaves DefaultScanLimitGbytes unset. The
helper applyGuardrailDefaults() runs at the top of
ExecuteQueryWithOptions, honors an opt-out flag (DisableGuardrails),
and preserves caller-specified values exactly. MaxResultRecords is
intentionally left at 0 so the server default continues to apply —
matches existing test expectations and avoids surprising behavior for
integrations that rely on server-side defaults.

## cmd/query.go — --no-guardrails

New --no-guardrails flag on `dtctl query` for power users who need
unrestricted scans (e.g. one-off exports, backfills). Default behavior:
guardrails active. Help text on --default-scan-limit-gbytes updated to
note the 500 GiB auto-applied default.

## test/dqlbench/ — regression harness

Fixture-based before/after harness for ongoing cost evaluation:

- fixtures/*.yaml — one per prompt/query with expect_rules,
  after_rewrite_no_rules, must_contain, must_not_contain, and
  budget.max_scanned_bytes fields.
- harness/harness.go — fixture loader.
- harness/harness_test.go — snapshot mode (no tenant): asserts expected
  rules fire on raw query and are cleared after rewriter runs.
- harness/bench_test.go — tenant mode (//go:build dqlbench): executes
  each fixture via DQLExecutor.ExecuteQueryWithOptions with
  IncludeContributions=true + MetadataFields=["all"], captures
  GrailMetadata.ScannedBytes, and writes a markdown comparison report
  to test/dqlbench/reports/<timestamp>.md.
- makefile — test-dqlbench-unit (snapshot, no tenant) and
  test-dqlbench (tenant via DTCTL_INTEGRATION_ENV/TOKEN).
- .gitignore — excludes test/dqlbench/reports/ (generated artifacts).

Three seed fixtures included: errors-last-24h, log-timeseries-7d,
cost-optimized (gold-path).

Refs: dynatrace-oss#175
@dynatrace-cla-bot

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@discostu105

Copy link
Copy Markdown
Collaborator

Feedback on the guardrails addition (delta over #177)

Thanks for this contribution, Dana — the domain knowledge here is really impressive, especially the COST005 calibration and the measurement-driven severity choices. The benchmark harness is a great idea for keeping rule quality honest over time.

I have one architectural suggestion on how the guardrail defaults are sourced. The current approach uses a package-level DefaultGuardrails variable with a hardcoded 500 GiB scan limit that's silently applied when callers pass 0. This works, but it has a couple of downsides:

  1. Silent behavioral change — today --default-scan-limit-gbytes 0 means "no limit." After this PR it means "use 500 GiB." Existing scripts doing large exports or backfills would get silently truncated results with no indication of why.
  2. One size fits all — a 500 GiB ceiling makes sense for production, but may be too restrictive for sandbox/dev environments, or too generous for cost-conscious teams. There's no way to differentiate without passing --no-guardrails or --default-scan-limit-gbytes on every invocation.

Proposal: source defaults from .dtctl.yaml per-context config

dtctl already has per-context configuration in .dtctl.yaml. We think query limits fit naturally there as a query_limits section:

contexts:
  production:
    env: https://abc123.apps.dynatrace.com
    token: dt0c01.xxx
    query_limits:
      scan_limit_gbytes: 500
      max_result_records: 0  # 0 = server default

  sandbox:
    env: https://sandbox.apps.dynatrace.com
    token: dt0c01.yyy
    query_limits:
      scan_limit_gbytes: 0  # no limit for dev

defaults:
  query_limits:
    scan_limit_gbytes: 500

This gives us:

  • No silent breaking change — existing configs without query_limits get current behavior. New installs or dtctl init can suggest 500 GiB as the default.
  • Per-context control — production gets a tight ceiling, sandbox gets none, without per-invocation flags.
  • No global mutable state — limits come from the config struct already threaded through the command rather than a package-level var.
  • --no-guardrails still works as a one-shot override meaning "ignore what the config says."
  • --default-scan-limit-gbytes 0 keeps meaning "no limit" — no semantic change to existing flags.
  • Discoverable — users see the knob in their config file rather than needing to read --help to learn the implicit 500 GiB default.

The applyGuardrailDefaults mechanism you wrote stays mostly the same — it would just read from the resolved context config instead of the package variable.

We also went back and forth on the naming — "guardrails" is a bit broad and could mean anything from input validation to permissions. Since this specifically covers query execution caps, query_limits feels more precise and matches the existing CLI flag vocabulary (--max-result-records, --default-scan-limit-gbytes).

Smaller notes

  • Test harness location — you raised the question yourself: should test/dqlbench/ live under test/e2e/dqlbench/ instead? Would be good to settle this before merge so it doesn't need moving later.
  • Makefile test-dqlbench target — the cat .integrationtests.env | grep | xargs pattern can break on values with spaces or special chars. Minor, but worth a look.
  • --no-guardrails scope — might be worth documenting exactly which limits this disables, so it's clear if future limits are added.

Really appreciate the care you've put into this series. Happy to discuss any of the above — especially the config-driven approach if you'd like to collaborate on the schema.

@herwigmoser

Copy link
Copy Markdown
Contributor

I agree with @discostu105 on all points, both on the proposed approach through the config file and opting in to a one shot override option, which should be named in closer alignment with query limits, rather than "guardrails", which is very broad and unspecific.

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.

4 participants