feat(analytics): group metric definitions by subject, add tags - #2397
Merged
cyberantonz merged 2 commits intoAug 11, 2026
Merged
Conversation
|
Warning Review limit reached
Next review available in: 6 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: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughMetric definitions now include nullable subjects and ordered tags. Migrations create the storage schema, registry seeds validate and persist metadata, listings return it, and OpenAPI and test schemas expose the new fields. ChangesMetric metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MetricSeed
participant MetricDatabase
participant MetricRepository
participant MetricDefinitionView
MetricSeed->>MetricDatabase: upsert subject and replace tags
MetricRepository->>MetricDatabase: fetch subject and ordered tags
MetricRepository->>MetricDefinitionView: map metadata into listing response
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 |
Metric definitions carried no grouping attribute, so a surface listing a whole family could only sort its metrics by name. Add a singular `subject` (the one topic a metric belongs to within its family) and a many-to-many `tags` set (cross-cutting filter labels), both authored in the builtin registry and exposed on GET /v1/metric-definitions. `subject` is a partition: exactly one per metric, required for every builtin and pinned by a registry invariant test, so it cannot be derived from a source (a metric's inputs may span sources) and no new metric silently falls into an "other" bucket. `tags` sit alongside for filtering and search. - schema: `subject` column on metric_definitions; new metric_definition_tags table with shape/order CHECKs wired into the startup probe. - registry: subject on all builtins; tags where they cross subjects (rate/duration/distribution). Reconciler seeds both. - listing: subject and tags on the wire view; repository fetch_tags. - contract/docs: regenerated openapi.json and stand schema; DESIGN storage model and authoring guidance; e2e collector projects both fields. Closes constructorfabric#2344 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
force-pushed
the
fix/2344-metric-subject-tags
branch
from
August 10, 2026 12:07
7a5c857 to
2905a67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/backend/services/analytics/src/migration/m20260810_000001_metric_definition_subject.rs (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
Debugfor both migration types.
src/backend/services/analytics/src/migration/m20260810_000001_metric_definition_subject.rs#L15-L16: AddDebugto theMigrationderives.src/backend/services/analytics/src/migration/m20260810_000002_metric_definition_tags.rs#L26-L27: AddDebugto theMigrationderives.As per coding guidelines, “Derive
Debugfor types.”🤖 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/migration/m20260810_000001_metric_definition_subject.rs` around lines 15 - 16, Add Debug to the derive attributes for the Migration type in both src/backend/services/analytics/src/migration/m20260810_000001_metric_definition_subject.rs lines 15-16 and src/backend/services/analytics/src/migration/m20260810_000002_metric_definition_tags.rs lines 26-27, preserving the existing DeriveMigrationName derive.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 `@src/backend/services/analytics/src/domain/metric_definitions/builtin.rs`:
- Around line 102-109: The builtin metric metadata must use parsed bounded slugs
instead of raw strings. In builtin.rs lines 102-109, introduce and use
MetricSubject and MetricTag boundary types enforcing lowercase snake case and a
maximum of 64 characters; update lines 325-354 to test the length limit while
preserving per-metric tag uniqueness validation. In
docs/domain/metrics/specs/DESIGN.md lines 663-669, document the 64-character
maximum in the metric-authoring guidance.
In `@src/backend/services/analytics/src/domain/metric_definitions/seeds.rs`:
- Around line 219-245: The replace_tags function must perform deletion and all
tag inserts within a single database transaction so failures cannot leave
partial results and concurrent reconcilers cannot interleave operations. Start a
transaction from db, execute the delete and insert loop through the transaction
handle, commit only after every operation succeeds, and roll back or propagate
errors on failure while preserving idempotent, race-safe replacement behavior.
---
Nitpick comments:
In
`@src/backend/services/analytics/src/migration/m20260810_000001_metric_definition_subject.rs`:
- Around line 15-16: Add Debug to the derive attributes for the Migration type
in both
src/backend/services/analytics/src/migration/m20260810_000001_metric_definition_subject.rs
lines 15-16 and
src/backend/services/analytics/src/migration/m20260810_000002_metric_definition_tags.rs
lines 26-27, preserving the existing DeriveMigrationName derive.
🪄 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: 3fc1e8f8-2be2-48bd-8f5d-17ccafcd47d8
📒 Files selected for processing (12)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/specs/DESIGN.mdsrc/backend/services/analytics/src/domain/metric_definitions/builtin.rssrc/backend/services/analytics/src/domain/metric_definitions/listing.rssrc/backend/services/analytics/src/domain/metric_definitions/registry.yamlsrc/backend/services/analytics/src/domain/metric_definitions/repository.rssrc/backend/services/analytics/src/domain/metric_definitions/seeds.rssrc/backend/services/analytics/src/migration/m20260810_000001_metric_definition_subject.rssrc/backend/services/analytics/src/migration/m20260810_000002_metric_definition_tags.rssrc/backend/services/analytics/src/migration/mod.rssrc/ingestion/tests/e2e/lib/collect_metric_definitions.pytests/stand/api/schemas/analytics.py
Comment on lines
+102
to
+109
| /// The single topic this metric belongs to within its family, so a surface | ||
| /// listing a family can partition it into topics. Required for builtins — | ||
| /// exactly one per metric, which is the partition a source key cannot give. | ||
| pub subject: String, | ||
| /// Cross-cutting labels a surface can filter or search by; many per metric, | ||
| /// unlike the singular `subject`. | ||
| #[serde(default)] | ||
| pub tags: Vec<String>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Parse bounded metadata slugs at the registry boundary.
subject and tags remain raw strings. The current tests accept values longer than the VARCHAR(64) columns. A future registry entry can pass cargo test and then fail or truncate during reconciliation.
src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L102-L109: Replace raw metadata strings with parsedMetricSubjectandMetricTagboundary types that enforce lowercase snake case and a 64-character maximum.src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L325-L354: Test the maximum length and retain per-metric tag uniqueness validation.docs/domain/metrics/specs/DESIGN.md#L663-L669: State the 64-character maximum in the metric-authoring instructions.
As per coding guidelines, “Parse, don't validate: introduce boundary newtypes … and do not carry raw String values through layers.”
📍 Affects 2 files
src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L102-L109(this comment)src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L325-L354docs/domain/metrics/specs/DESIGN.md#L663-L669
🤖 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_definitions/builtin.rs`
around lines 102 - 109, The builtin metric metadata must use parsed bounded
slugs instead of raw strings. In builtin.rs lines 102-109, introduce and use
MetricSubject and MetricTag boundary types enforcing lowercase snake case and a
maximum of 64 characters; update lines 325-354 to test the length limit while
preserving per-metric tag uniqueness validation. In
docs/domain/metrics/specs/DESIGN.md lines 663-669, document the 64-character
maximum in the metric-authoring guidance.
Source: Coding guidelines
Comment on lines
+219
to
+245
| async fn replace_tags( | ||
| db: &DatabaseConnection, | ||
| metric_id: Uuid, | ||
| tags: &[String], | ||
| ) -> Result<(), DbErr> { | ||
| db.execute(Statement::from_sql_and_values( | ||
| db.get_database_backend(), | ||
| "DELETE FROM metric_definition_tags WHERE metric_definition_id = ?", | ||
| [uuid_value(metric_id)], | ||
| )) | ||
| .await?; | ||
|
|
||
| for (idx, tag) in tags.iter().enumerate() { | ||
| db.execute(Statement::from_sql_and_values( | ||
| db.get_database_backend(), | ||
| "INSERT INTO metric_definition_tags \ | ||
| (id, metric_definition_id, tag, display_order) \ | ||
| VALUES (?, ?, ?, ?)", | ||
| [ | ||
| uuid_value(Uuid::now_v7()), | ||
| uuid_value(metric_id), | ||
| Value::from(tag.as_str()), | ||
| Value::from(order_value(idx)), | ||
| ], | ||
| )) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make tag replacement atomic.
The DELETE on Line 224 commits before the insert loop completes. If an insert fails, the metric keeps no tags or only a prefix of its tags. Concurrent reconcilers can also interleave deletes and inserts.
Reconcile tags in one transaction. Upsert desired tags before deleting obsolete tags, or otherwise serialize this replacement.
As per docs/domain/metrics/specs/DESIGN.md, builtin upserts must be idempotent and race-safe across replicas.
🤖 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_definitions/seeds.rs` around
lines 219 - 245, The replace_tags function must perform deletion and all tag
inserts within a single database transaction so failures cannot leave partial
results and concurrent reconcilers cannot interleave operations. Start a
transaction from db, execute the delete and insert loop through the transaction
handle, commit only after every operation succeeds, and roll back or propagate
errors on failure while preserving idempotent, race-safe replacement behavior.
…ct/tag length Addresses review feedback on the subject/tags reconcile: - Wrap each metric's definition upsert and its input/dimension/tag replacement in one transaction. A mid-way failure no longer leaves a metric with a partial child set, and a concurrent reconciler on another replica observes the whole prior set or the whole new one rather than a delete-in-progress — the idempotent, race-safe convergence DESIGN requires. The per-metric write helpers now take any connection so they run inside the transaction. - Bound subject and tag length to the VARCHAR(64) column width in the builtin registry invariant tests, so an over-long value fails the build instead of erroring or truncating at reconcile time. Refs constructorfabric#2344 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
aleksdotbar
approved these changes
Aug 11, 2026
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
2 participants
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.
Summary
Closes #2344.
Metric definitions carried no grouping attribute, so a surface listing a whole family could only sort its metrics by name. This adds two authored attributes, both exposed on
GET /v1/metric-definitions:subject— the single topic a metric belongs to within its family (meetings,messaging,email,documents,commits,pull_requests, …). This is the partition the issue asked for: exactly one per metric, required for every builtin and pinned by a registry invariant test. It deliberately is not derived from the source — a metric's numerator and denominator inputs may come from different sources, so a source-based grouping is not a partition — and being required means no new metric can silently fall into an "other" bucket.tags— a many-to-many set of cross-cutting filter/search labels (rate,duration,distribution), distinct from the singular grouping key.Changes
subjectcolumn onmetric_definitions; newmetric_definition_tagstable (shape + display-order CHECKs, wired into the startup probe). Two forward migrations.subjecton all builtin metrics (18 topics);tagswhere they cross subjects. Reconciler seedssubjectand areplace_tagsstep.subjectis a requiredMetricSeedfield, so a missing one fails the build; new invariant tests pin subject/tag shape and per-metric tag uniqueness.subjectandtagson the wire view;repository::fetch_tags.openapi.jsonand the standanalytics.pyschema via their canonical generators (drift-checked); DESIGN storage model + authoring guidance; e2e collector projects both fields.Custom-metric definitions store
NULL/no tags (the column is nullable); wiring subject into the custom-metric authoring flow is intentionally out of scope.Testing
cargo test -p analytics→ 414 passed (incl. the two new registry invariants), 0 failed.cargo clippy -p analytics --testsclean.🤖 Generated with Claude Code
Summary by CodeRabbit