Skip to content

Conversation

@knudtty
Copy link
Contributor

@knudtty knudtty commented Oct 16, 2025

@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2025

🦋 Changeset detected

Latest commit: 637a4a4

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

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

@knudtty knudtty marked this pull request as ready for review October 16, 2025 15:56
@vercel
Copy link

vercel bot commented Oct 16, 2025

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

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 27, 2025 5:41pm

@claude
Copy link

claude bot commented Oct 16, 2025

PR Review

Overall Assessment: Good fix for preventing ClickHouse query timeouts

Code Quality Issues:

  • Logging inaccuracy in util.ts:79 - numWindows value in second log message is stale (calculated before first truncation). Consider recalculating or removing from second log.

  • Minor: Variable oneDayMs in tests (line 344) is defined but never used - Remove unused variable.

Positive Notes:

  • Comprehensive test coverage (10 test cases)
  • Clear documentation in changeset
  • Proper error prevention logic
  • Good use of logging for observability

Recommendation: Address the logging accuracy issue before merge to avoid confusion during debugging.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2025

E2E Test Results

All tests passed • 25 passed • 3 skipped • 238s

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

View full report →

dhable
dhable previously approved these changes Oct 16, 2025
Copy link
Contributor

@dhable dhable left a comment

Choose a reason for hiding this comment

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

+1 with one minor nitpick

@knudtty
Copy link
Contributor Author

knudtty commented Oct 16, 2025

@claude remove the unused variable that the reviewer Dan mentioned

@claude
Copy link

claude bot commented Oct 16, 2025

Code Review

No critical issues found.

The PR implements time range truncation for alert queries to prevent excessive ClickHouse load. The implementation is solid with:

  • ✅ Comprehensive test coverage (189 new test lines)
  • ✅ Proper logging for debugging truncation events
  • ✅ Defensive limits (max 50 windows, 6h/24h lookback based on window size)
  • ✅ Correct integration into the alert checking flow

Minor observations (non-blocking):

  • Magic numbers: Consider exporting MAX_NUM_WINDOWS, maxLookbackTime constants for potential reuse (util.ts:44-46)
  • Calculation precision: Uses integer arithmetic correctly, no floating-point issues
  • Edge case handling: Zero time range and fractional windows properly tested

Testing recommendation: Verify alert behavior with historical data that would trigger truncation to ensure no unexpected side effects in production.


Review generated by Claude Code

Co-authored-by: Aaron Knudtson <[email protected]>
@kodiakhq kodiakhq bot merged commit 778092d into main Oct 27, 2025
8 of 9 checks passed
@kodiakhq kodiakhq bot deleted the aaron/limit-long-alert-timeranges branch October 27, 2025 17:40
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