feat(java): dashboard settings, overrides, metrics, ops#388
Open
pratyush618 wants to merge 2 commits into
Open
feat(java): dashboard settings, overrides, metrics, ops#388pratyush618 wants to merge 2 commits into
pratyush618 wants to merge 2 commits into
Conversation
Adds the settings KV API (reserved auth:/webhooks: prefixes, size caps), per-task/queue overrides with live queue pause, aggregated /api/metrics + /api/metrics/timeseries (raw rows replaced), and circuit-breakers/event-types/scaler endpoints plus /readiness and Prometheus /metrics probes (metrics-token gated). Task/queue listings are derived from observable state since the SDK has no registry.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds dashboard ops, metrics, settings, and overrides APIs in Java, wires them into ChangesDashboard Ops/Metrics/Settings/Overrides
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DashboardServer
participant OpsHandlers
Client->>DashboardServer: GET /readiness
DashboardServer->>DashboardServer: metricsTokenOk(request)
DashboardServer->>OpsHandlers: readiness()
OpsHandlers-->>DashboardServer: readiness JSON
DashboardServer-->>Client: 200 JSON
Client->>DashboardServer: GET /metrics
DashboardServer->>DashboardServer: metricsTokenOk(request)
DashboardServer->>OpsHandlers: prometheus()
OpsHandlers-->>DashboardServer: Prometheus text
DashboardServer-->>Client: 200 text/plain
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OpsHandlers.java (1)
64-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReadiness check doesn't actually check anything.
storageis hardcodedtrueandstatusis always"ready"; the endpoint never derives its result from a real health signal, so it can't detect an actual outage (e.g., unreachable settings/storage backend). This could mask real problems for anything consuming/readinessfor traffic/orchestration decisions.Consider performing a lightweight probe (e.g., a settings read) before reporting
storage: true, or clarify if this is an intentional stub for this phase.🤖 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 `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OpsHandlers.java` around lines 64 - 72, The readiness endpoint in OpsHandlers.readiness is hardcoded to report healthy state, so replace the static storage=true and always-ready status with a real lightweight health probe before building the response. Use an actual settings/storage check (or another minimal backend read) to determine the storage check result, and derive the top-level status from the collected checks instead of always returning "ready". If this is meant to remain a stub, make that explicit in the method name or response so callers do not treat it as a real readiness signal.sdks/java/src/main/java/org/byteveda/taskito/dashboard/store/OverridesStore.java (1)
117-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
rate_limitvalidation only checks for a/character.Values like
"/"or"abc/def"pass this check but are meaningless as a rate limit, potentially breaking whatever downstream code parsescount/period.♻️ Proposed tightening
case "rate_limit" -> { - if (!(value instanceof String s) || s.isEmpty() || !s.contains("/")) { + if (!(value instanceof String s) || !s.matches("\\d+/\\d+[a-zA-Z]*")) { throw DashboardError.badRequest("rate_limit must look like '<count>/<period>'"); } }🤖 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 `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/store/OverridesStore.java` around lines 117 - 121, The rate_limit validation in OverridesStore currently only checks for a non-empty string containing “/”, so invalid values like “/” or “abc/def” still pass. Tighten the validation in the rate_limit case to require a well-formed count/period value with both sides present and meaningful (for example, non-empty numeric count and non-empty period), using the same DashboardError.badRequest path when the format is invalid.sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardOpsTest.java (1)
102-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for the metrics-token-configured case.
Given the shared-gate concern raised in
DashboardServer.java(readiness incorrectly requiring the metrics bearer token), consider adding a test that setsTASKITO_DASHBOARD_METRICS_TOKENand verifies/readinessand/healthbehavior, to lock in the intended contract once fixed.🤖 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 `@sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardOpsTest.java` around lines 102 - 114, Add a test case in DashboardOpsTest to cover the metrics-token-configured behavior: set TASKITO_DASHBOARD_METRICS_TOKEN, start DashboardServer via DashboardServer.start, and verify that /health and /readiness still return the expected ok/ready responses while /metrics remains protected by the bearer token. Use the existing probes in probesAreServed and the same server/queue setup to lock in the intended contract around DashboardServer and its readiness/metrics gating.
🤖 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
`@sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/MetricsHandlers.java`:
- Around line 21-34: The query parameter parsing in MetricsHandlers.longParam is
not validating malformed numeric input, so invalid since/bucket values can
surface as uncaught NumberFormatException. Update aggregated and timeseries to
parse these params through a guarded path that catches format errors and returns
DashboardError.badRequest, matching the other handlers in this PR. Use longParam
as the locating symbol, and ensure both since and bucket report a clean 400
instead of bubbling up a 500.
In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OpsHandlers.java`:
- Around line 42-62: The scaler method in OpsHandlers currently parses the
target query parameter with an unguarded Long.parseLong, so invalid input can
throw NumberFormatException and return a 500. Update scaler(Map<String, String>)
to use the same safe parsing pattern as MetricsHandlers.longParam: validate the
target param, handle parse failures gracefully, and return a client error
response instead of letting the exception escape.
In
`@sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OverridesHandlers.java`:
- Around line 62-72: The live queue update in putQueueOverride is using the raw
patch body to decide whether to pause or resume, so clearing the paused override
with null never resumes the queue. Update this method to base the side effect on
the effective paused value returned by store.putQueue (or equivalent computed
row value) instead of body.get("paused"), and use queue.queue(name).resume()
whenever the resolved override is false. Keep the persistence logic and the
live-state synchronization aligned in OverridesHandlers.putQueueOverride.
---
Nitpick comments:
In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OpsHandlers.java`:
- Around line 64-72: The readiness endpoint in OpsHandlers.readiness is
hardcoded to report healthy state, so replace the static storage=true and
always-ready status with a real lightweight health probe before building the
response. Use an actual settings/storage check (or another minimal backend read)
to determine the storage check result, and derive the top-level status from the
collected checks instead of always returning "ready". If this is meant to remain
a stub, make that explicit in the method name or response so callers do not
treat it as a real readiness signal.
In
`@sdks/java/src/main/java/org/byteveda/taskito/dashboard/store/OverridesStore.java`:
- Around line 117-121: The rate_limit validation in OverridesStore currently
only checks for a non-empty string containing “/”, so invalid values like “/” or
“abc/def” still pass. Tighten the validation in the rate_limit case to require a
well-formed count/period value with both sides present and meaningful (for
example, non-empty numeric count and non-empty period), using the same
DashboardError.badRequest path when the format is invalid.
In
`@sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardOpsTest.java`:
- Around line 102-114: Add a test case in DashboardOpsTest to cover the
metrics-token-configured behavior: set TASKITO_DASHBOARD_METRICS_TOKEN, start
DashboardServer via DashboardServer.start, and verify that /health and
/readiness still return the expected ok/ready responses while /metrics remains
protected by the bearer token. Use the existing probes in probesAreServed and
the same server/queue setup to lock in the intended contract around
DashboardServer and its readiness/metrics gating.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f629ca58-e24c-455c-9db4-591d746039a9
📒 Files selected for processing (15)
sdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/Contract.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/CoreHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/Metrics.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/MetricsHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OpsHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/OverridesHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/SettingsHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/store/OverridesStore.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardClient.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardOpsTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/InMemorySettings.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/api/SettingsHandlersTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/AuthStoreTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/store/OverridesStoreTest.java
💤 Files with no reviewable changes (1)
- sdks/java/src/main/java/org/byteveda/taskito/dashboard/api/CoreHandlers.java
Addresses PR review: malformed numeric query params (since/limit/offset/target/bucket) now return 400 via a shared guarded parser instead of an uncaught NumberFormatException (500); clearing a queue's paused override now resumes the live queue (reconcile from the resulting row, not the request value).
Collaborator
Author
|
Addressed all review comments in 19b385e:
Resolving the threads. |
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.
Builds on the session-auth foundation to bring the read/write and platform surface to parity. Everything rides the settings KV store — no schema changes.
Endpoints
GET/PUT/DELETE /api/settings[/<key>], with reservedauth:/webhooks:prefixes hidden, a 256-char key / 64 KiB value cap, and control-char rejection.GET/PUT/DELETE /api/{tasks,queues}/<name>/override(rate-limit, concurrency, retries, timeout, priority, paused), validated and merge-on-write; setting a queue'spausedalso pauses/resumes it live./api/tasksand/api/queueslist the known names./api/metricsnow returns the aggregated per-task summary the SPA expects (count, success/failure, avg, p50/p95/p99, min/max) instead of raw rows, plus/api/metrics/timeseries. Percentiles are computed in pure Java (nearest-rank)./api/circuit-breakers./api/event-types./api/scaler(queue depth, live workers, capacity, per-queue)./health(public),/readinessand a Prometheus/metricsexposition, both gated byTASKITO_DASHBOARD_METRICS_TOKEN.WorkerInfonow also carriesthreadsin the contract (feeds the scaler capacity math).Tests
Settings API, overrides store, and an ops integration suite covering metrics/scaler/probes. Full Java suite + checkstyle + Spotless clean.
Summary by CodeRabbit
New Features
Bug Fixes