Skip to content

Use the repo's <host> placeholder for a measurement location in four files - #3020

Merged
erikdarlingdata merged 5 commits into
devfrom
chore/changelog-synthetic-host
Sep 5, 2026
Merged

erikdarlingdata merged 5 commits into
devfrom
chore/changelog-synthetic-host

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Four places named a specific monitored host as the location of a measurement. All four now use <host>, the placeholder spelling this repo already uses.

location was
CHANGELOG.md (#2759 entry) measurement location
PerformanceMonitor.Collectors/QueryStoreOpenIntervalState.cs:68 RefreshEvery doc comment, describing that same #2759 finding
PerformanceMonitor.Collectors/PlanCorrectionCollector.cs:151 doc comment, host and database
Lite.Tests/PlanCorrectionCollectorDefinitionTests.cs:190 test comment, same host and database

The two carrying a database qualifier use <host>/<database>.

No behaviour change — every edit is inside a comment or a changelog line. The measurements, dates, windows and findings all still read the same; only the location is now a placeholder.

<host> is the established spelling: Darling/README.md:148 (http://<host>:5153), Darling/README.md:1151 (psql -h <host>), and CHANGELOG.md:265. An earlier revision of this PR used <target-host>, which appeared nowhere else in the tree and would have introduced a second divergent spelling.

Scope note

The first revision changed only the changelog line. Review pointed out that the same string was live in three source files on the base branch, which made the change a partial one — the identical token would have remained in shipped source, including the doc comment on the very setting the changelog entry is about. Widened rather than split, since splitting would have left the observable half in place while presenting the work as done.

One other hostname-shaped token elsewhere in the changelog carries an allowlisted Greek-letter slug: synthetic, and left alone.

Comment thread CHANGELOG.md Outdated
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Reviewed. This is a one-line, docs-only change to CHANGELOG.md (no T-SQL, no app code, so the T-SQL style and Lite/Darling parity checks don't apply here).

The change itself is good practice — replacing a real-looking monitored host name in a changelog entry with a placeholder avoids leaking fleet-internal naming into a public file, and the entry's substance (measurement, dates, window, finding) is untouched.

One nit, left as an inline comment: the PR description claims <target-host> matches an existing convention in this file and in README examples, but <target-host> doesn't actually appear anywhere else in the repo. The placeholder this codebase already uses (e.g. Darling/README.md:148/1151, and this same CHANGELOG's http://<host>:5153 line) is <host>. Suggest swapping to <host> so this doesn't introduce a second, divergent placeholder spelling. Not blocking.

No correctness, security, or performance concerns — this doesn't touch executable code.

Comment thread CHANGELOG.md
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Reviewed. This PR is a documentation-only change (CHANGELOG.md, 1 line) — no SQL, no Lite/Darling app code, so the T-SQL style conventions and Lite/Darling parity checks don't apply here.

One finding posted inline: the hostname scrub is incomplete. prod-pos-use1-multi-45 is removed from the CHANGELOG entry but the identical string is still present in three source files already on the base branch (QueryStoreOpenIntervalState.cs, PlanCorrectionCollector.cs, PlanCorrectionCollectorDefinitionTests.cs) — including a doc comment on the exact setting (RefreshEvery) this changelog entry describes. If the goal is to stop naming a specific host, this PR should scrub all four occurrences, not just the CHANGELOG one.

No other issues — the placeholder choice (<host>, matching existing usage in Darling/README.md) is reasonable and consistent with the rest of the file.

@erikdarlingdata erikdarlingdata changed the title CHANGELOG: use a synthetic host name in the #2759 entry Use the repo's <host> placeholder for a measurement location in four files Sep 5, 2026
Comment thread PerformanceMonitor.Collectors/QueryStoreOpenIntervalState.cs
Comment thread PerformanceMonitor.Collectors/PlanCorrectionCollector.cs
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Reviewed. This is a docs/comment-only change (host-name redaction), no functional/SQL logic touched — no Lite/Darling parity concerns, no security surface, no perf impact.

One correctness issue found and flagged inline in both PerformanceMonitor.Collectors/QueryStoreOpenIntervalState.cs and PerformanceMonitor.Collectors/PlanCorrectionCollector.cs: the new <host> / <database> placeholders land inside /// XML doc comments unescaped, producing malformed doc-XML (unclosed tags) rather than literal text. This codebase's own convention, used elsewhere in these same doc comments, is to escape literal angle brackets as &lt;...&gt; (e.g. PgSessionStatesCollector.cs's &lt;insufficient privilege&gt;). Suggested fixes posted on both lines.

The CHANGELOG.md and Lite.Tests/PlanCorrectionCollectorDefinitionTests.cs edits are fine as-is — the changelog is Markdown, and the test file's placeholder sits inside a plain /* */ block comment, not an XML doc comment, so no escaping is needed there.

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.

1 participant