Skip to content

fix: use toJSONString for metric attribute hashing with JSON schema (#2087)#2118

Open
doyong365 wants to merge 1 commit intohyperdxio:mainfrom
doyong365:fix/json-schema-metric-mapconcat
Open

fix: use toJSONString for metric attribute hashing with JSON schema (#2087)#2118
doyong365 wants to merge 1 commit intohyperdxio:mainfrom
doyong365:fix/json-schema-metric-mapconcat

Conversation

@doyong365
Copy link
Copy Markdown

Summary

When the ClickHouse OTel exporter is configured with json: true (BETA_CH_OTEL_JSON_SCHEMA_ENABLED), attribute columns (Attributes, ScopeAttributes, ResourceAttributes) are stored as JSON type instead of Map(String, String). The existing metric bucketing queries always used cityHash64(mapConcat(...)), which fails with:

Function mapConcat requires at least one argument of type Map

This fix detects the column type at query time via metadata.getColumns() and substitutes mapConcat with toJSONString-based hashing when JSON schema is active. The detection falls back to the original Map behaviour if the column lookup fails (e.g. table not yet created). Affected query paths: Gauge, Sum, and Histogram (count + quantile) metrics.

Screenshots or video

N/A — backend query generation change only, no UI changes.

How to test locally

  1. Enable JSON schema mode by setting BETA_CH_OTEL_JSON_SCHEMA_ENABLED=true in your ClickHouse exporter config (json: true).
  2. Send metric data via the OTel Collector to populate otel_metrics_gauge, otel_metrics_sum, and otel_metrics_histogram tables.
  3. Open the Metrics Explorer and chart any Gauge, Sum, or Histogram metric.
  4. Verify the chart renders without the Function mapConcat requires at least one argument of type Map error.
  5. Without the fix, the same steps produce the error above.

References

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 15, 2026

⚠️ No Changeset found

Latest commit: 18c427d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 15, 2026

@doyong365 is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Copy Markdown
Contributor

PR Review

✅ No critical issues found.

The implementation is correct and well-tested. A few minor observations:

  • ⚠️ Silent catch {} in schema detection → Add at minimum a console.warn or logger call so detection failures aren't invisible during debugging; the current bare catch swallows programming errors too, not just "table doesn't exist" cases.
  • ℹ️ Only Attributes column is checked to determine JSON schema, then ScopeAttributes/ResourceAttributes are also assumed to be JSON — this is a valid simplification since the OTel exporter sets all three together, but worth a comment.
  • getColumns() uses this.cache.getOrFetch() with a per-table key, so the new detection call does not add repeated ClickHouse round-trips per chart render — no performance concern.
  • ✅ Good test coverage: 4 new snapshot tests for all metric types (gauge, sum, histogram quantile, histogram count) plus a regression test verifying Map schema is unchanged.

@doyong365 doyong365 force-pushed the fix/json-schema-metric-mapconcat branch from 89fb034 to 18c427d Compare April 15, 2026 04:05
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.

[Feature Request] JSON schema support for Kubernetes Dashboard and Metrics (Currently beta)

1 participant