Skip to content

Conversation

@haritamar
Copy link
Collaborator

@haritamar haritamar commented Oct 28, 2025

null

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of test result sample/metrics data: disables sample payloads when configured, consistently uses metrics for anomaly detection, and stabilizes sorting and type handling to produce more reliable test results.
  • Chores

    • Updated dependency sourcing and lock metadata to pin a specific data-reliability revision and adjusted package declarations to reflect the new source.

@linear
Copy link

linear bot commented Oct 28, 2025

@github-actions
Copy link
Contributor

👋 @haritamar
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

Cast and consume stored sample data as metrics for anomaly detection: tests handler now reads test_result_db_row.sample_data directly, sets it to None when samples are disabled, assigns metrics from that source, and conditionally sorts metrics by end_time. Also updates dbt_project package entries to a specific git revision.

Changes

Cohort / File(s) Summary
Test result handling
elementary/monitor/api/tests/tests.py
Read and cast test_result_db_row.sample_data directly; when disable_samples is true set sample_data to None. In the anomaly_detection branch assign metrics := test_result_db_row.sample_data, use metrics for type checks and sorting (sort by end_time when metrics is a list and subtype != "dimension"), and populate the schema's metrics field instead of passing sample_data.
DBT package updates
elementary/monitor/dbt_project/packages.yml, elementary/monitor/dbt_project/package-lock.yml
Switch active package entry to a specific git-sourced dbt-data-reliability revision (a01c958...) and update the package-lock git SHA (c91170dd...); previous unreleased/CLI entries moved into commented blocks and historical references preserved as comments.

Sequence Diagram(s)

sequenceDiagram
  participant DB as test_result_db_row
  participant API as tests handler
  participant Schema as ElementaryTestResultSchema

  DB->>API: provide test_result_db_row (sample_data)
  API->>API: cast test_result_db_row.sample_data
  alt disable_samples == true
    API->>API: set sample_data = None
  end
  API->>API: metrics := test_result_db_row.sample_data
  alt anomaly_detection branch
    opt metrics is list and subtype != "dimension"
      API->>API: sort(metrics) by end_time
    end
    API->>Schema: construct result with metrics -> metrics field
  else non-anomaly branch
    API->>Schema: construct result (previous non-anomaly flow)
  end
  Schema->>API: return ElementaryTestResultSchema
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect elementary/monitor/api/tests/tests.py for correct casting, None vs empty handling, and whether callers expect sample_data preserved.
  • Verify sorting condition (list && subtype != "dimension") and stability of sort key end_time.
  • Confirm packages.yml and package-lock.yml git revision and SHA are intentional and consistent.

Possibly related PRs

Suggested reviewers

  • ofek1weiss
  • arbiv

Poem

"I'm a rabbit in the code so spry,
Samples tucked away when told goodbye,
Metrics hop in line by end_time's light,
Commits pinned tight, the deps sit right,
Cheep, hop — the tests now sleep polite!"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Ele 5146 bugfix to disable_samples flag + update package ref" clearly describes the two main components of this changeset. It accurately references the disable_samples bugfix addressed in tests.py where the logic is modified to directly cast and set sample_data, and the package reference update reflected in both package-lock.yml and packages.yml where git dependency metadata is updated. The title is concise, specific, and includes the ticket identifier, making it clear and scannable for someone reviewing the repository history. The title avoids vague terminology and provides sufficient detail to understand the primary changes without listing individual files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ele-5146-add-flag-for-disabling-samples-in-cloud

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8a191e7 and a378426.

📒 Files selected for processing (1)
  • elementary/monitor/api/tests/tests.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • elementary/monitor/api/tests/tests.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: test / test
  • GitHub Check: code-quality

Comment @coderabbitai help to get the list of available commands and usage tips.

) -> Optional[Union[DbtTestResultSchema, ElementaryTestResultSchema]]:
test_results: Optional[Union[DbtTestResultSchema, ElementaryTestResultSchema]]

sample_data = test_result_db_row.sample_data if not disable_samples else None
Copy link
Collaborator Author

@haritamar haritamar Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug here where the disable_samples flags also removed anomaly tests rows (metrics), which is incorrect

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
elementary/monitor/api/tests/tests.py (1)

463-474: Good fix! Correctly preserves anomaly detection metrics.

The variable rename to metrics and the absence of disable_samples handling correctly addresses the issue noted in the past review - anomaly test metrics should not be affected by the disable_samples flag since they represent the actual test results, not just samples.

Consider adding a brief comment explaining that metrics are intentionally not affected by disable_samples:

 if test_result_db_row.test_type == "anomaly_detection":
+    # Metrics represent the actual test results and should not be disabled
     metrics = test_result_db_row.sample_data
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c9211de and aa5cc87.

📒 Files selected for processing (3)
  • elementary/monitor/api/tests/tests.py (2 hunks)
  • elementary/monitor/dbt_project/package-lock.yml (1 hunks)
  • elementary/monitor/dbt_project/packages.yml (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/monitor/api/tests/tests.py (2)
elementary/monitor/api/tests/schema.py (1)
  • ElementaryTestResultSchema (8-14)
elementary/monitor/alerts/test_alert.py (2)
  • display_name (114-115)
  • test_sub_type_display_name (106-107)
🔇 Additional comments (3)
elementary/monitor/api/tests/tests.py (1)

447-458: LGTM! Proper implementation of disable_samples for dbt tests.

The explicit cast and conditional emptying of sample_data correctly implements the feature to disable samples for dbt tests when the flag is enabled.

elementary/monitor/dbt_project/package-lock.yml (1)

5-6: LGTM! Consistent with packages.yml.

The revision and sha1_hash are correctly updated and consistent with the dependency changes in packages.yml.

elementary/monitor/dbt_project/packages.yml (1)

4-5: Confirm intentional use of unreleased development code.

The git revision exists and is valid, but it is a post-release commit on the main branch (dated 2025-10-28) that has not been included in any stable release. The latest stable release is 0.20.1 (2025-10-09). Using this commit means your dependency is on unreleased, development code rather than a tested, production release. Verify this is intentional and acceptable for your project's stability requirements. If stability is a concern, consider pinning to the latest stable release tag (e.g., revision: 0.20.1) instead.

@haritamar haritamar changed the title Ele 5146 add flag for disabling samples in cloud Ele 5146 bugfix to disable_samples flag + update package ref Oct 28, 2025
@haritamar haritamar merged commit ff1fc83 into master Oct 29, 2025
5 of 6 checks passed
@haritamar haritamar deleted the ele-5146-add-flag-for-disabling-samples-in-cloud branch October 29, 2025 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants