feat(metrics): add metric drilldown CSV/XLSX export - #2074
Conversation
📝 WalkthroughWalkthroughThe PR adds ChangesMetric drilldown export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant export_metric_drilldown
participant EvidenceSnapshot
participant build_export
Client->>export_metric_drilldown: Submit metric drilldown export request
export_metric_drilldown->>EvidenceSnapshot: Validate selection and read bounded evidence rows
EvidenceSnapshot-->>export_metric_drilldown: Return rows and verify snapshot
export_metric_drilldown->>build_export: Build CSV or XLSX
build_export-->>export_metric_drilldown: Return bounded binary content
export_metric_drilldown-->>Client: Return attachment with filename
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
added
the
stack:metric-drilldown
a500819 to
f6d2173
Compare
5747583 to
9164c8b
Compare
f6d2173 to
8be69a3
Compare
9164c8b to
ba538a5
Compare
8be69a3 to
bc26311
Compare
1fd5dbc to
2cd99c1
Compare
030855f to
b984c30
Compare
2cd99c1 to
2d03e23
Compare
b984c30 to
d00e88f
Compare
ab1fb15 to
4c3ccc3
Compare
d00e88f to
5aed07b
Compare
4c3ccc3 to
974b295
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: 1
🧹 Nitpick comments (6)
src/backend/Cargo.toml (1)
70-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a newer
rust_xlsxwriterversion.
rust_xlsxwriter = "0.90"restricts Cargo to the0.90.xseries. Because this crate is pre-1.0, Cargo's caret matching treats the minor version as the compatibility boundary, so this pin cannot pick up the newer0.91–0.96releases without a manual version bump. The crate is at0.96.0as of this review.csv = "1.3"is less of a concern since1.xcaret matching already allows automatic upgrades up to the current1.4.0.Bump
rust_xlsxwriterto a current0.9xversion, or use a version range that covers the latest release, to pick up fixes made since0.90.Since crate version currency changes frequently, please confirm the current latest stable release of
rust_xlsxwriterbefore bumping.🤖 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/Cargo.toml` around lines 70 - 71, Update the rust_xlsxwriter dependency declaration from 0.90 to the latest stable 0.9x release confirmed from the current crate registry, so Cargo can receive fixes from newer releases while preserving the existing dependency configuration.src/backend/services/analytics/src/domain/metric_drilldown/mod.rs (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
pub(crate)for these domain re-exports.The targeted symbols are only needed within the analytics crate, so
pub(crate)restricts them from becoming part of the crate’s public API while keeping the current cross-module call sites working.🤖 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_drilldown/mod.rs` around lines 20 - 22, Change the metric_drilldown re-exports for MAX_EXPORT_BYTES, build_export, export_filename, export_internal, export_limit, build_response, presentation, validate_export_request, and validate_request from pub to pub(crate), preserving their existing module paths and cross-module availability.Source: Path instructions
src/backend/services/analytics/src/domain/metric_drilldown/export.rs (2)
89-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the blank-cell
Formatout of the per-cell loop.
Format::new()is constructed on everyNullcell inside the per-row, per-column loop.date_formatis already hoisted above the loop for the same purpose. Do the same for the blank format to avoid repeated allocation across up toMAX_EXPORT_ROWSrows and every column.♻️ Proposed fix
let date_format = Format::new().set_num_format("yyyy-mm-dd"); + let blank_format = Format::new(); for (column, header) in columns.iter().enumerate() { @@ match (column.r#type, value) { (_, serde_json::Value::Null) => worksheet - .write_blank(row_index, column_index, &Format::new()) + .write_blank(row_index, column_index, &blank_format) .map_err(|_| export_internal())?,Based on learnings, "Avoid allocations in per-row or per-item loops when values can be borrowed or hoisted" (
src/backend/**/*.rscoding guideline).🤖 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_drilldown/export.rs` around lines 89 - 91, Hoist a reusable blank-cell Format alongside date_format before the per-row/per-column loop, then pass a reference to it in the serde_json::Value::Null branch instead of constructing Format::new() for each cell. Preserve the existing write_blank behavior and error mapping.Source: Coding guidelines
64-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the underlying
XlsxErrorbefore returningexport_internal().Only the
save_to_writerfailure (lines 138-141) logs the originating error viatracing::warn!. Every other.map_err(|_| export_internal())?inbuild_xlsx(write_string, write_number, write_blank, write_datetime_with_format, add_table, theu16/u32conversions) discards theXlsxErrorsilently. If cell writing fails in production, there is no trace of why.Centralize this into one helper that logs then converts, and use it at every call site in this function.
♻️ Proposed fix
+fn xlsx_error(context: &str, error: rust_xlsxwriter::XlsxError) -> CanonicalError { + tracing::error!(error = %error, context, "metric drilldown XLSX cell write failed"); + export_internal() +}Then replace
.map_err(|_| export_internal())?calls inbuild_xlsxwith.map_err(|error| xlsx_error("write_string", error))?(with a context string per call site).As per coding guidelines, "centralize error construction in one helper per failure kind; log detailed errors internally while returning generic wire errors."
🤖 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_drilldown/export.rs` around lines 64 - 143, Update build_xlsx to centralize XLSX failure handling in an xlsx_error helper that logs the underlying error and returns export_internal(), then replace every relevant map_err(|_| export_internal()) in build_xlsx—including worksheet writes, add_table, and numeric conversions—with xlsx_error calls using a context string identifying each operation. Preserve the existing specialized save_to_writer handling unless the helper is appropriate for that failure path.Source: Coding guidelines
src/backend/services/analytics/src/api/metric_drilldown.rs (1)
143-158: 🩺 Stability & Availability | 🔵 Trivial
spawn_blockingis not cancelled bytimeout_at; the export permit can outlive the client-visible timeout.
tokio::task::spawn_blockingruns to completion once started; wrapping itsJoinHandleintokio::time::timeout_atonly stops the caller from waiting further, it does not abort the blocking closure. If the deadline elapses whilebuild_exportis still running, the client already received an "execution time limit" error, but_permitinside the still-running closure keeps occupying one of the two export concurrency slots until that closure finishes on its own.Given the upstream row/byte/cell caps bound the blocking work's size, this window is likely short in practice, but it means the
MAX_CONCURRENT_EXPORTScap is a soft bound in wall-clock terms rather than a hard one implied by the INVARIANT comment. Confirm this trade-off is accepted, or consider havingbuild_exportperiodically check the deadline internally to bail out early.🤖 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/api/metric_drilldown.rs` around lines 143 - 158, Update serialize_export and the blocking build_export flow so deadline expiration can be observed inside the blocking work and the export exits early when the deadline is reached, releasing its semaphore permit promptly. Preserve the existing timeout_at error mapping while ensuring the concurrency cap is not held beyond the client-visible execution limit.src/backend/services/analytics/src/api/mod.rs (1)
587-628: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
openapi_document()'s export-response patching.
openapi_document()mutates the generated document with severaland_then/ok_or_elsesteps that fail closed only at build time. No test in this file callsopenapi_document()to confirm it returnsOkand that the export path's "200" response actually contains bothtext/csvand the xlsx media type plus theContent-Dispositionheader. A short test would catch a regression here (e.g., from anOperationBuilder/utoipa upgrade) before it reaches the OpenAPI consumers.As per coding guidelines, "For non-obvious semantics, add a test whose name states the rule rather than adding a comment."
🤖 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/api/mod.rs` around lines 587 - 628, Add a focused test for openapi_document() that asserts it returns Ok, locates the /v1/metric-drilldown/export POST 200 response, and verifies both text/csv and XLSX media types plus the Content-Disposition header are present. Name the test to state the export-response OpenAPI rule and place it with the existing API tests.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/api/mod.rs`:
- Around line 605-627: Update the Content-Disposition header construction in the
response-building block to use a valid utoipa 5.5.0 HeaderBuilder API for
assigning its string schema, or directly construct the Header value when no
builder method supports it. Preserve the existing “Attachment filename”
description and response header key while removing the unsupported
HeaderBuilder::schema call.
---
Nitpick comments:
In `@src/backend/Cargo.toml`:
- Around line 70-71: Update the rust_xlsxwriter dependency declaration from 0.90
to the latest stable 0.9x release confirmed from the current crate registry, so
Cargo can receive fixes from newer releases while preserving the existing
dependency configuration.
In `@src/backend/services/analytics/src/api/metric_drilldown.rs`:
- Around line 143-158: Update serialize_export and the blocking build_export
flow so deadline expiration can be observed inside the blocking work and the
export exits early when the deadline is reached, releasing its semaphore permit
promptly. Preserve the existing timeout_at error mapping while ensuring the
concurrency cap is not held beyond the client-visible execution limit.
In `@src/backend/services/analytics/src/api/mod.rs`:
- Around line 587-628: Add a focused test for openapi_document() that asserts it
returns Ok, locates the /v1/metric-drilldown/export POST 200 response, and
verifies both text/csv and XLSX media types plus the Content-Disposition header
are present. Name the test to state the export-response OpenAPI rule and place
it with the existing API tests.
In `@src/backend/services/analytics/src/domain/metric_drilldown/export.rs`:
- Around line 89-91: Hoist a reusable blank-cell Format alongside date_format
before the per-row/per-column loop, then pass a reference to it in the
serde_json::Value::Null branch instead of constructing Format::new() for each
cell. Preserve the existing write_blank behavior and error mapping.
- Around line 64-143: Update build_xlsx to centralize XLSX failure handling in
an xlsx_error helper that logs the underlying error and returns
export_internal(), then replace every relevant map_err(|_| export_internal()) in
build_xlsx—including worksheet writes, add_table, and numeric conversions—with
xlsx_error calls using a context string identifying each operation. Preserve the
existing specialized save_to_writer handling unless the helper is appropriate
for that failure path.
In `@src/backend/services/analytics/src/domain/metric_drilldown/mod.rs`:
- Around line 20-22: Change the metric_drilldown re-exports for
MAX_EXPORT_BYTES, build_export, export_filename, export_internal, export_limit,
build_response, presentation, validate_export_request, and validate_request from
pub to pub(crate), preserving their existing module paths and cross-module
availability.
🪄 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 Plus
Run ID: e5eafb95-52e6-4b20-af3f-9419ae7ed40f
📥 Commits
Reviewing files that changed from the base of the PR and between 5ac0398 and 974b295eeeab01d62bc7fbc6a267be08ce61739c.
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/README.mddocs/domain/metrics/specs/DESIGN.mdsrc/backend/Cargo.tomlsrc/backend/services/analytics/Cargo.tomlsrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/metric_drilldown.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/domain/metric_drilldown/dto.rssrc/backend/services/analytics/src/domain/metric_drilldown/export.rssrc/backend/services/analytics/src/domain/metric_drilldown/mod.rssrc/backend/services/analytics/src/domain/metric_drilldown/validation.rssrc/ingestion/tests/e2e/api/test_metric_drilldown.py
974b295 to
48fbea8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/backend/services/analytics/src/domain/metric_drilldown/export.rs (7)
238-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
DebugforLimitedBuffer.The coding guidelines require
Debugon types. Add#[derive(Debug)]. Do not addClone, because no consumer clones the buffer.♻️ Proposed change
+#[derive(Debug)] struct LimitedBuffer { inner: Cursor<Vec<u8>>, limit: usize, }🤖 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_drilldown/export.rs` around lines 238 - 241, Derive Debug for the LimitedBuffer struct by adding the required type attribute, without deriving Clone or changing its fields.Source: Coding guidelines
302-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the function-local constants to the module top. Both sites declare a constant inside a function body. The coding guidelines ask for constants at the module top, grouped, with unit-suffixed names.
MAX_EXPORT_BYTESandMAX_CELL_BYTESat lines 13-14 are the group to join.
src/backend/services/analytics/src/domain/metric_drilldown/export.rs#L302-L303: renameMAX_BYTEStoMAX_FILENAME_SLUG_BYTESand move it to the module top.src/backend/services/analytics/src/domain/metric_drilldown/export.rs#L156-L160: moveDEADLINE_CHECK_EVERY_ROWSto the module top with the other constants.🤖 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_drilldown/export.rs` around lines 302 - 303, Move the function-local constants into the grouped module-level constants in src/backend/services/analytics/src/domain/metric_drilldown/export.rs:302-303: rename MAX_BYTES to MAX_FILENAME_SLUG_BYTES and update filename_slug to use it. At src/backend/services/analytics/src/domain/metric_drilldown/export.rs:156-160, move DEADLINE_CHECK_EVERY_ROWS to the same module-level group; no other behavior changes are needed.Source: Coding guidelines
94-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider matching
MetricDrilldownColumnTypeexhaustively.The arms at lines 116, 119, and 122 use
_for the column type. If a variant is added toMetricDrilldownColumnType, this code compiles and silently writes the new type as a string. The coding guidelines require exhaustive matches on project-owned enums without a wildcard arm.You can keep the code compact by matching the column type first, then the value shape inside each branch.
🤖 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_drilldown/export.rs` around lines 94 - 129, Update the match in the metric drilldown export flow to handle every MetricDrilldownColumnType variant explicitly instead of using wildcard column-type arms. Match the column type first and branch on the JSON value shape within each type, preserving the existing number, date, string, boolean, null, and fallback serialization behavior while ensuring newly added enum variants require a compile-time match update.Source: Coding guidelines
22-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid materializing all formatted rows for the XLSX branch.
formatted_rowsholds a complete string copy of the dataset. The CSV branch consumes it. The XLSX branch discards it and re-serializes the values fromrowsinbuild_xlsx. For a 25 MiB export this doubles peak memory for no output benefit.Consider validating per row without retaining the whole vector, and building the vector only for CSV. For example, accumulate the byte total and the cell-size check row by row, then let
build_csvformat each row as it writes.🤖 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_drilldown/export.rs` around lines 22 - 38, Update the export flow around formatted_rows and build_xlsx to avoid collecting formatted rows before selecting the format. For CSV, validate and retain the formatted values needed by build_csv; for XLSX, validate each row’s export input bounds while iterating without materializing or retaining the full formatted dataset, then pass rows directly to build_xlsx.Source: Coding guidelines
154-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the prose comment with a named test or an
INVARIANT:tag.The coding guidelines permit only the one-line tags
SAFETY,INVARIANT, andWORKAROUND, and they ask for a test whose name states the rule instead of a comment. The rule here is that serialization stops at the deadline so the concurrency permit is released.an_elapsed_deadline_aborts_serializationalready covers it. Delete the comment, or shorten it to a single// INVARIANT:line.🤖 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_drilldown/export.rs` around lines 154 - 155, In the export serialization logic, remove the multi-line prose comment because the invariant is already covered by the existing test an_elapsed_deadline_aborts_serialization. If a comment is still needed, replace it with a single-line // INVARIANT: comment describing that serialization stops at the deadline and releases the concurrency permit.Source: Coding guidelines
390-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the test cases for formula safety and the input bound.
Two gaps:
csv_export_is_bounded_and_formula_safeasserts only the=prefix.csv_safe_cellalso guards+,-,@, tab, CR, LF, and space. Use a table-driven loop over every prefix byte with a per-case assertion message, as the coding guidelines request.- No test covers
ensure_export_input_bound. Add a case that exceedsMAX_EXPORT_BYTESand asserts the rejection.The guidelines also ask tests to alias
type R = Result<(), Box<dyn Error>>to reduce ceremony. That would remove the repeatedunwrap_or_else(|error| panic!(...))calls in this module.Also applies to: 450-461
🤖 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_drilldown/export.rs` around lines 390 - 406, Extend csv_export_is_bounded_and_formula_safe with a table-driven check covering every csv_safe_cell dangerous prefix (=, +, -, @, tab, CR, LF, and space), including a per-case assertion message. Add a test for ensure_export_input_bound that supplies input exceeding MAX_EXPORT_BYTES and verifies rejection. In this test module, define the requested Result alias R and use it to simplify test return types and error handling.Source: Coding guidelines
325-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffGroup the error helpers and move the
MetricDrilldownExportFormatimpl.Three concerns in this block:
export_limitandexport_internalconstruct errors, but the module already haserror.rswithevidence_unavailable. The guidelines ask to centralize error construction. Move both helpers toerror.rs.impl MetricDrilldownExportFormatat lines 331-338 implements a type declared indto.rs, and it splits the two error helpers. Move it todto.rsnext to the enum.- The file is 589 lines. The guidelines ask to split a module that exceeds about 400 lines into focused modules. Serialization, the bounded buffer, and filename generation are three separable nouns.
🤖 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_drilldown/export.rs` around lines 325 - 342, Centralize export_limit and export_internal in error.rs alongside evidence_unavailable, and move the MetricDrilldownExportFormat implementation next to its enum in dto.rs. Split the oversized export.rs into focused modules for serialization, bounded-buffer handling, and filename generation, updating references while preserving behavior.Source: Coding guidelines
src/backend/services/analytics/src/domain/metric_drilldown/mod.rs (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the
dtomodule to crate-only visibility.Lines 13-18 re-export internal domain DTOs, validation data, and API request/response types, but they are only used inside the analytics crate. Change this block to
pub(crate)to avoid publishing internal service 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/domain/metric_drilldown/mod.rs` around lines 13 - 18, Change the `dto` re-export block in the metric drilldown module from public visibility to `pub(crate)`, keeping the existing exported symbols unchanged so these internal DTOs remain available throughout the analytics crate without being exposed externally.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_drilldown/export.rs`:
- Around line 109-115: Update the date handling in the MetricDrilldown export
match arm so an unparseable date does not become an internal server error;
either write the original value as a text cell with write_string or return a
client-visible invalid-argument/failed-precondition error that identifies the
column and row. Align the test an_unparseable_date_cell_fails_the_export with
the chosen behavior by naming that rule explicitly.
- Around line 98-107: Update the Number-column fallback in the metric drilldown
export match arm to write string values without JSON quoting, while preserving
stringification for other non-numeric JSON values. Extend
xlsx_writes_every_cell_shape_the_contract_allows or add a focused test that
verifies a non-numeric string in a Number column is written as unquoted cell
text.
- Around line 146-151: Update the error mapping around workbook.save_to_writer
in the XLSX export flow to distinguish LimitedBuffer’s “export byte limit
exceeded” write error from other XlsxError failures. Return export_limit(...)
only for the byte-limit case, and return export_internal() for non-limit I/O or
ZIP serialization failures while preserving the existing warning log.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_drilldown/export.rs`:
- Around line 238-241: Derive Debug for the LimitedBuffer struct by adding the
required type attribute, without deriving Clone or changing its fields.
- Around line 302-303: Move the function-local constants into the grouped
module-level constants in
src/backend/services/analytics/src/domain/metric_drilldown/export.rs:302-303:
rename MAX_BYTES to MAX_FILENAME_SLUG_BYTES and update filename_slug to use it.
At src/backend/services/analytics/src/domain/metric_drilldown/export.rs:156-160,
move DEADLINE_CHECK_EVERY_ROWS to the same module-level group; no other behavior
changes are needed.
- Around line 94-129: Update the match in the metric drilldown export flow to
handle every MetricDrilldownColumnType variant explicitly instead of using
wildcard column-type arms. Match the column type first and branch on the JSON
value shape within each type, preserving the existing number, date, string,
boolean, null, and fallback serialization behavior while ensuring newly added
enum variants require a compile-time match update.
- Around line 22-38: Update the export flow around formatted_rows and build_xlsx
to avoid collecting formatted rows before selecting the format. For CSV,
validate and retain the formatted values needed by build_csv; for XLSX, validate
each row’s export input bounds while iterating without materializing or
retaining the full formatted dataset, then pass rows directly to build_xlsx.
- Around line 154-155: In the export serialization logic, remove the multi-line
prose comment because the invariant is already covered by the existing test
an_elapsed_deadline_aborts_serialization. If a comment is still needed, replace
it with a single-line // INVARIANT: comment describing that serialization stops
at the deadline and releases the concurrency permit.
- Around line 390-406: Extend csv_export_is_bounded_and_formula_safe with a
table-driven check covering every csv_safe_cell dangerous prefix (=, +, -, @,
tab, CR, LF, and space), including a per-case assertion message. Add a test for
ensure_export_input_bound that supplies input exceeding MAX_EXPORT_BYTES and
verifies rejection. In this test module, define the requested Result alias R and
use it to simplify test return types and error handling.
- Around line 325-342: Centralize export_limit and export_internal in error.rs
alongside evidence_unavailable, and move the MetricDrilldownExportFormat
implementation next to its enum in dto.rs. Split the oversized export.rs into
focused modules for serialization, bounded-buffer handling, and filename
generation, updating references while preserving behavior.
In `@src/backend/services/analytics/src/domain/metric_drilldown/mod.rs`:
- Around line 13-18: Change the `dto` re-export block in the metric drilldown
module from public visibility to `pub(crate)`, keeping the existing exported
symbols unchanged so these internal DTOs remain available throughout the
analytics crate without being exposed externally.
🪄 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 Plus
Run ID: f60a2169-6c6e-4e1a-9558-17f8d9782b6f
📥 Commits
Reviewing files that changed from the base of the PR and between 974b295eeeab01d62bc7fbc6a267be08ce61739c and 1a6d70b1da8efaac7f312ec49ea80c5e89595564.
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/README.mddocs/domain/metrics/specs/DESIGN.mdsrc/backend/Cargo.tomlsrc/backend/services/analytics/Cargo.tomlsrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/metric_drilldown.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/domain/metric_drilldown/dto.rssrc/backend/services/analytics/src/domain/metric_drilldown/export.rssrc/backend/services/analytics/src/domain/metric_drilldown/mod.rssrc/backend/services/analytics/src/domain/metric_drilldown/validation.rssrc/ingestion/tests/e2e/api/test_metric_drilldown.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/backend/services/analytics/src/domain/metric_drilldown/validation.rs
- src/ingestion/tests/e2e/api/test_metric_drilldown.py
- src/backend/services/analytics/Cargo.toml
- src/backend/services/analytics/src/domain/metric_drilldown/dto.rs
- src/backend/services/analytics/src/api/mod.rs
- src/backend/services/analytics/src/api/metric_drilldown.rs
- docs/components/backend/analytics/openapi.json
- docs/domain/metrics/README.md
- src/backend/Cargo.toml
- src/backend/services/analytics/src/api/http_live_tests.rs
- docs/domain/metrics/specs/DESIGN.md
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
1a6d70b to
3fa69b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/backend/services/analytics/src/domain/metric_drilldown/export.rs (2)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the visibility of
BYTE_LIMIT_MARKER,build_export, andexport_filename.
BYTE_LIMIT_MARKERis referenced only inside this file (lines 174, 308, 310, 324) andmod.rsdoes not re-export it.build_exportandexport_filenameare re-exported aspub(crate)inmod.rs, sopubadds no reachable surface.♻️ Proposed visibility tightening
-pub const BYTE_LIMIT_MARKER: &str = "export byte limit exceeded"; +const BYTE_LIMIT_MARKER: &str = "export byte limit exceeded";-pub fn build_export( +pub(crate) fn build_export(-pub fn export_filename( +pub(crate) fn export_filename(As per coding guidelines: "Use the smallest visibility that compiles: prefer
pub(crate)beforepub, and do not add speculative API surface."🤖 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_drilldown/export.rs` around lines 12 - 16, Reduce the visibility of `BYTE_LIMIT_MARKER` to private because it is used only within `export.rs`; change `build_export` and `export_filename` from `pub` to `pub(crate)` because `mod.rs` already re-exports them at crate visibility. Preserve their existing behavior and exports.Source: Coding guidelines
24-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the shared budget loop and reuse the formatted values.
Both match arms build an
ExportInputBudgetand add every row. The XLSX arm callsexport_valuesonce per row, allocates aVec<String>, then discards it;build_xlsxre-reads the raw JSON values afterwards. Extract one helper that formats rows and charges the budget, then pass the result to each writer.This also removes the duplicated budget construction.
As per coding guidelines: "Extract repetition into named helpers" and "Avoid allocations in per-row or per-item loops when values can be borrowed or hoisted".
🤖 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_drilldown/export.rs` around lines 24 - 52, Extract the duplicated row-formatting and budget-accounting logic from the match arms into a named helper near the export flow, returning the formatted rows after charging one ExportInputBudget. Update both Csv and Xlsx branches to call this helper once and pass the resulting formatted values to build_csv and build_xlsx, avoiding repeated export_values calls and discarded per-row allocations.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_drilldown/export.rs`:
- Line 190: Declare a workspace or crate minimum Rust version of 1.91, which
supports both Duration::from_mins and usize::is_multiple_of; update the relevant
package metadata or add the repository’s standard rust-toolchain configuration
so CI and contributors use at least that compiler version.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_drilldown/export.rs`:
- Around line 12-16: Reduce the visibility of `BYTE_LIMIT_MARKER` to private
because it is used only within `export.rs`; change `build_export` and
`export_filename` from `pub` to `pub(crate)` because `mod.rs` already re-exports
them at crate visibility. Preserve their existing behavior and exports.
- Around line 24-52: Extract the duplicated row-formatting and budget-accounting
logic from the match arms into a named helper near the export flow, returning
the formatted rows after charging one ExportInputBudget. Update both Csv and
Xlsx branches to call this helper once and pass the resulting formatted values
to build_csv and build_xlsx, avoiding repeated export_values calls and discarded
per-row allocations.
🪄 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 Plus
Run ID: ee3bcc33-f4cc-4b77-ad20-79abaebd679d
📥 Commits
Reviewing files that changed from the base of the PR and between 1a6d70b1da8efaac7f312ec49ea80c5e89595564 and 3fa69b5.
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/README.mddocs/domain/metrics/specs/DESIGN.mdsrc/backend/Cargo.tomlsrc/backend/services/analytics/Cargo.tomlsrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/metric_drilldown.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/domain/metric_drilldown/dto.rssrc/backend/services/analytics/src/domain/metric_drilldown/error.rssrc/backend/services/analytics/src/domain/metric_drilldown/export.rssrc/backend/services/analytics/src/domain/metric_drilldown/mod.rssrc/backend/services/analytics/src/domain/metric_drilldown/validation.rssrc/ingestion/tests/e2e/api/test_metric_drilldown.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/backend/services/analytics/Cargo.toml
- src/ingestion/tests/e2e/api/test_metric_drilldown.py
- docs/components/backend/analytics/openapi.json
- src/backend/services/analytics/src/domain/metric_drilldown/validation.rs
- src/backend/services/analytics/src/api/metric_drilldown.rs
- src/backend/Cargo.toml
- src/backend/services/analytics/src/api/http_live_tests.rs
- docs/domain/metrics/README.md
- docs/domain/metrics/specs/DESIGN.md
- src/backend/services/analytics/src/domain/metric_drilldown/dto.rs
- src/backend/services/analytics/src/api/mod.rs
`POST /v1/metric-drilldown/export` (#2074) is the analytics document's 30th operation and the only one that does not answer JSON — it serves CSV or XLSX. Catalogued anyway: the edge refuses an anonymous caller before content negotiation happens, which is the property the sweep asserts about every other operation, and an uncatalogued route is one the sweep silently never reaches. Generated models regenerated against the same document, so `--check` agrees. It has no session-carrying test yet, so the gate now reports it SWEPT ONLY alongside `POST /v1/metric-drilldown` itself — correctly. Drilldown is one of the areas this suite has never exercised with a caller, and naming it is what makes that visible rather than absent. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
… next ones Rebase picks up the first #1669 fidelity fixes. metric-definitions, metric-drilldown and metric-results now declare what they can actually answer instead of the seven-code `.standard_errors` stamp, which retires two of this gate's 403 exclusions: those operations no longer declare a 403 to subtract. The gate did not notice, and its own comment claimed it would. `stale_blocked` only asked whether the OPERATION had left the document — but a fidelity fix does not remove the operation, it stops the operation over-declaring. So an exclusion written against the old text goes on suppressing nothing while still reading as a live judgement about the route, which is the worst state for a suppression list to be in. `blocked_undeclared` closes that, and immediately named both entries. They are gone. `POST /v1/metric-drilldown/export` takes their place: it arrived with #2074 carrying the old boilerplate and the same absent gate, so its 403 is the over-declaration the others just shed. Reported, never blocking — a corrected document must not fail the gate. Generated models regenerated against the same spec: 72/150 coverable codes. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
3 participants
Summary
POST /v1/metric-drilldown/export: the drilldown selection exported server-side as CSV or XLSX with the same projected columnsStack
Merges bottom-up; each PR retargets to
mainas its parent lands.UI layer: constructorfabric/insight-front#226
Closes #2070
Validation
cargo test -p analyticsSummary by CodeRabbit