Skip to content

fix(api): load connection passwords in the usage stats job - #3613

Merged
jordan-simonovski merged 1 commit into
mainfrom
jordansimonovski/usage-stats-connection-password
Nov 10, 2026
Merged

jordan-simonovski merged 1 commit into
mainfrom
jordansimonovski/usage-stats-connection-password

Conversation

@jordan-simonovski

Copy link
Copy Markdown
Contributor

The usage stats job loaded connections without their password (the field is select: false), so every run sent ClickHouse an empty password. Each password-protected connection logged AUTHENTICATION_FAILED every 4 hours, and its table sizes were left out of the report because the job swallows the error. The job now selects +password, as the alert checks already do.

Fixes #3612

The password field is select: false, so the job built ClickHouse clients
with no password and every run failed auth on password-protected
connections.

Fixes #3612
@jordan-simonovski
jordan-simonovski requested a review from a team as a code owner November 10, 2026 22:42
@vercel

vercel Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Nov 10, 2026 10:45pm UTC
hyperdx-storybook Ready Ready Preview Nov 10, 2026 10:45pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4fc4c77

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Nov 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Additional context: touches background tasks or the delivery pipeline lightly (2 lines, under the 30-line bar for Tier 4)

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 1
  • Production lines changed: 2
  • Branch: jordansimonovski/usage-stats-connection-password
  • Author: jordan-simonovski

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

const getClickhouseTableSize = async () => {
// fetch mongo data
const connections = await Connection.find();
const connections = await Connection.find().select('+password');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — No regression test that the usage stats job sends the connection password to ClickHouse

Add a case to packages/api/src/tasks/__tests__/usageStats.test.ts, which never mentions Connection or password today. It should create a Connection with a password, mock ClickhouseClient, and assert the constructor receives that password. The catch {} at usageStats.ts:220 swallows auth errors, so if someone drops .select('+password') again the job fails silently and nothing catches it.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@github-actions

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.


Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.20%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

The one-line change (Connection.find().select('+password')) correctly fixes the described bug and mirrors the established .select('+password') pattern already used throughout packages/api/src/tasks/checkAlerts/providers/default.ts. Security tracing confirms the now-loaded password is consumed only to construct the ClickhouseClient and is never serialized into the outbound usage-stats telemetry (the payload is built from chStats/chTables, not connection objects), and the ClickHouse query remains parameterized. The changeset's package, bump type, and body format match repo conventions.

🟡 P2 — recommended

  • packages/api/src/tasks/usageStats.ts:154 — The restored password-loading behavior has no regression test, even though the sibling job asserts exactly this (checkAlerts/providers/__tests__/default.int.test.ts checks conn.password is populated), so a select: false regression here would again fail silently.
    • Fix: Add a test that persists a password-protected Connection, exercises the load path, and asserts connection.password is non-empty before ClickhouseClient construction, exporting getClickhouseTableSize or spying on the client constructor args as needed.
    • testing, correctness, reliability
🔵 P3 nitpicks (1)
  • packages/api/src/tasks/usageStats.ts:220 — The pre-existing catch { // ignore } around the per-connection query still swallows all errors, so the report stays silently incomplete for any connection whose password is wrong or rotated even after this fix.
    • Fix: Log the caught error with the connection id so future per-connection query failures surface instead of silently dropping that connection's table sizes.

Reviewers (6): correctness, security, reliability, testing, maintainability, project-standards.

Testing gaps: getClickhouseTableSize/getUsageStats are unexported and untested — only sendUsageStats (the pure OTLP path) has coverage, so the Mongo-load and ClickHouse-query logic has no CI protection against a re-regression.

Note: the 2 prior PR comments are automated bot output (Vercel, changeset-bot) with no reviewer feedback to verify.

No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 464 passed • 1 skipped • 1474s

Status Count
✅ Passed 464
❌ Failed 0
⚠️ Flaky 2
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@jordan-simonovski
jordan-simonovski merged commit ef56bd7 into main Nov 10, 2026
32 checks passed
@jordan-simonovski
jordan-simonovski deleted the jordansimonovski/usage-stats-connection-password branch November 10, 2026 22:56

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — 4fc4c77b Deployed Nov 10, 2026 by vercel[bot]
Preview – hyperdx-storybook — 4fc4c77b Deployed Nov 10, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Usage-stats job loads connections without their password, so every run fails ClickHouse auth

2 participants