feat(analytics): carry subject and tags through custom-metric CRUD - #2401
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 43b16584397b378f14dd8bb7e4e30f063d850992 and ddeb42f. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCustom metrics now support optional ChangesCustom metric metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalyticsService
participant MariaDB
Client->>AnalyticsService: Create custom metric with subject and tags
AnalyticsService->>AnalyticsService: Validate subject and tags
AnalyticsService->>MariaDB: Persist metric definition and ordered tags
Client->>AnalyticsService: Fetch custom metric
AnalyticsService->>MariaDB: Read definition and tags
MariaDB-->>AnalyticsService: Return subject and tags
AnalyticsService-->>Client: Return reconstructed custom metric
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/backend/services/analytics/src/domain/metric_crud.rs (2)
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove documentation comments from service code.
metric_crud.rsis service code. Preserve these API descriptions in the schema-generation input instead of///comments.As per coding guidelines, “Use
///documentation comments only on exported items in shared library crates” and “Do not add documentation comments to binaries or services.”Also applies to: 82-85, 103-106
🤖 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/backend/services/analytics/src/domain/metric_crud.rs` around lines 58 - 61, Remove the `///` documentation comments associated with the `subject` field and the additional referenced fields in `metric_crud.rs`. Preserve their API descriptions in the schema-generation input, leaving the field definitions and serde attributes unchanged.Source: Coding guidelines
1129-1150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven validation cases and test the tag limit.
The new tests do not execute the
graph.tags.len() > MAX_TAGSbranch. Add a case with 65 tags. Put the reject and accept cases in table-driven loops with per-case assertion messages.As per coding guidelines, “Make tests read as specifications: use table-driven loops with per-case assertion messages such as
"should reject: {input:?}".”🤖 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/backend/services/analytics/src/domain/metric_crud.rs` around lines 1129 - 1150, Update the tests around rejects_malformed_subject_and_tags and accepts_absent_subject_and_empty_tags to use table-driven cases and per-case assertion messages such as “should reject: {input:?}” or “should accept: {input:?}”. Include a rejection case whose tags contain 65 entries so the graph.tags.len() > MAX_TAGS branch is exercised, while preserving coverage for malformed subjects, invalid tags, duplicate tags, absent subjects, and empty tags.Source: Coding guidelines
🤖 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 `@docs/components/backend/analytics/openapi.json`:
- Around line 213-219: The CustomMetric.tags schema must match the Rust DTO’s
Vec<String> with serde default: update the source schema metadata to define an
empty-array default, then regenerate
docs/components/backend/analytics/openapi.json at lines 213-219 and
tests/stand/api/schemas/analytics.py at lines 783-784 so tags is a required
non-optional list defaulting to [] and rejects null.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_crud.rs`:
- Around line 58-61: Remove the `///` documentation comments associated with the
`subject` field and the additional referenced fields in `metric_crud.rs`.
Preserve their API descriptions in the schema-generation input, leaving the
field definitions and serde attributes unchanged.
- Around line 1129-1150: Update the tests around
rejects_malformed_subject_and_tags and accepts_absent_subject_and_empty_tags to
use table-driven cases and per-case assertion messages such as “should reject:
{input:?}” or “should accept: {input:?}”. Include a rejection case whose tags
contain 65 entries so the graph.tags.len() > MAX_TAGS branch is exercised, while
preserving coverage for malformed subjects, invalid tags, duplicate tags, absent
subjects, and empty tags.
🪄 Autofix
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 Plus
Run ID: b335faf3-caeb-41ff-ba64-8a0329441585
📥 Commits
Reviewing files that changed from the base of the PR and between 29e3c8b and 43b16584397b378f14dd8bb7e4e30f063d850992.
📒 Files selected for processing (4)
docs/components/backend/analytics/openapi.jsonsrc/backend/services/analytics/src/domain/metric_crud.rssrc/backend/services/analytics/src/domain/metric_crud_live_tests.rstests/stand/api/schemas/analytics.py
| "tags": { | ||
| "description": "Cross-cutting filter labels; lowercase snake-case slugs, unique per\nmetric. Optional — defaults to empty.", | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "type": "array" | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked candidate files ---'
git ls-files | rg '(^|/)(analytics\.py|openapi\.json|.*\.rs)$' | head -200
printf '%s\n' '--- OpenAPI excerpt ---'
cat -n docs/components/backend/analytics/openapi.json | sed -n '190,235p'
printf '%s\n' '--- Pydantic excerpt ---'
cat -n tests/stand/api/schemas/analytics.py | sed -n '755,805p'
printf '%s\n' '--- tags references ---'
rg -n -C 3 '"tags"|tags:' --glob '*.rs' --glob '*.py' --glob '*.json' . | head -300Repository: constructorfabric/insight
Length of output: 42704
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Rust custom metric model ---'
cat -n src/backend/services/analytics/src/domain/metric_crud.rs | sed -n '35,105p'
printf '%s\n' '--- All analytics tags schema occurrences ---'
rg -n -C 5 'tags' docs/components/backend/analytics/openapi.json tests/stand/api/schemas/analytics.py src/backend/services/analytics/src --glob '*.json' --glob '*.py' --glob '*.rs' | sed -n '1,500p'
printf '%s\n' '--- OpenAPI schema names and tags properties ---'
python3 - <<'PY'
import json
from pathlib import Path
p = Path("docs/components/backend/analytics/openapi.json")
doc = json.loads(p.read_text())
for name, schema in doc.get("components", {}).get("schemas", {}).items():
props = schema.get("properties", {})
if "tags" in props:
print(name, "required=", "tags" in schema.get("required", []),
"tags=", json.dumps(props["tags"], ensure_ascii=False, sort_keys=True))
PY
printf '%s\n' '--- Stand schema classes containing tags ---'
python3 - <<'PY'
from pathlib import Path
lines = Path("tests/stand/api/schemas/analytics.py").read_text().splitlines()
for i, line in enumerate(lines):
if "tags" in line:
start = max(0, i - 12)
end = min(len(lines), i + 13)
print(f"--- lines {start+1}-{end} ---")
for n in range(start, end):
print(f"{n+1}: {lines[n]}")
PYRepository: constructorfabric/insight
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CustomMetric request/response uses ---'
rg -n -C 4 'CustomMetric|custom metric|metrics/import|Json<|from_slice|serde_json' \
src/backend/services/analytics/src/api \
src/backend/services/analytics/src/domain/metric_crud.rs \
tests/stand --glob '*.rs' --glob '*.py' | head -400
printf '%s\n' '--- Stand schema generation and validation references ---'
rg -n -C 3 'tests/stand/api/schemas|datamodel|openapi|schema' \
Makefile* pyproject.toml package.json .github scripts tools tests/stand \
--glob '*.py' --glob '*.toml' --glob '*.json' --glob '*.yaml' --glob '*.yml' --glob 'Makefile*' 2>/dev/null | head -400
printf '%s\n' '--- Relevant OpenAPI defaults and required fields ---'
python3 - <<'PY'
import json
from pathlib import Path
doc = json.loads(Path("docs/components/backend/analytics/openapi.json").read_text())
for name, schema in doc.get("components", {}).get("schemas", {}).items():
props = schema.get("properties", {})
if "tags" in props:
print(name)
print(" required:", schema.get("required", []))
print(" tags default:", props["tags"].get("default", "<absent>"))
print(" tags type:", props["tags"].get("type"))
print(" tags nullable:", props["tags"].get("nullable", "<absent>"))
PY
printf '%s\n' '--- Exact API handler excerpts ---'
rg -l 'CustomMetric' src/backend/services/analytics/src/api | while read -r f; do
echo "--- $f ---"
rg -n -C 8 'CustomMetric|Json<' "$f" | head -160
doneRepository: constructorfabric/insight
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- OpenAPI metadata ---'
python3 - <<'PY'
import json
from pathlib import Path
doc = json.loads(Path("docs/components/backend/analytics/openapi.json").read_text())
for name, schema in doc["components"]["schemas"].items():
if "tags" in schema.get("properties", {}):
prop = schema["properties"]["tags"]
print(name, {
"required": "tags" in schema.get("required", []),
"type": prop.get("type"),
"default": prop.get("default", "<absent>"),
"nullable": prop.get("nullable", "<absent>"),
})
PY
printf '%s\n' '--- Schema generator ---'
cat -n tests/generate_schemas.py | sed -n '1,220p'
printf '%s\n' '--- Existing generated collection defaults ---'
rg -n -C 2 'list\[.*\].*(default_factory|Field|= \[\])|default_factory=list|default=\[\]' tests/stand/api/schemas --glob '*.py' | head -160
printf '%s\n' '--- Analytics handler request signatures ---'
rg -n -C 10 'Json<|Request<|CustomMetric|ImportCustomMetricsRequest' \
src/backend/services/analytics/src/api/metrics.rs \
src/backend/services/analytics/src/api/mod.rs \
src/backend/services/analytics/src/api/*.rs | head -260Repository: constructorfabric/insight
Length of output: 38809
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Rust schema default annotations ---'
rg -n -C 3 '#\[schema\([^]]*default|default\s*=' src/backend --glob '*.rs' | head -240
printf '%s\n' '--- OpenAPI default keys ---'
rg -n -C 2 '"default"' docs/components/backend --glob '*.json' | head -240
printf '%s\n' '--- Rust DTO array fields with serde defaults ---'
rg -n -C 3 '#\[serde\(default[^]]*\)\][[:space:]]*$|pub [A-Za-z0-9_]+: Vec<' \
src/backend/services/analytics/src --glob '*.rs' | head -300Repository: constructorfabric/insight
Length of output: 30405
Align CustomMetric.tags with the Rust DTO.
tags is Vec<String> with #[serde(default)]. Omitted tags becomes []; JSON null is rejected. Add an empty-array default to the source schema metadata, then regenerate docs/components/backend/analytics/openapi.json and tests/stand/api/schemas/analytics.py so tags is a non-optional list with an empty-list default.
📍 Affects 2 files
docs/components/backend/analytics/openapi.json#L213-L219(this comment)tests/stand/api/schemas/analytics.py#L783-L784
🤖 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 `@docs/components/backend/analytics/openapi.json` around lines 213 - 219, The
CustomMetric.tags schema must match the Rust DTO’s Vec<String> with serde
default: update the source schema metadata to define an empty-array default,
then regenerate docs/components/backend/analytics/openapi.json at lines 213-219
and tests/stand/api/schemas/analytics.py at lines 783-784 so tags is a required
non-optional list defaulting to [] and rejects null.
| @@ -779,6 +780,8 @@ class CustomMetric(BaseModel): | |||
| scale: float | None = None | |||
| short_label: str | None = None | |||
| source_key: str | |||
| subject: str | None = Field(None, description='The single topic this metric groups under within its family; a\nlowercase snake-case slug. Optional for custom metrics.') | |||
There was a problem hiding this comment.
Can we add or extend test for this endpoints with new fields?
43b1658 to
31acfa7
Compare
31acfa7 to
b921247
Compare
Builtin metrics gained subject/tags in the registry, but the custom-metric CRUD could neither accept nor return them, so tenant-authored metrics were stuck with a NULL subject and no tags — unreachable from the API. Thread both through the whole custom-metric path: - DTO: `subject` (optional slug) and `tags` (slug list) on CustomMetric, plus `subject` on CustomMetricSummary so the management list groups like the definitions listing. - Validation: subject shape, tag shape + per-metric uniqueness, MAX_TAGS bound. - Create/import: subject column on the metric_definitions insert; a tags insert loop mirroring dimensions. - Read/export: subject on the definition read and summary; fetch_tag_keys; into_graph carries subject + tags, so export/import round-trips them. Regenerated openapi.json and the stand schema; added validation and round-trip tests. Frontend display of subject/tags is a follow-up. Refs constructorfabric#2344 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Assert the custom-metric authoring path persists and returns the new fields, against a deployed stand: - create → read → list → update round-trips `subject` and `tags`, with distinct values per stage so a handler that echoes the create body without persisting fails at the read, and the list summary carries the grouping subject a management screen groups by. Export is left to the Rust into_graph test (the stand export path is xfail on a fresh stand per constructorfabric#2360). - the invalid-graph 400 sweep gains a malformed-subject and a malformed-tag case, both rejected by validate_graph ahead of the observation-SQL probe. `custom_metric_body` gains optional subject/tags kwargs; the default body is unchanged so existing callers keep the minimal valid graph. Refs constructorfabric#2344 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
b921247 to
ddeb42f
Compare
2 participants
Summary
#2397 gave builtin metrics
subject(grouping partition) andtags(cross-cutting filters) via the registry. This PR closes the matching gap for custom (tenant-authored) metrics: the CRUD could neither accept nor return them, so a custom metric was stuck withsubject = NULLand no tags, unreachable from the API.Both are now threaded through the full custom-metric path (
domain/metric_crud.rs):subject(optional slug) andtags(slug list) onCustomMetric;subjectonCustomMetricSummaryso the management list can group by topic like the definitions listing.MAX_TAGSbound — mirroring the existing measures/dimensions rules.subjecton themetric_definitionsinsert; ametric_definition_tagsinsert loop mirroring the dimensions loop.subjecton the definition read and summary; newfetch_tag_keys;into_graphcarries subject + tags, so export/import round-trips them.Not included (follow-up)
insight-frontside) — will be a separate change on this branch or its own PR.Testing
cargo test -p analytics→ 416 passed (incl. newrejects_malformed_subject_and_tags,accepts_absent_subject_and_empty_tags, and round-trip assertions in the ignored live tests), 0 failed.cargo clippy -p analytics --testsclean.openapi.json(drift-checked) and the standanalytics.pyschema.🤖 Generated with Claude Code
Summary by CodeRabbit