feat(metrics): add dimensional timeseries querying - #1836
Merged
Conversation
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between da7afad505890908510356cbb550ea002cc076cd and d286a86. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughGit metrics now expose additional dimensions and measures. Metric requests support validated per-metric dimension filters that flow through batching and SQL compilation. Period and timeseries results preserve missing values as null instead of fabricated zeros. ChangesAnalytics metric dimensions and filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MetricRequest
participant validate_request
participant plan_queries
participant metric_results_compiler
participant git_metric_observations
participant build_period_view
MetricRequest->>validate_request: dimension filters
validate_request->>plan_queries: ValidatedMetricRequest with filters
plan_queries->>metric_results_compiler: grouped filters
metric_results_compiler->>git_metric_observations: SQL predicates and bound values
git_metric_observations-->>metric_results_compiler: metric rows
metric_results_compiler-->>build_period_view: compiled results
build_period_view-->>MetricRequest: values or null
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
aleksdotbar
changed the title
aleksdotbar
force-pushed
the
feat/1781-weekly-git-output
branch
from
July 21, 2026 13:04
da7afad to
4b5bd29
Compare
aleksdotbar
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dev-compose.sh (1)
290-314: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPrevent duplicate YAML keys when combining flags.
If a user runs the compose script with both
--no-analyticsand--from-ghcr=api-gateway, the generatedoverride.generated.ymlwill declare theapi-gateway:key twice: once from theghcr_listloop and again from theno_analyticsblock.Docker Compose strictly rejects duplicate mapping keys and will fail to parse the file, preventing the stack from starting. To fix this, merge the
api-gatewayconfiguration directly inside theall_backendloop to ensure the service key is emitted only once.🐛 Proposed fix to merge YAML blocks
- if contains "$ghcr_list" "$svc"; then + if contains "$ghcr_list" "$svc" || { [[ "$svc" == "api-gateway" ]] && [[ "$no_analytics" == "true" ]]; }; then # Ghcr images are amd64-only for now (arm64 builds are # tracked separately). Pin the platform so Apple-silicon # hosts pull the amd64 manifest and run it under Rosetta # instead of erroring with "no matching manifest for # linux/arm64/v8". cat <<YML ${svc}: +YML + if contains "$ghcr_list" "$svc"; then + cat <<YML build: !reset null volumes: !override [] entrypoint: !reset null command: !reset null platform: linux/amd64 YML fi + if [[ "$svc" == "api-gateway" ]] && [[ "$no_analytics" == "true" ]]; then + cat <<YML + extra_hosts: + - "analytics:host-gateway" +YML + fi + fi done if [[ "$no_analytics" == "true" ]]; then cat <<'YML' analytics: profiles: ["analytics"] - api-gateway: - extra_hosts: - - "analytics:host-gateway" YML fi🤖 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 `@dev-compose.sh` around lines 290 - 314, Move the no-analytics api-gateway configuration into the existing all_backend/ghcr_list generation flow so that api-gateway is emitted under a single service key when both flags are used. Update the relevant loop and conditionals around the ghcr service generation, while preserving the analytics profile and api-gateway extra_hosts settings; remove the separate api-gateway block from the no_analytics section.
🧹 Nitpick comments (2)
src/backend/services/analytics/src/domain/metric_results/batch.rs (1)
60-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd unit tests for batch splitting by filters.
Including
metric.filtersin the grouping keys cleanly solves the issue of applying differing filters within the same batch.Consider adding a test case to
plan_queriesthat confirms views with identical sources but different filter configurations are correctly separated into distinctPlannedQuery::PeriodBatchorPlannedQuery::PeerBatchoutputs.🤖 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_results/batch.rs` around lines 60 - 81, Add unit coverage in plan_queries for batch grouping by filters, using views with the same source but different metric.filters. Assert they produce separate PlannedQuery::PeriodBatch or PlannedQuery::PeerBatch outputs, while identical filter configurations remain grouped.src/backend/services/analytics/src/domain/metric_results/compiler.rs (1)
546-560: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd unit tests for filter SQL compilation.
The new
dimension_filter_wherelogic successfully guards against emptyIN ()syntax and safely handles ClickHouse array boundaries, thanks to upstream validation and theindexOfpattern.However, since all updated tests in this file pass
&[]for the filters parameter, consider adding a unit test for one of the query compilation functions (ordimension_filter_wheredirectly) that passes one or moreValidatedDimensionFilterinstances. This will ensure that the filter SQL predicates and parameter lockstep are protected against future regressions.🤖 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_results/compiler.rs` around lines 546 - 560, Add a unit test covering non-empty dimension filters, either through dimension_filter_where or a query compilation function that invokes it. Construct one or more ValidatedDimensionFilter instances and assert the generated SQL contains the expected indexOf/IN predicates and that params contains the corresponding values in matching order.
🤖 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_results/validation.rs`:
- Around line 357-374: Update the filter-value construction in the validation
loop to collect trimmed, validated, deduplicated values in the existing
BTreeSet, then build ValidatedDimensionFilter.values directly from that set so
values are sorted deterministically and avoid the current duplicate vector
allocations. Preserve the existing empty-value and maximum-byte validation
behavior.
In `@src/ingestion/gold/git_metric_observations.sql`:
- Line 304: Update the git.commits_per_active_day metric explanation in
builtin.rs to explicitly document that active days are counted separately for
each repository due to the source_dimensions passed to presence_measure for
commit_day. Preserve the existing repository drill-down behavior and ensure the
explanation clarifies the resulting repository-day denominator.
---
Outside diff comments:
In `@dev-compose.sh`:
- Around line 290-314: Move the no-analytics api-gateway configuration into the
existing all_backend/ghcr_list generation flow so that api-gateway is emitted
under a single service key when both flags are used. Update the relevant loop
and conditionals around the ghcr service generation, while preserving the
analytics profile and api-gateway extra_hosts settings; remove the separate
api-gateway block from the no_analytics section.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_results/batch.rs`:
- Around line 60-81: Add unit coverage in plan_queries for batch grouping by
filters, using views with the same source but different metric.filters. Assert
they produce separate PlannedQuery::PeriodBatch or PlannedQuery::PeerBatch
outputs, while identical filter configurations remain grouped.
In `@src/backend/services/analytics/src/domain/metric_results/compiler.rs`:
- Around line 546-560: Add a unit test covering non-empty dimension filters,
either through dimension_filter_where or a query compilation function that
invokes it. Construct one or more ValidatedDimensionFilter instances and assert
the generated SQL contains the expected indexOf/IN predicates and that params
contains the corresponding values in matching order.
🪄 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: 69dcd1a9-7fb4-4529-b766-cca4aef79067
📥 Commits
Reviewing files that changed from the base of the PR and between ef86925 and da7afad505890908510356cbb550ea002cc076cd.
📒 Files selected for processing (11)
dev-compose.shsrc/backend/services/analytics/src/domain/metric_definitions/builtin.rssrc/backend/services/analytics/src/domain/metric_definitions/definition.rssrc/backend/services/analytics/src/domain/metric_results/batch.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/compiler.rssrc/backend/services/analytics/src/domain/metric_results/dto.rssrc/backend/services/analytics/src/domain/metric_results/validation.rssrc/ingestion/dbt/macros/metric_observation_measures.sqlsrc/ingestion/gold/git_metric_observations.sqlsrc/ingestion/gold/schema.yml
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
ktursunov
approved these changes
Jul 21, 2026
11 tasks
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.
Adds grouped timeseries queries and per-metric dimension filters with validation and batch-query support. Extends Git metrics with repository, project, source, category, file extension, change type, and destination branch dimensions; adds lines removed and preserves missing values as null.\n\nCloses #1781
Summary by CodeRabbit
New Features
Bug Fixes