Skip to content

[HDX-5951] Start check-alerts CronJob through bin/hyperdx - #303

Merged
wrn14897 merged 1 commit into
mainfrom
warren/revisit-instrumentation-config
Nov 9, 2026
Merged

wrn14897 merged 1 commit into
mainfrom
warren/revisit-instrumentation-config

Conversation

@wrn14897

@wrn14897 wrn14897 commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

The check-alerts CronJob started its container with node /app/packages/api/build/tasks/index.js check-alerts. That bypasses bin/hyperdx, the entrypoint the image itself uses in entry.prod.sh, so the task never loaded a self-telemetry preload and OTEL_SERVICE_NAME=hdx-oss-task-check-alerts was dead config.

ClickHouse/ClickStack#3575 swaps the self-instrumentation SDK for @clickhouse/clickstack-node, and the preload path moves with it. Mirroring DeploySentinel/hyperdx-operator#314, the CronJob now runs /app/packages/api/bin/hyperdx task check-alerts so the image picks its own preload and the chart stops caring where it lives.

Linear: HDX-5951

Changes

  • templates/hyperdx/cronjob-check-alerts.yaml: the command switch now has three branches, keyed on the image tag:
    • < 2.7.0: node /app/packages/api/tasks/index check-alerts (unchanged)
    • >= 2.7.0, < 2.22.0: node /app/packages/api/build/tasks/index.js check-alerts (unchanged)
    • >= 2.22.0 or non-semver tag (2-beta, latest): /app/packages/api/bin/hyperdx task check-alerts (new; default appVersion 2.39.1 takes this path)
    • additionalArgs passthrough is preserved in every branch.
  • tests/task-checkAlerts_test.yaml: 2-beta and 3.0.0 cases expect the new command; added boundary tests for 2.21.9, 2.22.0, the default tag, and additionalArgs on the new path.
  • Changeset (patch).

Compatibility

bin/hyperdx first ships in HyperDX 2.22.0 (ClickHouse/ClickStack#1951) and is copied into every stage of the all-in-one image. It execs node so PID 1 and signal handling are unchanged. Post-#3575 the preload only starts the SDK when HYPERDX_API_KEY or OTEL_EXPORTER_OTLP_HEADERS is set; the chart sets HYPERDX_API_KEY by default, so the task starts exporting to the chart's collector like the API pod already does.

Validation

  • make test: 286/286
  • make coverage: 99.85% line, 91.19% branch
  • helm template --set hyperdx.tasks.enabled=true --set hyperdx.tasks.checkAlerts.additionalArgs.concurrency=4 renders ["/app/packages/api/bin/hyperdx", "task", "check-alerts", "--concurrency", "4"]

…5951)

On HyperDX images >= 2.22.0 (and non-semver tags) the check-alerts
CronJob now runs /app/packages/api/bin/hyperdx task check-alerts, so the
image picks its own self-telemetry preload. This mirrors the operator
fix in DeploySentinel/hyperdx-operator#314 and is needed once
ClickHouse/ClickStack#3575 (ClickStack Node SDK) ships. Older semver tags
keep their previous commands.
@wrn14897
wrn14897 requested a review from a team as a code owner November 9, 2026 19:28
@changeset-bot

changeset-bot Bot commented Nov 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6719eac

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

This PR includes changesets to release 1 package
Name Type
helm-charts 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 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Helm template coverage

Metric Value
Line coverage 99.85%
Branch coverage 91.19%
Line threshold 99% (met)
Branch threshold 90% (met)
Uncovered details

charts/clickstack/templates/NOTES.txt

Uncovered lines: none

Uncovered branches: 37:default:fallback

charts/clickstack/templates/_helpers.tpl

Uncovered lines: 17

Uncovered branches: 111:or(1):true, 128:range:empty, 159:default:fallback, 160:or(1):true, 16:if:true, 176:if:false

charts/clickstack/templates/additional-manifests.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/clickhouse/cluster.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/clickhouse/keeper.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/hyperdx/configmap.yaml

Uncovered lines: none

Uncovered branches: 8:range:empty

charts/clickstack/templates/hyperdx/cronjob-check-alerts.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/hyperdx/dashboard-configmap.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/hyperdx/dashboard-provisioner-rbac.yaml

Uncovered lines: none

Uncovered branches: 40:range:empty

charts/clickstack/templates/hyperdx/deployment.yaml

Uncovered lines: none

Uncovered branches: 117:default:fallback, 127:default:fallback, 150:if:false, 169:and(2):false, 16:with:empty, 192:default:fallback, 197:with:empty, 22:default:fallback, 38:with:empty

charts/clickstack/templates/hyperdx/hpa.yaml

Uncovered lines: none

Uncovered branches: 16:with:empty

charts/clickstack/templates/hyperdx/ingress.yaml

Uncovered lines: none

Uncovered branches: 35:if:false, 40:range:empty, 45:range:empty, 48:with:empty, 54:range:empty, 58:range:empty, 59:or(2):true, 59:or(3):true

charts/clickstack/templates/hyperdx/networkpolicy.yaml

Uncovered lines: none

Uncovered branches: 11:with:empty

charts/clickstack/templates/hyperdx/pdb.yaml

Uncovered lines: none

Uncovered branches: 10:with:empty

charts/clickstack/templates/hyperdx/secret.yaml

Uncovered lines: none

Uncovered branches: 13:range:empty, 1:or(2):true, 1:or(3):true

charts/clickstack/templates/hyperdx/service.yaml

Uncovered lines: none

Uncovered branches: 14:default:fallback, 9:with:empty

charts/clickstack/templates/hyperdx/serviceaccount.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/mongodb/community.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/mongodb/password-secret.yaml

Uncovered lines: none

Uncovered branches: none

charts/clickstack/templates/otel-collector/custom-config.yaml

Uncovered lines: none

Uncovered branches: none

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: charts/clickstack/templates/hyperdx/cronjob-check-alerts.yaml, its helm-unittest suite, and a patch changeset — making the check-alerts CronJob command image-tag-aware so images ≥ 2.22.0 launch through /app/packages/api/bin/hyperdx.

The rendering logic is sound. The three-way conditional, the $semver := regexMatch guard, and the semverCompare boundaries are correct, and Go-template and short-circuiting on modern Helm (Go ≥ 1.18) means non-semver tags route to the bin/hyperdx branch without evaluating semverCompare. All three branches preserve the additionalArgs passthrough, and the new boundary tests (2.21.9, 2.22.0, default appVersion, additionalArgs) lock in the branch selection. No P0/P1 issues.

✅ No critical issues found.

🟡 P2 -- recommended

  • charts/clickstack/templates/hyperdx/cronjob-check-alerts.yaml:44 -- the identical range $key, $value := .Values.hyperdx.tasks.checkAlerts.additionalArgs block is copied verbatim into all three branches (lines 44-47, 53-56, 63-66), so a future change to arg quoting or formatting can drift between branches.
    • Fix: Hoist the per-branch command prefix into a variable and emit a single command: block with one additionalArgs loop after the conditional.
🔵 P3 nitpicks (4)
  • charts/clickstack/templates/hyperdx/cronjob-check-alerts.yaml:58 -- the # bin/hyperdx loads the image's own self-telemetry preload line is a literal YAML comment that renders into the output manifest, while the other two branches dropped their inline rationale comments, leaving the treatment inconsistent.
    • Fix: Convert it to a non-rendering template comment {{- /* ... */ -}} (or apply the same comment style to the other branches).
  • charts/clickstack/tests/task-checkAlerts_test.yaml:295 -- the pre-esbuild (2.6.0) additionalArgs case asserts only via contains, so it would still pass if the base command or arg ordering regressed, unlike branches 2 and 3 which use full-array equal.
    • Fix: Assert the full command array with equal, e.g. ["node", "/app/packages/api/tasks/index", "check-alerts", "--concurrency", "4"].
  • charts/clickstack/tests/task-checkAlerts_test.yaml:228 -- no test covers a fully non-numeric tag such as latest; the non-semver else branch is already exercised by 2-beta and the default appVersion, but latest is a distinct input class the change is intended to handle.
    • Fix: Add a tag: "latest" case asserting the command equals ["/app/packages/api/bin/hyperdx", "task", "check-alerts"] to pin the short-circuit routing.
    • testing, correctness, project-standards
  • charts/clickstack/templates/hyperdx/cronjob-check-alerts.yaml:45 -- additionalArgs keys/values are emitted with literal double-quotes (- "{{ $value }}") rather than | quote, so a value containing a quote or YAML-significant character would produce invalid YAML; pre-existing pattern now duplicated into the new branch.
    • Fix: Render as - {{ printf "--%s" $key | quote }} and - {{ $value | quote }}.

Reviewers (4): correctness, testing, maintainability, project-standards.

Testing gaps: No test pins prerelease-suffixed semver tags (e.g. 2.22.0-rc.1 routes to branch 2, 2.7.0-beta to branch 1) that match the regex prefix but compare below their base version.

Pre-existing (not introduced by this diff): A semver-shaped-but-invalid tag like 1.2.3.4 passes the regex guard and reaches semverCompare, which errors and fails rendering on all Helm versions — unchanged from the pre-PR code.

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.

@wrn14897
wrn14897 merged commit 789c949 into main Nov 9, 2026
9 checks passed
@wrn14897
wrn14897 deleted the warren/revisit-instrumentation-config branch November 9, 2026 19:54
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.

2 participants