feat: custom metric units display and formatting - #790
Open
123FLO321 wants to merge 35 commits into
Open
Conversation
…superpowers Shell scripts checked out with CRLF on Windows (core.autocrlf=true) broke the Docker entrypoint at container start. Force LF for all text files at the git layer, matching the existing .editorconfig and Prettier settings.
Whitespace-only line-wrap fixes found by prettier --check during Task 7 verification; no logic changes.
…helper, docs notes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds configurable metric names, units, and decimal formatting across monitor storage, APIs, management screens, charts, embeds, and badges. It also adds Vitest browser/server projects, tests, CI execution, documentation, and translation support. ChangesConfigurable metric display
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MonitorAPI
participant MonitorOverview
participant StatusBarCalendar
participant LatencyTrendChart
MonitorAPI->>MonitorOverview: monitor data and valueDisplay
MonitorOverview->>StatusBarCalendar: status data and valueDisplay
MonitorOverview->>LatencyTrendChart: chart data and valueDisplay
StatusBarCalendar-->>MonitorOverview: formatted tooltip values
LatencyTrendChart-->>MonitorOverview: formatted trend values
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| src/lib/server/db/repositories/monitoring.ts | Adds non-null reading counts to monitor aggregate queries. |
| src/lib/server/tool.ts | Updates value aggregation to use custom unit formatting and reading-count gates. |
| src/lib/clientTools.ts | Adds shared helpers for custom unit detection, reading detection, and value formatting. |
| src/lib/components/StatusBarCalendar.svelte | Updates calendar tooltips and day detail wiring for custom metric display. |
| src/lib/components/MonitorOverview.svelte | Updates overview labels, chart data, and summary values for custom metric display. |
| src/lib/components/MonitorDayDetail.svelte | Updates day detail chart labels and filters for custom metric values. |
| src/lib/server/api-server/monitor-bar/shared.ts | Passes metric display settings into monitor bar response aggregation. |
| src/lib/server/api-server/monitor-day-latency/post.ts | Updates minute-level metric filtering and formatting for custom units. |
Reviews (4): Last reviewed commit: "style: fix indents" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 9-11: Update the workflow containing the test job to explicitly
set the default GITHUB_TOKEN permissions to read-only, preferably with a
top-level permissions block granting contents: read; preserve the existing
jobs.test configuration and avoid granting broader permissions.
In `@src/routes/`(embed)/embed/monitor-[tag]/+page.svelte:
- Around line 80-86: Extract the trimmed value-display name into a derived
variable named customName alongside the existing derived declarations,
defaulting to an empty string when unavailable. Update the template condition
and translated label to use customName instead of repeating
overviewData?.valueDisplay?.name?.trim(), while preserving the “Avg Latency”
fallback.
In
`@src/routes/`(manage)/manage/app/monitors/[tag]/components/ModifyDataCard.svelte:
- Around line 22-23: Remove the duplicated displayName and unitSuffix
derivations from ModifyDataCard.svelte and reuse the shared derivation
introduced by the consolidated implementation, preserving the existing fallback
values and valueDisplay behavior.
In
`@src/routes/`(manage)/manage/app/monitors/[tag]/components/MonitorRecentLogs.svelte:
- Around line 23-24: Remove the duplicate displayName and unitSuffix derivations
from MonitorRecentLogs.svelte and reuse the existing shared derivation or
consolidated implementation established with ModifyDataCard.svelte, preserving
the current Latency and ms defaults.
- Around line 25-26: Update the recent-log latency rendering to use
FormatValue(log.latency, valueDisplay) instead of appending valueSuffix
directly, while preserving the existing valueDisplay configuration and
surrounding log layout.
In
`@src/routes/`(manage)/manage/app/monitors/[tag]/components/MonitorTypeCard.svelte:
- Around line 46-55: Update the testResultLabel derivation in MonitorTypeCard to
recognize the unit suffix case-insensitively, so any capitalization of “ms”
renders as “milliseconds” while preserving custom-unit text unchanged. Align
this display logic with the existing IsCustomUnit behavior used by
clientTools.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5f64751-f944-4804-8e2c-f82a25f63386
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (46)
.gitattributes.github/workflows/test.yml.gitignoreCLAUDE.mdmigrations/20250111153517_init.tspackage.jsonsrc/lib/clientTools.test.tssrc/lib/clientTools.tssrc/lib/components/BadgesMenu.sveltesrc/lib/components/CopyButton.svelte.test.tssrc/lib/components/EmbedMenu.sveltesrc/lib/components/LatencyTrendChart.sveltesrc/lib/components/MonitorBar.sveltesrc/lib/components/MonitorDayDetail.sveltesrc/lib/components/MonitorOverview.sveltesrc/lib/components/StatusBarCalendar.sveltesrc/lib/components/StatusBarCalendar.svelte.test.tssrc/lib/components/ThemePlus.sveltesrc/lib/locales/en.jsonsrc/lib/server/api-server/monitor-bar/get.tssrc/lib/server/api-server/monitor-bar/shared.tssrc/lib/server/api-server/monitor-day-latency/post.tssrc/lib/server/controllers/monitorsController.tssrc/lib/server/tool.test.tssrc/lib/server/tool.tssrc/lib/server/types/db.tssrc/lib/server/types/monitor.tssrc/lib/stores/i18n.test.tssrc/lib/stores/i18n.tssrc/routes/(api)/api/v4/monitors/[monitor_tag]/data/+server.tssrc/routes/(api)/api/v4/monitors/[monitor_tag]/data/[timestamp]/+server.tssrc/routes/(docs)/docs/content/v4/monitors.mdsrc/routes/(embed)/embed/latency-[tag]/+page.sveltesrc/routes/(embed)/embed/monitor-[tag]/+page.sveltesrc/routes/(kener)/monitors/[monitor_tag]/+page.server.tssrc/routes/(kener)/monitors/[monitor_tag]/+page.sveltesrc/routes/(manage)/manage/app/monitoring-data/+page.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/+page.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/MetricDisplayCard.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/MetricDisplayCard.svelte.test.tssrc/routes/(manage)/manage/app/monitors/[tag]/components/ModifyDataCard.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/MonitorRecentLogs.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/MonitorTypeCard.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/types/monitor-group.sveltevite.config.tsvitest-setup-client.ts
Day-level aggregates for custom-unit monitors treated buckets whose readings were all NULL as real 0 samples: SQL AVG/MIN/MAX correctly ignore per-row NULLs, but an all-NULL bucket collapses to a NULL aggregate that the JS mapping coerced to 0, and the custom-unit gate (DayHasData) only checked status counts, which stayed true. Carry a non-NULL latency reading count (latencyCount, via COUNT(latency)) through the aggregate pipeline and gate on it instead (DayHasReading), matching how the minute-level endpoint already excludes no-reading samples.
123FLO321
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(kener)/monitors/[monitor_tag]/+page.server.ts (1)
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify assignment logic using optional chaining and nullish coalescing.
While functionally correct (since a falsy
0latency safely falls back to0), relying on number truthiness checks can be slightly confusing. You can clean this up and make it more idiomatic by using the?.and??operators.♻️ Proposed refactor
- countOfUp: lastStatus && lastStatus.status === GC.UP ? 1 : 0, - countOfDown: lastStatus && lastStatus.status === GC.DOWN ? 1 : 0, - countOfDegraded: lastStatus && lastStatus.status === GC.DEGRADED ? 1 : 0, - countOfMaintenance: lastStatus && lastStatus.status === GC.MAINTENANCE ? 1 : 0, - avgLatency: lastStatus && lastStatus.latency ? lastStatus.latency : 0, - maxLatency: lastStatus && lastStatus.latency ? lastStatus.latency : 0, - minLatency: lastStatus && lastStatus.latency ? lastStatus.latency : 0, - latencyCount: lastStatus && lastStatus.latency !== null ? 1 : 0, + countOfUp: lastStatus?.status === GC.UP ? 1 : 0, + countOfDown: lastStatus?.status === GC.DOWN ? 1 : 0, + countOfDegraded: lastStatus?.status === GC.DEGRADED ? 1 : 0, + countOfMaintenance: lastStatus?.status === GC.MAINTENANCE ? 1 : 0, + avgLatency: lastStatus?.latency ?? 0, + maxLatency: lastStatus?.latency ?? 0, + minLatency: lastStatus?.latency ?? 0, + latencyCount: lastStatus?.latency != null ? 1 : 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(kener)/monitors/[monitor_tag]/+page.server.ts around lines 61 - 68, In the monitor status aggregation object, simplify the lastStatus property accessors using optional chaining and nullish coalescing: update countOfUp, countOfDown, countOfDegraded, countOfMaintenance, and the latency fields so absent values default to 0 without truthiness checks, while preserving latencyCount’s null-handling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/routes/`(kener)/monitors/[monitor_tag]/+page.server.ts:
- Around line 61-68: In the monitor status aggregation object, simplify the
lastStatus property accessors using optional chaining and nullish coalescing:
update countOfUp, countOfDown, countOfDegraded, countOfMaintenance, and the
latency fields so absent values default to 0 without truthiness checks, while
preserving latencyCount’s null-handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd42256b-8dc1-4792-8a28-74aac6caf84b
📒 Files selected for processing (21)
.github/workflows/test.ymlsrc/lib/clientTools.tssrc/lib/components/MonitorOverview.sveltesrc/lib/components/StatusBarCalendar.sveltesrc/lib/components/StatusBarCalendar.svelte.test.tssrc/lib/server/api-server/monitor-bar/shared.tssrc/lib/server/api-server/monitor-bars/get.tssrc/lib/server/api-server/monitor-day-status/post.tssrc/lib/server/controllers/dashboardController.tssrc/lib/server/db/repositories/monitoring.tssrc/lib/server/tool.test.tssrc/lib/server/tool.tssrc/lib/server/types/db.tssrc/routes/(docs)/docs/content/v4/monitors.mdsrc/routes/(embed)/embed/latency-[tag]/+page.sveltesrc/routes/(embed)/embed/monitor-[tag]/+page.sveltesrc/routes/(kener)/monitors/[monitor_tag]/+page.server.tssrc/routes/(manage)/manage/app/monitoring-data/+page.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/ModifyDataCard.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/MonitorRecentLogs.sveltesrc/routes/(manage)/manage/app/monitors/[tag]/components/MonitorTypeCard.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Custom metric units
Monitors aren't always about latency (e.g. a worker queue length).
You can now configure per monitor how its value is displayed.
items,%,ppl, …),nonefor a bare number; empty keeps the defaultmswith the usual ms -> s/m/h auto-scalingDisplay-only: values still arrive as
latencythrough the existing paths (API monitor eval,PATCH /api/v4/monitors/[tag]/data), nothing changes in storage or the public API. Existing monitors render exactly as before. For custom units,0is a valid reading instead of "no data".Applied everywhere the value shows up: status page, charts/tooltips, day detail, badges, embeds, admin tables.
For next major (potentially)
latency→value(DB column,MonitoringResult, v4 API)requires #789
Summary by CodeRabbit
%namevariables resolve more consistently.