feat: add summary module for aggregating runtime information and internal metrics #365

simonsan · 2024-11-21T22:16:01Z

An informative summary system for aggregating and condensing data collected
from runtime checks, including warnings, issues, and operational metrics.

This system should provide end-users with a clear, concise summary of command
execution results without conflicting with existing error-handling standards.
In scenarios where execution cannot proceed due to a critical error, a
RusticError will be raised instead, and no summary will be provided.

Separation of Concerns

Critical runtime errors that prevent further execution are handled through the
existing RusticError system. The Summary will only collect information for
non-fatal events.

Compatibility with Existing Error Handling

Summaries must coexist with error propagation rules. They will not replace
the core behavior of error propagation but act as a complementary mechanism
for presenting non-fatal feedback.

User-Friendly Reporting

Summaries should aggregate detailed runtime information—such as warnings,
issues, and metrics — in a clear and condensed format for the end-user.

Aggregation & Condensation

Similar or repeated errors should be aggregated to avoid redundant information,
presenting users with a high-level overview.

…rnal metrics Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

codecov · 2024-11-21T22:28:51Z

Codecov Report

Attention: Patch coverage is 54.65116% with 39 lines in your changes missing coverage. Please review.

Project coverage is 42.2%. Comparing base (e812ae1) to head (f30bb6e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/core/src/error/summary.rs	54.6%	39 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
crates/core/src/error.rs	`67.5% <ø> (-2.0%)`	⬇️
crates/core/src/error/summary.rs	`54.6% <54.6%> (ø)`

... and 22 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

…nd update export logic Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan · 2024-11-21T22:59:13Z

crates/core/src/error/summary.rs

+pub enum DisplayOptionKind {
+    #[default]
+    Issues,
+    Timing,
+    Metrics,
+    All,
+}
+


These could be bitflags or enumflags, but I'm unsure if that wouldn't be overengineering?

…ic method Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

…ethods to use EcoString Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan · 2024-11-22T01:04:54Z

What's still missing is functionality to store different categories of issues, e.g. warnings or soft errors. And then to log them to the terminal.

TODO:

store different categories of issues (warnings, soft errors)
add matching logic to decide which log macro to use
add setting to Summary, that enables/disables logging on adding issues

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

…ove logging Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

… Summary Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

…tional fields in Summary Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

crates/core/src/error/summary.rs

simonsan · 2024-11-25T13:52:16Z

crates/core/src/error/summary.rs

+
+use ecow::EcoString;
+
+pub type Issues = BTreeMap<IssueCategory, BTreeMap<IssueScope, CondensedIssue>>;


Is IssueScope needed, or do we want to deduplicated on some other stuff? We can also not use a BTreeMap here, but just a Vector.

pub type Issues = BTreeMap<IssueCategory, Vec<CondensedIssue>>;

Could IssueScope be a member of CondensedIssue?
So you use a Vec and still have a notion of IssueScope

…king logic Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan · 2024-11-25T16:12:13Z

crates/core/src/error/summary.rs

+    // ! TODO: Refactor this method to merge the context of each summary
+    // !
+    // ? How do we merge the context of each summary?
+    // ? We can't merge the context, as it's a unique identifier for each summary.
+    // ? We could add a new field to each CondensedIssue to store the context of the
+    // ? merged summaries.
+    // ?
+    // ? How do we merge the other fields? E.g. Timing, Metrics, etc.
+    pub fn merge(&mut self, other: Self) {
+        self.issues.extend(other.issues);
+        self.metrics.extend(other.metrics);
+    }


Use conflate and implement Merge, make sure we fulfil above requirements and make it as ergonomic as possible. Make sure deduplication works well also for edge cases. We want to be sure, that we can assess if a command soft errored (warnings are lower priority).

Ah, conflate is actually behind the merge feature flag and doesn't make sense then to be used here.

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

nardoor · 2024-12-01T22:23:06Z

crates/core/src/error/summary.rs

+
+use ecow::EcoString;
+
+pub type Issues = BTreeMap<IssueCategory, BTreeMap<IssueScope, CondensedIssue>>;


Could IssueScope be a member of CondensedIssue?
So you use a Vec and still have a notion of IssueScope

nardoor · 2024-12-01T22:27:52Z

crates/core/src/error/summary.rs

+    metrics: Metrics,
+
+    /// Display this data
+    display: HashSet<DisplayOptionKind>,


As I'm beginning to read through the code, I am not (yet) fully undertanding what DisplayOptionKind is about.
Could you detail it a bit more in the code-doc? (for instance in the enum's doc)

nardoor · 2024-12-01T22:33:19Z

crates/core/src/error/summary.rs

+    pub fn enable_log(&mut self) -> Self {
+        self.log_enabled = true;
+        self.clone()
+    }


I wonder what is the reason to use clone there?

The builder pattern usually is something like:

pub fn enable_log(mut self) -> Self { self.log_enabled = true; self }

nardoor · 2024-12-01T22:38:44Z

crates/core/src/error/summary.rs

+        _ = self
+            .issues
+            .entry(category)
+            .or_default()
+            .entry(scope)
+            .and_modify(|val| {
+                val.count += 1;
+                if val.root_cause.is_none() {
+                    val.root_cause.clone_from(&root_cause);
+                }
+            })
+            .or_insert(CondensedIssue {
+                category,
+                message,
+                count: 1,
+                root_cause,
+            });


As I see it, when we get in the and_modify case, we loose the message.
Is that voluntary?

Same we might have != root_cause but we'll keep only one.

Wouldn't this lead to "wrong" reporting of some error A that happened N times, but in reality, A happened N1 times and B, C, D, E had the same scope and happened but their "message" and "root_cause" are lost.

I understand we want to aggregate and get a high level overview. But as I understand this, we could become misleading there.

nardoor · 2024-12-01T22:40:09Z

crates/core/src/error/summary.rs

+        _ = self
+            .metrics
+            .entry(key.into())
+            .and_modify(|val| *val = value.clone())
+            .or_insert_with(|| value);
+    }


Here I see that metrics are erase on save.

I feel like this should be documented in the add method.

Maybe also this method should be renamed set_metric?

nardoor · 2024-12-01T22:43:34Z

crates/core/src/error/summary.rs

+
+    pub fn export_all(&mut self) -> bool {
+        self.display.insert(DisplayOptionKind::All)
+    }


could you let me know of use cases where we want only some DisplayOptionKind and not some others?

Also I feel it's a bit weird that ALL is an enum value.

feat: add summary module for aggregating runtime information and inte…

3be2def

…rnal metrics Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan added 2 commits November 21, 2024 23:55

feat: enhance Summary struct to use EcoString

5908230

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

refactor: remove unnecessary Deserialize derive from CondensedIssue a…

9b6dcb7

…nd update export logic Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan commented Nov 21, 2024

View reviewed changes

feat: update Metrics type to use MetricValue enum and modify add_metr…

9611df5

…ic method Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan requested review from aawsome and nardoor November 21, 2024 23:14

simonsan added A-errors Area: error handling needs improvement C-enhancement Category: New feature or request A-api Area: Related to API design of rustic_core A-dx Area: Related to Developer Experience A-commands Area: Related to commands in `rustic_core` labels Nov 21, 2024

refactor: simplify Metrics type and update add_issue and add_metric m…

2265256

…ethods to use EcoString Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan added the S-waiting-for-review Status: PRs waiting for review label Nov 22, 2024

simonsan added 8 commits November 24, 2024 20:51

Merge branch 'main' into feat/summary

a9e89dc

feat: add IssueCategory enum and logging for issues in Summary

8bbace6

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: refactor issue handling to categorize by IssueCategory and impr…

1f40182

…ove logging Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: add merge method to Summary and corresponding unit test

efb345e

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: add TODO comments for refactoring merge method in Summary

bbafe09

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: add methods to log errors, warnings, and info in Summary

a437de4

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: add method to enable logging in Summary

8e13aa9

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: enhance logging functionality and update root cause handling in…

f169967

… Summary Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan mentioned this pull request Nov 25, 2024

feat: Improve summary handling and error reporting in repository checks #369

Draft

simonsan added 2 commits November 25, 2024 14:24

feat: update merge method to use Self type and improve handling of op…

6d61317

…tional fields in Summary Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: add methods to convert Summary into boxed and arc mutex types

a27dc6e

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan commented Nov 25, 2024

View reviewed changes

crates/core/src/error/summary.rs Show resolved Hide resolved

simonsan commented Nov 25, 2024

View reviewed changes

feat: rename is_error method to contains_error and improve error chec…

ddf7828

…king logic Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan commented Nov 25, 2024

View reviewed changes

simonsan added 3 commits November 25, 2024 18:10

feat: add method to retrieve Summary from Arc<Mutex> with error handling

4b3d073

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: update retrieve_from_arc_mutex method to use Self type

08a66fb

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

feat: add assertions to tests for contains_error method in Summary

f30bb6e

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

nardoor reviewed Dec 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add summary module for aggregating runtime information and internal metrics #365

feat: add summary module for aggregating runtime information and internal metrics #365

simonsan commented Nov 21, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

simonsan Nov 21, 2024

simonsan commented Nov 22, 2024 •

edited

Loading

simonsan Nov 25, 2024

nardoor Dec 1, 2024

simonsan Nov 25, 2024

simonsan Nov 25, 2024

nardoor Dec 1, 2024

nardoor Dec 1, 2024

nardoor Dec 1, 2024

nardoor Dec 1, 2024

nardoor Dec 1, 2024

nardoor Dec 1, 2024


		use ecow::EcoString;

		pub type Issues = BTreeMap<IssueCategory, BTreeMap<IssueScope, CondensedIssue>>;

feat: add summary module for aggregating runtime information and internal metrics #365

Are you sure you want to change the base?

feat: add summary module for aggregating runtime information and internal metrics #365

Conversation

simonsan commented Nov 21, 2024

Separation of Concerns

Compatibility with Existing Error Handling

User-Friendly Reporting

Aggregation & Condensation

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

simonsan commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2024 •

edited

Loading

simonsan commented Nov 22, 2024 •

edited

Loading