Skip to content

Add lint rule to enforce no rST markup in tests/scripts prose - #20929

Draft
dd-apm-ecosystems[bot] wants to merge 4 commits into
mainfrom
automated/lint-rst-markup-in-prose-2026-10-09
Draft

dd-apm-ecosystems[bot] wants to merge 4 commits into
mainfrom
automated/lint-rst-markup-in-prose-2026-10-09

Conversation

@dd-apm-ecosystems

Copy link
Copy Markdown

Summary

This PR adds a mechanically-detectable lint rule enforcing an existing documented convention: no reStructuredText (rST) double-backtick markup in non-Sphinx-rendered prose (docstrings, comments) under tests/ or scripts/.

(a) Review theme and frequency

Scanning review comments on human-authored PRs updated/opened against DataDog/dd-trace-py in the trailing 30 days, this exact theme — reviewers flagging newly added like_this rST markup in test/script docstrings or comments as violating the repo's documented "plain prose for non-rendered code" convention — was observed in at least 12 of the ~101 human-authored PRs that received review comments in the window (e.g. PRs #19273, #19857, #19939, #20526, #20565, #20671, #20688, #20700, #20772, #20773, #20777, #20900; a broader 34 PRs had the same markup flagged across all files, not just tests//scripts/). Several of these were explicitly accepted and fixed in follow-up commits, e.g.:

Even a single confirmed, accepted, mechanically-detectable occurrence in the window would be sufficient grounds to act (per the standing automation policy for this task) — the fact that this recurred across a dozen distinct PRs is additional supporting evidence that the pattern is persistent and worth automating away.

(b) Why this is a good automation candidate

  • Mechanically detectable: a regex matching text (double backticks, not triple-or-more, to avoid Markdown code fences) with essentially no semantic judgment required.
  • Low false-positive rate: the rule only fires on added diff lines (incremental, like the existing check_no_new_aidev_anchors.py), and only inside tests/ and scripts/ — the two directories where nothing is Sphinx-rendered (verified: no automodule directive under docs/ points at either), so every match there is unambiguously a convention violation, never a legitimate rendered docstring.
  • No broad codebase breakage: because it's diff-scoped, the ~220 pre-existing double-backtick occurrences already in tests//scripts/ are untouched; only new occurrences fail CI.

(c) Tooling choice and CI integration

This repo already has an idiomatic precedent for exactly this kind of check: scripts/check_no_new_aidev_anchors.py, a custom incremental-diff Python script (not a generic linter rule), wired into both GitLab CI (.gitlab-ci.yml → check_no_new_aidev_anchors job, which blocks merge) and tests/suitespec.yml (so its own unit tests run in the internal test suite). I followed this exact pattern rather than introducing a new tool:

  • Added scripts/check_no_rst_markup_in_prose.py — same structure as the aidev-anchors checker (merge-base diff, added-lines-only, excluded self/own-test files).
  • Added tests/internal/test_check_no_rst_markup_in_prose.py — unit tests mirroring the aidev-anchors test file's structure.
  • Added a new check_no_rst_markup_in_prose GitLab CI job in .gitlab-ci.yml, immediately after check_no_new_aidev_anchors, with identical stage/rules/id_tokens — this job runs on every PR (via the GitLab mirror) and blocks merge on failure, the same way check_no_new_aidev_anchors does today.
  • Added scripts/check_no_rst_markup_in_prose.py to the internal component's paths: in tests/suitespec.yml, next to check_no_new_aidev_anchors.py, so the new checker's own unit tests are included in that CI suite.
  • Excluded the new script from bandit scanning in pyproject.toml, mirroring the existing exclusion for check_no_new_aidev_anchors.py.
  • Documented the rule in AGENTS.md under a new "rST Markup in tests/ and scripts/" section, next to the existing "AIDEV Anchor Comments" section it mirrors.

I also attempted to add a GitHub Actions pull_request workflow (mirroring check-no-new-aidev-anchors.yml) as an additional PR-only gate, but the automation's GitHub App lacks permission to write .github/workflows/*; the GitLab CI job above is the actual existing merge-blocking mechanism for this repo (GitHub Actions workflows here appear to be supplementary/PR-context-only duplicates of GitLab jobs), so it alone is sufficient.

(d) How to verify locally

# Run the checker against your branch's diff from main:
python3 scripts/check_no_rst_markup_in_prose.py --base-ref origin/main

# Run its unit tests (via the repo's test runner, per AGENTS.md convention):
scripts/run-tests tests/internal/test_check_no_rst_markup_in_prose.py

A clean run prints OK: no new rST double-backtick markup on added lines under tests/ or scripts/. and exits 0; a violation prints the offending file/line and exits 1.


Opened as a draft per the automated code-review-efficiency workflow. Human review/merge required.

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Pipelines  Tests

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 16 Pipeline jobs failed

Changelog | Validate changelog

View more details · View in GitHub Actions

DataDog/apm-reliability/dd-trace-py | prechecks

View more details · View in GitLab

System Tests | integration-frameworks-system-tests / End-to-end #1 / anthropic-py@0.75.0 1

View more details · View in GitHub Actions

View all 16 failed jobs.

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 0042d21 | Docs | View more details | Give us feedback!

@cit-pr-commenter-54b7da

Copy link
Copy Markdown

Codeowners resolved as

Resolved from the full PR diff against main using the target branch CODEOWNERS file.
CODEOWNERS team requests not listed below are not required by the current file set.

AGENTS.md                                                               @DataDog/python-guild
pyproject.toml                                                          @DataDog/python-guild
scripts/check_no_rst_markup_in_prose.py                                 @DataDog/python-guild
tests/internal/test_check_no_rst_markup_in_prose.py                     @DataDog/apm-core-python

@cit-pr-commenter-54b7da

Copy link
Copy Markdown

Circular import analysis

⚠️ Existing circular imports

There are 1 circular imports that already exist on the base branch and have not been changed by this PR.

ddtrace.errortracking._handled_exceptions.bytecode_injector -> ddtrace.errortracking._handled_exceptions.callbacks -> ddtrace.errortracking._handled_exceptions.collector -> ddtrace.errortracking._handled_exceptions.bytecode_reporting -> ddtrace.errortracking._handled_exceptions.bytecode_injector

@cit-pr-commenter-54b7da

Copy link
Copy Markdown

Dependency direction analysis

⚠️ Existing dependency direction violations

There are 174 dependency direction violations that already exist on the base branch and have not been changed by this PR.

Show existing violations (showing 5 of 174 highest severity)
ddtrace.internal.tracemethods -×-> ddtrace.trace  (internal-core -> product:tracing, score=124)
ddtrace.appsec._listeners -×-> ddtrace.trace  (product:appsec -> product:tracing, score=122)
ddtrace.llmobs._integrations.pydantic_ai -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=122)
ddtrace.internal.test_visibility.api -×-> ddtrace.trace  (product:ci_visibility -> product:tracing, score=122)
ddtrace.llmobs._integrations.vllm -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=122)

To see all violations, download the layers-base.json and layers-pr.json artifacts from this CI job and run:

uv run --script scripts/import-analysis/layers.py compare layers-base.json layers-pr.json

@pr-commenter

pr-commenter Bot commented Oct 9, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-10-09 22:04:47

Comparing candidate commit 0042d21 in PR branch automated/lint-rst-markup-in-prose-2026-10-09 with baseline commit 1ff6b81 in branch main.

📊 Benchmarking dashboard

Found 0 performance improvements and 10 performance regressions! Performance is the same for 588 metrics, 10 unstable metrics, 6 known flaky benchmarks, 18 flaky benchmarks without significant changes.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:httppropagationextract-tracecontext_headers

  • 🟥 execution_time [+3.118µs; +3.352µs] or [+10.316%; +11.091%]

scenario:iastaspects-add_aspect

  • 🟥 execution_time [+6.875µs; +9.360µs] or [+7.719%; +10.508%]

scenario:iastaspects-format_map_noaspect

  • 🟥 execution_time [+95.029µs; +100.027µs] or [+26.163%; +27.539%]

scenario:iastaspects-lstrip_aspect

  • 🟥 execution_time [+104.103µs; +108.422µs] or [+44.495%; +46.342%]

scenario:iastaspects-upper_aspect

  • 🟥 execution_time [+90.169µs; +94.631µs] or [+38.211%; +40.102%]

scenario:iastaspectsremodule-re_expand_aspect

  • 🟥 execution_time [+88.064µs; +96.161µs] or [+14.064%; +15.357%]

scenario:msgpackencoderscenario-simple_one_span

  • 🟥 execution_time [+467.399ns; +532.328ns] or [+11.937%; +13.595%]

scenario:openfeatureflagevaluation-hook-enqueue-typical

  • 🟥 execution_time [+1.700µs; +1.844µs] or [+8.026%; +8.708%]

scenario:otelspan-start

  • 🟥 execution_time [+2.541ms; +3.428ms] or [+9.882%; +13.335%]

scenario:recursivecomputation-shallow

  • 🟥 execution_time [+59.342µs; +63.140µs] or [+8.549%; +9.096%]

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:coreapiscenario-context_with_data_listeners

  • unstable execution_time [-746.010ns; +725.486ns] or [-7.167%; +6.970%]

scenario:coreapiscenario-core_dispatch_1_listener

  • unstable execution_time [-37.523ns; +41.157ns] or [-5.620%; +6.164%]

scenario:coreapiscenario-core_dispatch_50_listeners

  • unstable execution_time [-1881.979ns; +1940.224ns] or [-9.460%; +9.753%]

scenario:coreapiscenario-core_dispatch_exception_listeners

  • unstable execution_time [-1902.148ns; +1738.021ns] or [-9.993%; +9.131%]

scenario:coreapiscenario-core_dispatch_listeners

  • unstable execution_time [-373.558ns; +384.582ns] or [-8.763%; +9.022%]

scenario:coreapiscenario-core_dispatch_no_args_listeners

  • unstable execution_time [-222.962ns; +232.140ns] or [-8.298%; +8.639%]

scenario:coreapiscenario-core_dispatch_with_results_1_listener

  • unstable execution_time [-112.836ns; +74.912ns] or [-8.243%; +5.472%]

scenario:coreapiscenario-core_dispatch_with_results_50_listeners

  • unstable execution_time [-4979.724ns; +4384.563ns] or [-10.203%; +8.983%]

scenario:coreapiscenario-core_dispatch_with_results_listeners

  • unstable execution_time [-996.032ns; +889.686ns] or [-9.659%; +8.628%]

scenario:packagesupdateimporteddependencies-import_many_stdlib_cached

  • unstable execution_time [-59.500µs; +48.599µs] or [-10.515%; +8.588%]

Known flaky benchmarks

These benchmarks are marked as flaky and will not trigger a failure. Modify FLAKY_BENCHMARKS_REGEX to control which benchmarks are marked as flaky.

scenario:httppropagationinject-ids_only

  • 🟥 execution_time [+2.500µs; +2.605µs] or [+18.464%; +19.241%]

scenario:iastaspectsospath-ospathbasename_aspect

  • 🟥 execution_time [+142.066µs; +149.559µs] or [+37.595%; +39.578%]

scenario:iastaspectssplit-rsplit_aspect

  • 🟥 execution_time [+29.692µs; +32.878µs] or [+19.308%; +21.379%]

scenario:span-start

  • 🟥 execution_time [+1.481ms; +1.776ms] or [+13.495%; +16.185%]

scenario:telemetryaddmetric-1-count-metric-1-times

  • 🟥 execution_time [+180.756ns; +213.279ns] or [+9.339%; +11.020%]

scenario:tracer-small

  • 🟥 execution_time [+44.818µs; +46.396µs] or [+18.388%; +19.036%]

Known flaky benchmarks without significant changes:

  • scenario:errortrackingflasksqli-baseline
  • scenario:flasksimple-iast-get
  • scenario:iastaspects-casefold_aspect
  • scenario:iastaspects-casefold_noaspect
  • scenario:iastaspects-index_aspect
  • scenario:iastaspects-ljust_noaspect
  • scenario:iastaspects-lower_aspect
  • scenario:iastaspects-replace_aspect
  • scenario:iastaspects-rstrip_aspect
  • scenario:iastaspects-swapcase_aspect
  • scenario:iastaspects-title_noaspect
  • scenario:iastaspects-translate_aspect
  • scenario:iastaspects-translate_noaspect
  • scenario:iastaspects-upper_noaspect
  • scenario:packagespackageforrootmodulemapping-cache_off
  • scenario:packagespackageforrootmodulemapping-cache_on
  • scenario:sethttpmeta-all-enabled
  • scenario:telemetryaddmetric-record-100-metrics

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.

0 participants