From d47a55eca689befb0a1550abe05ede1e1056602e Mon Sep 17 00:00:00 2001 From: C Turner Date: Thu, 19 Mar 2026 10:35:53 -0400 Subject: [PATCH 1/3] Improve reliability, scoring transparency, and query classification - io.py: wrap file operations with IoError for missing files, corrupt JSON, and OS failures - api_clients.py: add 3-attempt retry with exponential backoff and Retry-After parsing for 429 rate limits - recommendations.py: replace magic numbers with named constants with inline rationale comments - heuristics.py: extract COMPLEXITY_BLOCKER_THRESHOLD as a named constant - compare.py: name title similarity thresholds with explanatory comments - normalize.py: fix query_family() to strip comments before classifying and use word-boundary regex; distinguish Datadog DDSQL from Dynatrace USQL so Datadog-native SQL is not flagged for translation - terraform_planner.py: treat ddsql as validate_and_apply; use query family aware placeholders; mark DQL/USQL gaps with TRANSLATE prefix - tests: add DDSQL vs USQL classification coverage --- dashboard_tooling/api_clients.py | 65 ++++++++++++++++++++++---- dashboard_tooling/compare.py | 17 +++++-- dashboard_tooling/heuristics.py | 8 +++- dashboard_tooling/io.py | 42 ++++++++++++----- dashboard_tooling/normalize.py | 37 +++++++++++---- dashboard_tooling/recommendations.py | 52 ++++++++++++++------- dashboard_tooling/terraform_planner.py | 41 ++++++++++++++-- tests/test_tooling.py | 49 +++++++++++++++++++ 8 files changed, 259 insertions(+), 52 deletions(-) diff --git a/dashboard_tooling/api_clients.py b/dashboard_tooling/api_clients.py index ff0cddd..c82e0c8 100644 --- a/dashboard_tooling/api_clients.py +++ b/dashboard_tooling/api_clients.py @@ -1,8 +1,10 @@ from __future__ import annotations import json +import time from dataclasses import dataclass from typing import Callable +from urllib.error import HTTPError from urllib.parse import urlencode from urllib.request import Request, urlopen @@ -13,27 +15,74 @@ class ApiClientError(RuntimeError): pass +class RateLimitError(ApiClientError): + pass + + Transport = Callable[[Request], object] +# Number of retries on transient failures (5xx, network errors, rate limits) +_MAX_RETRIES = 3 +# Seconds to wait between retries; doubled each attempt +_RETRY_BASE_DELAY = 2.0 + def _default_transport(request: Request): return urlopen(request, timeout=60) +def _is_retryable(exc: Exception) -> bool: + if isinstance(exc, HTTPError): + return exc.code == 429 or exc.code >= 500 + return True # network / timeout errors are retryable + + +def _retry_delay(exc: Exception, attempt: int) -> float: + if isinstance(exc, HTTPError) and exc.code == 429: + retry_after = exc.headers.get("Retry-After") + if retry_after: + try: + return float(retry_after) + except ValueError: + pass + return _RETRY_BASE_DELAY * (2 ** attempt) + + @dataclass class JsonHttpClient: transport: Transport = _default_transport + def _execute(self, request: Request, url: str) -> str: + last_exc: Exception | None = None + for attempt in range(_MAX_RETRIES): + try: + with self.transport(request) as response: + return response.read().decode("utf-8") + except HTTPError as exc: + if exc.code == 429: + delay = _retry_delay(exc, attempt) + time.sleep(delay) + last_exc = exc + continue + if exc.code >= 500 and attempt < _MAX_RETRIES - 1: + time.sleep(_retry_delay(exc, attempt)) + last_exc = exc + continue + raise ApiClientError(f"HTTP {exc.code} for {url}: {exc.reason}") from exc + except Exception as exc: # pragma: no cover + if attempt < _MAX_RETRIES - 1: + time.sleep(_retry_delay(exc, attempt)) + last_exc = exc + continue + raise ApiClientError(f"request failed for {url}: {exc}") from exc + raise ApiClientError(f"request failed after {_MAX_RETRIES} attempts for {url}: {last_exc}") from last_exc + def get_json(self, url: str, headers: dict[str, str], *, query: dict[str, str] | None = None) -> object: final_url = url if query: final_url = f"{url}?{urlencode(query, doseq=True)}" request = Request(final_url, headers=headers, method="GET") - try: - with self.transport(request) as response: - payload = response.read().decode("utf-8") - except Exception as exc: # pragma: no cover - raise ApiClientError(f"request failed for {final_url}: {exc}") from exc + payload = self._execute(request, final_url) try: return json.loads(payload) except json.JSONDecodeError as exc: @@ -46,11 +95,7 @@ def request_json(self, method: str, url: str, headers: dict[str, str], *, body: payload_bytes = json.dumps(body).encode("utf-8") request_headers.setdefault("Content-Type", "application/json") request = Request(url, headers=request_headers, data=payload_bytes, method=method) - try: - with self.transport(request) as response: - payload = response.read().decode("utf-8") - except Exception as exc: # pragma: no cover - raise ApiClientError(f"request failed for {url}: {exc}") from exc + payload = self._execute(request, url) try: return json.loads(payload) except json.JSONDecodeError as exc: diff --git a/dashboard_tooling/compare.py b/dashboard_tooling/compare.py index aa9b7a5..a7a9274 100644 --- a/dashboard_tooling/compare.py +++ b/dashboard_tooling/compare.py @@ -2,9 +2,20 @@ from difflib import SequenceMatcher +from dashboard_tooling.heuristics import COMPLEXITY_BLOCKER_THRESHOLD from dashboard_tooling.models import DashboardRecord, ParityRecord from dashboard_tooling.normalize import normalize_title +# SequenceMatcher ratio thresholds for dashboard title matching. +# STRONG (0.92): titles that differ only by minor word reordering or punctuation +# e.g. "Service Latency Overview" vs "Service Latency - Overview" +# WEAK (0.72): titles that share most tokens but have one meaningful difference +# e.g. "Host CPU Usage" vs "Host Memory Usage" — useful for flagging candidates, +# not for automatic matching. +# Values below WEAK are treated as no match (missing_in_target). +DEFAULT_STRONG_MATCH_THRESHOLD: float = 0.92 +DEFAULT_WEAK_MATCH_THRESHOLD: float = 0.72 + def _title_similarity(left: str, right: str) -> float: return SequenceMatcher(None, normalize_title(left), normalize_title(right)).ratio() @@ -14,8 +25,8 @@ def compare_dashboards( source_dashboards: list[DashboardRecord], target_dashboards: list[DashboardRecord], *, - strong_match_threshold: float = 0.92, - weak_match_threshold: float = 0.72, + strong_match_threshold: float = DEFAULT_STRONG_MATCH_THRESHOLD, + weak_match_threshold: float = DEFAULT_WEAK_MATCH_THRESHOLD, ) -> list[ParityRecord]: parity_records: list[ParityRecord] = [] for source in source_dashboards: @@ -56,7 +67,7 @@ def compare_dashboards( if source.query_count == 0: record.manual_review_reasons.append("source_needs_manual_query_capture") - if source.complexity_score >= 12: + if source.complexity_score >= COMPLEXITY_BLOCKER_THRESHOLD: record.manual_review_reasons.append("high_complexity") if source.variables and not best_target: record.heuristic_blockers.append("dynamic_filter_mapping_required") diff --git a/dashboard_tooling/heuristics.py b/dashboard_tooling/heuristics.py index b554e36..823ca5a 100644 --- a/dashboard_tooling/heuristics.py +++ b/dashboard_tooling/heuristics.py @@ -4,6 +4,11 @@ from dashboard_tooling.models import DashboardRecord, QueryRecord +# Complexity score at or above this value triggers the high_complexity_dashboard blocker. +# Score is: widget_count + query_count*2 + variable_count + (table_widget_count * 2). +# A value of 12 corresponds roughly to a 4-widget, 3-query dashboard with one table widget. +COMPLEXITY_BLOCKER_THRESHOLD = 12 + def infer_query_signals(query: QueryRecord) -> list[str]: text = query.query_text.lower() @@ -18,6 +23,7 @@ def infer_query_signals(query: QueryRecord) -> list[str]: signals.append("result_shaping_logic") if any(token in text for token in ("timezone", "timestamp", "date_trunc", "bin(", "timeshift")): signals.append("time_semantics_risk") + # ddsql is Datadog-native and does not need translation; sql_like is Dynatrace USQL if query.query_family in {"sql_like", "dynatrace_dql"}: signals.append("translation_review_required") if "select " in text and " from " in text: @@ -43,7 +49,7 @@ def infer_dashboard_blockers(dashboard: DashboardRecord) -> list[str]: blockers["table_or_list_result_shaping"] += 1 if any(token in item.lower() for item in dashboard.widget_types for token in ("markdown", "note")): blockers["textual_context_dependency"] += 1 - if dashboard.complexity_score >= 12: + if dashboard.complexity_score >= COMPLEXITY_BLOCKER_THRESHOLD: blockers["high_complexity_dashboard"] += 1 return sorted(blockers) diff --git a/dashboard_tooling/io.py b/dashboard_tooling/io.py index e67a729..9d7c70c 100644 --- a/dashboard_tooling/io.py +++ b/dashboard_tooling/io.py @@ -5,9 +5,20 @@ from pathlib import Path +class IoError(RuntimeError): + pass + + def load_json(path: Path) -> object: - with path.open("r", encoding="utf-8") as fh: - return json.load(fh) + if not path.exists(): + raise IoError(f"file not found: {path}") + try: + with path.open("r", encoding="utf-8") as fh: + return json.load(fh) + except json.JSONDecodeError as exc: + raise IoError(f"invalid JSON in {path}: {exc}") from exc + except OSError as exc: + raise IoError(f"could not read {path}: {exc}") from exc def ensure_dir(path: Path) -> None: @@ -15,19 +26,28 @@ def ensure_dir(path: Path) -> None: def write_json(path: Path, payload: object) -> None: - with path.open("w", encoding="utf-8") as fh: - json.dump(payload, fh, indent=2) + try: + with path.open("w", encoding="utf-8") as fh: + json.dump(payload, fh, indent=2) + except OSError as exc: + raise IoError(f"could not write {path}: {exc}") from exc def write_csv(path: Path, rows: list[dict[str, object]], fieldnames: list[str]) -> None: - with path.open("w", encoding="utf-8", newline="") as fh: - writer = csv.DictWriter(fh, fieldnames=fieldnames) - writer.writeheader() - for row in rows: - writer.writerow(row) + try: + with path.open("w", encoding="utf-8", newline="") as fh: + writer = csv.DictWriter(fh, fieldnames=fieldnames) + writer.writeheader() + for row in rows: + writer.writerow(row) + except OSError as exc: + raise IoError(f"could not write {path}: {exc}") from exc def write_text(path: Path, content: str) -> None: - with path.open("w", encoding="utf-8") as fh: - fh.write(content) + try: + with path.open("w", encoding="utf-8") as fh: + fh.write(content) + except OSError as exc: + raise IoError(f"could not write {path}: {exc}") from exc diff --git a/dashboard_tooling/normalize.py b/dashboard_tooling/normalize.py index c18e859..d779710 100644 --- a/dashboard_tooling/normalize.py +++ b/dashboard_tooling/normalize.py @@ -16,16 +16,30 @@ def normalize_title(value: str) -> str: return re.sub(r"[^a-z0-9]+", " ", value.lower()).strip() -def query_family(query_text: str) -> str: - lowered = query_text.lower() - if "select " in lowered: - return "sql_like" - if "timeseries " in lowered or "fetch " in lowered: +def query_family(query_text: str, source_system: str = "") -> str: + # Strip single-line and block comments before classifying to avoid matching + # keywords that appear only in comment text (e.g. "// timeseries" or "-- select"). + stripped = re.sub(r"(--[^\n]*|//[^\n]*|/\*.*?\*/)", " ", query_text, flags=re.DOTALL).lower().strip() + + # SQL-shaped query (SELECT ... FROM): the family depends on origin. + # - Dynatrace USQL (user sessions query language) looks like SQL but targets DT data. + # - Datadog DDSQL is Datadog-native and does not require translation. + # Both are flagged separately so downstream logic can treat them correctly. + if re.search(r"\bselect\b.+\bfrom\b", stripped, re.DOTALL): + return "ddsql" if source_system == "datadog" else "sql_like" + + # Dynatrace DQL: timeseries or fetch must appear at the start of a statement + if re.search(r"(?:^|;|\|)\s*(?:timeseries|fetch)\b", stripped): return "dynatrace_dql" - if "avg:" in lowered or "sum:" in lowered or "anomalies(" in lowered: + + # Datadog metric query syntax: metric selectors use colon notation + if re.search(r"\b(?:avg|sum|min|max|count):[a-z]", stripped) or "anomalies(" in stripped: return "datadog_metric" - if "logs(" in lowered or "service:" in lowered or "@http." in lowered: + + # Datadog log/trace query syntax + if "logs(" in stripped or re.search(r"service:[a-z\-_*]", stripped) or "@http." in stripped: return "datadog_logs" + return "unknown" @@ -56,6 +70,7 @@ def _append_query( widget_title: str, widget_type: str, query_text: str, + source_system: str = "", ) -> None: cleaned = str(query_text).strip() if not cleaned: @@ -68,7 +83,7 @@ def _append_query( widget_title=widget_title, widget_type=widget_type, query_text=cleaned, - query_family=query_family(cleaned), + query_family=query_family(cleaned, source_system), heuristic_signals=[], ) ) @@ -135,6 +150,7 @@ def normalize_dynatrace_dashboards(payload: object) -> list[DashboardRecord]: widget_title=widget_title, widget_type=widget_type, query_text=str(item), + source_system="dynatrace", ) elif path: _append_query( @@ -143,6 +159,7 @@ def normalize_dynatrace_dashboards(payload: object) -> list[DashboardRecord]: widget_title=widget_title, widget_type=widget_type, query_text=str(path), + source_system="dynatrace", ) if isinstance(widget.get("markdown"), str): @@ -152,6 +169,7 @@ def normalize_dynatrace_dashboards(payload: object) -> list[DashboardRecord]: widget_title=widget_title, widget_type=widget_type, query_text=str(widget["markdown"]), + source_system="dynatrace", ) dashboard.widget_types = sorted(type_counter) @@ -220,6 +238,7 @@ def normalize_datadog_dashboards(payload: object) -> list[DashboardRecord]: widget_title=widget_title, widget_type=widget_type, query_text=str(request["q"]), + source_system="datadog", ) formulas = request.get("formulas") if isinstance(formulas, list): @@ -231,6 +250,7 @@ def normalize_datadog_dashboards(payload: object) -> list[DashboardRecord]: widget_title=widget_title, widget_type=widget_type, query_text=str(formula["formula"]), + source_system="datadog", ) if definition.get("query"): _append_query( @@ -239,6 +259,7 @@ def normalize_datadog_dashboards(payload: object) -> list[DashboardRecord]: widget_title=widget_title, widget_type=widget_type, query_text=str(definition["query"]), + source_system="datadog", ) dashboard.widget_types = sorted(type_counter) diff --git a/dashboard_tooling/recommendations.py b/dashboard_tooling/recommendations.py index cc2d90f..c585dde 100644 --- a/dashboard_tooling/recommendations.py +++ b/dashboard_tooling/recommendations.py @@ -4,6 +4,36 @@ from dashboard_tooling.models import DashboardRecommendation, DashboardRecord +# Minimum value_score to consider a dashboard worth building +VALUE_BUILD_THRESHOLD = 5 +# Minimum value_score to keep a dashboard as a candidate pending answers +VALUE_CANDIDATE_THRESHOLD = 4 + +# Base automation score before penalties; represents a dashboard with no known blockers +AUTOMATION_SCORE_BASE = 8 + +# Penalty weights — calibrated so that the three manual-only signals (all 4-pt +# blockers together) push the score below the high-confidence threshold of 6, +# while single soft blockers (1 pt) still allow automation-assisted workflows. +AUTOMATION_PENALTIES: dict[str, int] = { + "translation_review_required": 1, # DQL/SQL present; mapping work needed but automatable + "custom_logic_dependency": 2, # CASE/COALESCE/window logic; likely needs redesign + "multi_stage_query_logic": 2, # JOINs/UNIONs; Datadog has no equivalent primitives + "dynamic_filter_dependency": 1, # Template variables exist; mapping needed but low risk + "table_or_list_result_shaping": 1, # Table widgets; layout parity may need manual review + "textual_context_dependency": 1, # Markdown notes; content must be reviewed manually + "manual_query_capture_required": 4, # No query payload at all; cannot automate without input + "high_complexity_dashboard": 2, # Complexity score >= threshold; risk of missed signals +} + +# Automation score thresholds: +# >= HIGH → Terraform-ready; no manual query work required +# >= MED → Terraform possible after query mapping +# < MED → Manual design first; automation not viable yet +AUTOMATION_HIGH_THRESHOLD = 6 +AUTOMATION_MED_THRESHOLD = 4 + + VALUE_KEYWORDS: dict[str, str] = { "overview": "service_health", @@ -91,21 +121,11 @@ def _value_score(dashboard: DashboardRecord, tier: str) -> tuple[int, list[str], def _automation_score(dashboard: DashboardRecord) -> tuple[int, bool, str, list[str], list[str], str]: blockers = set(dashboard.heuristic_blockers) | set(dashboard.annotation_blockers) - score = 8 + score = AUTOMATION_SCORE_BASE required_inputs: list[str] = [] open_questions: list[str] = [] - penalties = { - "translation_review_required": 1, - "custom_logic_dependency": 2, - "multi_stage_query_logic": 2, - "dynamic_filter_dependency": 1, - "table_or_list_result_shaping": 1, - "textual_context_dependency": 1, - "manual_query_capture_required": 4, - "high_complexity_dashboard": 2, - } - for blocker, penalty in penalties.items(): + for blocker, penalty in AUTOMATION_PENALTIES.items(): if blocker in blockers: score -= penalty @@ -138,11 +158,11 @@ def _automation_score(dashboard: DashboardRecord) -> tuple[int, bool, str, list[ open_questions.append("Which ownership and scope tags should be applied in Datadog?") score = max(score, 0) - if score >= 6 and "manual_query_capture_required" not in blockers: + if score >= AUTOMATION_HIGH_THRESHOLD and "manual_query_capture_required" not in blockers: terraform_strategy = "terraform_dashboard_json" terraform_ready = True confidence = "high" - elif score >= 4: + elif score >= AUTOMATION_MED_THRESHOLD: terraform_strategy = "terraform_after_query_mapping" terraform_ready = False confidence = "medium" @@ -200,9 +220,9 @@ def recommend_dashboard(dashboard: DashboardRecord) -> DashboardRecommendation: confidence, ) = _automation_score(dashboard) - if value_score >= 5 and terraform_ready: + if value_score >= VALUE_BUILD_THRESHOLD and terraform_ready: status = "create_with_terraform" - elif value_score >= 4: + elif value_score >= VALUE_CANDIDATE_THRESHOLD: status = "candidate_pending_answers" elif dashboard.query_count == 0: status = "defer_missing_signal" diff --git a/dashboard_tooling/terraform_planner.py b/dashboard_tooling/terraform_planner.py index 95ab50e..14df420 100644 --- a/dashboard_tooling/terraform_planner.py +++ b/dashboard_tooling/terraform_planner.py @@ -22,7 +22,8 @@ def _group_queries(dashboard: DashboardRecord) -> list[QueryRecord]: def _suggest_definition_type(query: QueryRecord) -> str: widget_type = query.widget_type.lower() - if "table" in widget_type or query.query_family == "sql_like": + # sql_like (Dynatrace USQL) and ddsql (Datadog DDSQL) both return tabular results + if "table" in widget_type or query.query_family in {"sql_like", "ddsql"}: return "query_table" if "markdown" in widget_type or "note" in widget_type: return "note" @@ -37,15 +38,49 @@ def _mapping_status(query: QueryRecord) -> tuple[str, list[str]]: if query.query_family in {"dynatrace_dql", "sql_like"}: notes.append("Translate the source query to Datadog-native telemetry before applying Terraform.") return "query_translation_required", notes + # ddsql is Datadog's own SQL dialect — no translation needed, but semantics must be validated + if query.query_family == "ddsql": + notes.append("DDSQL is Datadog-native; validate query semantics and table scoping before apply.") + return "validate_and_apply", notes notes.append("Query family already resembles Datadog syntax; validate semantics before apply.") return "validate_and_apply", notes def _placeholder_query(query: QueryRecord, definition_type: str) -> str: - if definition_type == "query_table": - return "avg:system.load.1{*} by {host}" + # Notes have no query — return empty and let the draft widget use source text as context. if definition_type == "note": return "" + # Use the query family to hint at a more semantically relevant placeholder. + # All placeholders are clearly synthetic; the mapping_status field tells the + # engineer whether translation work is required before applying Terraform. + family = query.query_family + if family == "ddsql": + # DDSQL is Datadog-native; use a generic table placeholder that still compiles + if definition_type == "query_table": + return "SELECT host, AVG(system.cpu.user) FROM metrics GROUP BY host" + return "SELECT host, AVG(system.cpu.user) FROM metrics GROUP BY host" + if family == "datadog_metric": + # Source already uses Datadog metric syntax — mirror the structure + base = "avg:system.cpu.user{*}" + if definition_type == "query_table": + return f"{base} by {{host}}" + return base + if family == "datadog_logs": + if definition_type == "query_table": + return "logs(\"service:*\").index(\"*\").rollup(\"count\").last(\"15m\")" + return "logs(\"service:*\").index(\"*\").rollup(\"count\").last(\"15m\")" + if family == "dynatrace_dql": + # DQL cannot be used in Datadog; placeholder makes the gap explicit + if definition_type == "query_table": + return "# TRANSLATE: avg:custom.metric{*} by {host}" + return "# TRANSLATE: avg:custom.metric{*}" + if family == "sql_like": + if definition_type == "query_table": + return "# TRANSLATE: avg:custom.metric{*} by {host}" + return "# TRANSLATE: avg:custom.metric{*}" + # unknown family — generic placeholder, table variant groups by host + if definition_type == "query_table": + return "avg:system.load.1{*} by {host}" return "avg:system.load.1{*}" diff --git a/tests/test_tooling.py b/tests/test_tooling.py index b0d7dd5..71c25d3 100644 --- a/tests/test_tooling.py +++ b/tests/test_tooling.py @@ -232,5 +232,54 @@ def test_terraform_planner_emits_import_mode_for_existing_dashboard(self) -> Non self.assertTrue(plans[0].import_instructions) + def test_ddsql_not_flagged_for_translation(self) -> None: + # A Datadog dashboard with a DDSQL query should be classified as "ddsql", + # not "sql_like", and must not receive a translation_review_required signal. + payload = { + "dashboards": [ + { + "id": "dd-sql", + "title": "Cost Overview", + "widgets": [ + { + "definition": { + "type": "query_table", + "title": "Cost by Service", + "requests": [{"q": "SELECT service, SUM(cost) FROM usage GROUP BY service"}], + } + } + ], + } + ] + } + dashboards = normalize_datadog_dashboards(payload) + query = dashboards[0].queries[0] + self.assertEqual(query.query_family, "ddsql") + self.assertNotIn("translation_review_required", query.heuristic_signals) + + def test_dynatrace_usql_still_flagged_for_translation(self) -> None: + # A Dynatrace dashboard with a USQL query must remain "sql_like" and + # receive translation_review_required since it targets DT data. + payload = { + "dashboards": [ + { + "id": "dt-sql", + "name": "Session Overview", + "tiles": [ + { + "name": "Sessions", + "tileType": "TABLE", + "query": "SELECT usersession.userType, COUNT(*) FROM usersession GROUP BY usersession.userType", + } + ], + } + ] + } + dashboards = normalize_dynatrace_dashboards(payload) + query = dashboards[0].queries[0] + self.assertEqual(query.query_family, "sql_like") + self.assertIn("translation_review_required", query.heuristic_signals) + + if __name__ == "__main__": unittest.main() From 4bafa503df91ca580b7daf10535db8d1a9d36cc8 Mon Sep 17 00:00:00 2001 From: C Turner Date: Thu, 19 Mar 2026 10:43:53 -0400 Subject: [PATCH 2/3] Suppress Bandit false positives with nosec annotations - api_clients.py: B310 urlopen targets env-configured API endpoints only - deployment.py: B404/B603 subprocess calls are intentional Terraform invocations --- dashboard_tooling/api_clients.py | 2 +- dashboard_tooling/deployment.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dashboard_tooling/api_clients.py b/dashboard_tooling/api_clients.py index c82e0c8..d9e751d 100644 --- a/dashboard_tooling/api_clients.py +++ b/dashboard_tooling/api_clients.py @@ -28,7 +28,7 @@ class RateLimitError(ApiClientError): def _default_transport(request: Request): - return urlopen(request, timeout=60) + return urlopen(request, timeout=60) # nosec B310 — URLs are env-configured API endpoints, not user input def _is_retryable(exc: Exception) -> bool: diff --git a/dashboard_tooling/deployment.py b/dashboard_tooling/deployment.py index 73a9fcd..85b40fb 100644 --- a/dashboard_tooling/deployment.py +++ b/dashboard_tooling/deployment.py @@ -2,7 +2,7 @@ import json import shutil -import subprocess +import subprocess # nosec B404 from dataclasses import dataclass from pathlib import Path @@ -96,4 +96,4 @@ def run_terraform(work_dir: Path, command: str, *, auto_approve: bool = False) - args = [terraform, command] if command == "apply" and auto_approve: args.append("-auto-approve") - return subprocess.run(args, cwd=work_dir, check=True, capture_output=True, text=True) + return subprocess.run(args, cwd=work_dir, check=True, capture_output=True, text=True) # nosec B603 From a609b0be92f0d4b7c0e7a761207c446407ef4bfa Mon Sep 17 00:00:00 2001 From: C Turner Date: Thu, 19 Mar 2026 10:56:40 -0400 Subject: [PATCH 3/3] Fix retry logic, dead code, and redundant placeholder branches - api_clients.py: fix 429 retry to respect MAX_RETRIES on final attempt and raise RateLimitError instead of generic ApiClientError; remove unused _is_retryable() function - terraform_planner.py: collapse _placeholder_query branches where both definition_type arms returned identical strings; merge dynatrace_dql and sql_like into a single branch --- dashboard_tooling/api_clients.py | 15 +++++---------- dashboard_tooling/terraform_planner.py | 12 ++---------- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/dashboard_tooling/api_clients.py b/dashboard_tooling/api_clients.py index d9e751d..179fa6e 100644 --- a/dashboard_tooling/api_clients.py +++ b/dashboard_tooling/api_clients.py @@ -31,12 +31,6 @@ def _default_transport(request: Request): return urlopen(request, timeout=60) # nosec B310 — URLs are env-configured API endpoints, not user input -def _is_retryable(exc: Exception) -> bool: - if isinstance(exc, HTTPError): - return exc.code == 429 or exc.code >= 500 - return True # network / timeout errors are retryable - - def _retry_delay(exc: Exception, attempt: int) -> float: if isinstance(exc, HTTPError) and exc.code == 429: retry_after = exc.headers.get("Retry-After") @@ -60,10 +54,11 @@ def _execute(self, request: Request, url: str) -> str: return response.read().decode("utf-8") except HTTPError as exc: if exc.code == 429: - delay = _retry_delay(exc, attempt) - time.sleep(delay) - last_exc = exc - continue + if attempt < _MAX_RETRIES - 1: + time.sleep(_retry_delay(exc, attempt)) + last_exc = exc + continue + raise RateLimitError(f"rate limited after {_MAX_RETRIES} attempts for {url}") from exc if exc.code >= 500 and attempt < _MAX_RETRIES - 1: time.sleep(_retry_delay(exc, attempt)) last_exc = exc diff --git a/dashboard_tooling/terraform_planner.py b/dashboard_tooling/terraform_planner.py index 14df420..6b3e03a 100644 --- a/dashboard_tooling/terraform_planner.py +++ b/dashboard_tooling/terraform_planner.py @@ -56,8 +56,6 @@ def _placeholder_query(query: QueryRecord, definition_type: str) -> str: family = query.query_family if family == "ddsql": # DDSQL is Datadog-native; use a generic table placeholder that still compiles - if definition_type == "query_table": - return "SELECT host, AVG(system.cpu.user) FROM metrics GROUP BY host" return "SELECT host, AVG(system.cpu.user) FROM metrics GROUP BY host" if family == "datadog_metric": # Source already uses Datadog metric syntax — mirror the structure @@ -66,15 +64,9 @@ def _placeholder_query(query: QueryRecord, definition_type: str) -> str: return f"{base} by {{host}}" return base if family == "datadog_logs": - if definition_type == "query_table": - return "logs(\"service:*\").index(\"*\").rollup(\"count\").last(\"15m\")" return "logs(\"service:*\").index(\"*\").rollup(\"count\").last(\"15m\")" - if family == "dynatrace_dql": - # DQL cannot be used in Datadog; placeholder makes the gap explicit - if definition_type == "query_table": - return "# TRANSLATE: avg:custom.metric{*} by {host}" - return "# TRANSLATE: avg:custom.metric{*}" - if family == "sql_like": + if family in {"dynatrace_dql", "sql_like"}: + # DQL/USQL cannot be used in Datadog; placeholder makes the gap explicit if definition_type == "query_table": return "# TRANSLATE: avg:custom.metric{*} by {host}" return "# TRANSLATE: avg:custom.metric{*}"