Skip to content

Conversation

@pulpdrew
Copy link
Contributor

@pulpdrew pulpdrew commented Dec 12, 2025

Closes HDX-3057

This PR updates the Request Error Rate query config so that it can correctly leverage materialized views.

@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2025

🦋 Changeset detected

Latest commit: 45a1fc8

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

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api 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

@vercel
Copy link

vercel bot commented Dec 12, 2025

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

Project Deployment Review Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview, Comment Dec 15, 2025 2:32pm

@claude
Copy link

claude bot commented Dec 12, 2025

Code Review

✅ No critical issues found.

Changes are solid:

  • Correctly refactors error rate calculation to enable materialized view optimization
  • Splits countIf() / count() into separate aggregations (total_requests, error_requests) then divides
  • Properly uses aggCondition with aggConditionLanguage: sql for the error condition
  • Correctly hides intermediate series (hiddenSeries prop) to only show final error_rate
  • Consistent pattern applied in both HTTP and Errors tabs
  • Follows existing codebase patterns for aggFn: count usage

The approach matches ClickHouse MV best practices by separating aggregations from calculations.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 12, 2025

E2E Test Results

All tests passed • 46 passed • 3 skipped • 254s

Status Count
✅ Passed 46
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 3

View full report →

@pulpdrew pulpdrew force-pushed the drew/fix-request-error-rate-config branch from 28c6fc4 to 32556e6 Compare December 12, 2025 17:39
@pulpdrew pulpdrew changed the title fix: Update Request Error Rate config to use MVs fix: Update Services Dashboard count() queries to correctly use MVs Dec 12, 2025
@pulpdrew pulpdrew requested review from a team and knudtty and removed request for a team December 12, 2025 18:06
Copy link
Contributor

@knudtty knudtty left a comment

Choose a reason for hiding this comment

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

approve with 1 nit

alias: 'total_requests',
},
{
valueExpression: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is valueExpression needed here anymore? Could it just be left out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unfortunately required to satisfy the current types. We could potentially introduce an additional union item here to support an undefined valueExpression for aggFn: 'count'

Screenshot 2025-12-15 at 9 28 42 AM

@kodiakhq kodiakhq bot merged commit 19b710f into main Dec 15, 2025
9 checks passed
@kodiakhq kodiakhq bot deleted the drew/fix-request-error-rate-config branch December 15, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants