feat: add optimization settings to all queries if available

[knudtty]

Adds several optimizations that helps immensely with search query duration. On our own data we've seen a 2-3x improvement, for extremely large log tables we've seen a 50x improvement

Closes HDX-3429

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
hyperdx-v2-oss-app	Ready	Preview, Comment	Feb 20, 2026 7:27pm

[changeset-bot]

🦋 Changeset detected

Latest commit: 03bab25

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

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/common-utils	Minor
@hyperdx/api	Minor
@hyperdx/app	Minor
@hyperdx/otel-collector	Minor

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

[claude]

PR Review: feat: add optimization settings to all queries if available

Approach looks solid overall — the caching pattern, recursion guard, and test coverage are well done. A few issues to address:

---

• ❌ Non-permissions errors propagate and break queries → In metadata.ts getSettings, only 'Not enough privileges' is caught; all other errors (network timeouts, other ClickHouse auth errors, etc.) are re-thrown. Since processClickhouseSettings has no catch around getSettings, a transient ClickHouse error when fetching system.settings will fail the user's original query. This is a regression. Catch all errors (or at least all Error instances) and fall back to returning an empty Map with a warning.

• ⚠️ Fragile error string matching → e.message.includes('Not enough privileges') will silently miss other permission-related errors (e.g. ACCESS_DENIED, different ClickHouse versions/locales). Broaden the catch if the intent is "any error means skip optimization settings".

• ⚠️ Fetches all system.settings to check 7 specific keys → The query SELECT name, value FROM system.settings returns hundreds of rows. Use WHERE name IN ('query_plan_optimize_lazy_materialization', 'use_skip_indexes_for_top_k', ...) to limit results and reduce overhead on every cold-cache startup.

• ℹ️ node.ts doesn't pass connectionId to processClickhouseSettings → browser.ts passes it but node.ts doesn't (cache key becomes "undefined.availableSettings"). Harmless since the node client connects to one server, but worth making consistent.

---

The most important fix before merge is #1 — the error propagation will cause query failures in any environment where system.settings is temporarily inaccessible.

[github-actions]

PR Review: feat: add optimization settings to all queries if available

The approach is sound overall. A few items to consider:

• ⚠️ shouldSkipApplySettings in public QueryInputs interface → This is an internal recursion-guard leaking into the public API. Callers could accidentally pass it. Consider making it internal (e.g., a private overload or a separate internal method) rather than exposing it on the shared type.

• ⚠️ No TTL on settings cache → MetadataCache caches results indefinitely. If ClickHouse is upgraded mid-runtime with new settings, they won't be picked up until restart. Consider documenting this limitation or adding a staleness window.

• ⚠️ applySettingIfAvailable enables settings regardless of server's current value → It checks serverSettings.has(name) (existence) but applies hardcoded values (always '1', '100000'). If a user has explicitly configured these settings to different values in their CH config, this will silently override them. Worth a comment clarifying the intent is version detection, not value-copying.

• ℹ️ SELECT name, value FROM system.settings fetches all settings (~1000+ rows) on first query → Adds latency to the very first request. Not a bug since it's cached, but worth noting the cold-start cost.

✅ Recursion prevention via shouldSkipApplySettings: true in getSettings is correct. Caching deduplication in MetadataCache handles concurrent first-calls properly. Error handling (silent fallback on permissions errors, retry on transient errors) looks correct.

[karl-power]

I have the same concern as Claude. Shouldn't this be if (!shouldSkipApplySettings)?

[knudtty]

Yes, I uncovered that locally when adding tests too!

[github-actions]

PR Review: feat: add optimization settings to all queries if available

Approach is solid — caching pattern, recursion guard via shouldSkipApplySettings, and test coverage are all well done.

• ❌ Non-permissions errors propagate and break queries → In metadata.ts getSettings, only 'Not enough privileges' is caught; all other errors (transient network issues, other ClickHouse errors) are re-thrown. Since processClickhouseSettings has no catch around getSettings, a transient error fetching system.settings will fail the user's original query. Catch all errors and return empty Map with a warning instead.

• ⚠️ Fragile permission error string matching → e.message.includes('Not enough privileges') will miss other permission-related errors (e.g. ACCESS_DENIED). Broaden the catch to all errors if the intent is "any failure → skip optimization settings".

• ⚠️ Fetches all ~1000 system.settings rows to check 7 keys → Add WHERE name IN ('query_plan_optimize_lazy_materialization', ...) to limit the query.

• ℹ️ shouldSkipApplySettings is an internal guard exposed in the public QueryInputs interface → Callers could accidentally pass it. Consider keeping it internal or documenting it as internal-only.

The most critical fix before merge is #1 — any environment where system.settings is temporarily inaccessible will cause all queries to fail.

[github-actions]

E2E Test Results

✅ All tests passed • 71 passed • 4 skipped • 805s

Status	Count
✅ Passed	71
❌ Failed	0
⚠️ Flaky	0
⏭️ Skipped	4

Tests ran across 4 shards in parallel.

View full report →