store mttr history for given date #1451
Conversation
add structs for daily mttr history add helper functions in object store to save mttr history
WalkthroughThis PR introduces MTTR (Mean Time To Resolution) data structures and persistent storage capabilities. It adds three new data structures for MTTR tracking, extends the Metastore trait with MTTR history accessor methods, implements those methods in ObjectStoreMetastore, and adds a new storage path utility for MTTR JSON files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Metastore
participant ObjectStore
participant Storage as Object Storage
Client->>Metastore: get_mttr_history()
activate Metastore
Metastore->>ObjectStore: Read mttr.json from ALERTS_ROOT_DIRECTORY
ObjectStore->>Storage: Fetch file at mttr_json_path()
Storage-->>ObjectStore: Return JSON content (or not found)
ObjectStore-->>Metastore: Deserialize to MTTRHistory or None
Metastore-->>Client: Result<Option<MTTRHistory>>
deactivate Metastore
Client->>Metastore: put_mttr_history(history)
activate Metastore
Metastore->>ObjectStore: Write MTTRHistory object
ObjectStore->>Storage: Write to history.get_object_path()
Storage-->>ObjectStore: Success
ObjectStore-->>Metastore: Result<()>
Metastore-->>Client: Result<()>
deactivate Metastore
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/alerts/alert_structs.rs(4 hunks)src/metastore/metastore_traits.rs(2 hunks)src/metastore/metastores/object_store_metastore.rs(3 hunks)src/storage/object_storage.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/metastore/metastore_traits.rs (1)
src/metastore/metastores/object_store_metastore.rs (2)
get_mttr_history(310-323)put_mttr_history(326-329)
src/alerts/alert_structs.rs (2)
src/storage/object_storage.rs (3)
alert_json_path(1140-1142)alert_state_json_path(1157-1162)mttr_json_path(1167-1169)src/metastore/metastore_traits.rs (2)
get_object_id(204-204)get_object_path(203-203)
src/metastore/metastores/object_store_metastore.rs (2)
src/metastore/metastore_traits.rs (2)
get_mttr_history(84-84)put_mttr_history(85-85)src/storage/object_storage.rs (1)
to_bytes(1082-1086)
⏰ 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). (10)
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
🔇 Additional comments (11)
src/storage/object_storage.rs (2)
1155-1155: Documentation correction looks good.The comment now accurately reflects the actual path format used in the implementation below.
1164-1169: LGTM!The new
mttr_json_path()function follows the established pattern for path helpers and is correctly integrated with the MTTR history storage implementation.src/metastore/metastore_traits.rs (2)
30-33: LGTM!The import correctly adds
MTTRHistoryalongside existing alert-related types, maintaining consistent structure.
83-85: LGTM!The new MTTR history methods follow the established patterns in the
Metastoretrait, withget_mttr_historyreturningOption<MTTRHistory>for safe handling of missing data andput_mttr_historyusing the trait object pattern consistent with other put methods.src/metastore/metastores/object_store_metastore.rs (3)
35-38: LGTM!The import correctly adds
MTTRHistoryfor use in the new metastore method implementations.
116-116: Minor style improvement.Consolidating to a single line is consistent with similar methods throughout the file.
325-329: LGTM!The
put_mttr_historyimplementation correctly follows the established pattern by usingobj.get_object_path()to retrieve the storage path.src/alerts/alert_structs.rs (4)
21-21: LGTM!
NaiveDateimport is correctly added for use in theDailyMTTRStatsstruct.
38-38: LGTM!The
mttr_json_pathimport is correctly added for use in theMTTRHistoryimplementation ofMetastoreObject.
613-638: LGTM!The new MTTR data structures are well-designed:
DailyMTTRStatsappropriately usesNaiveDatefor date-only storageMTTRHistoryprovides a clean container for time-series MTTR dataMTTRQueryParamsusesOption<String>for flexible date filtering from query parameters- All structs follow the established serialization patterns with camelCase
805-813: LGTM!The
MetastoreObjectimplementation forMTTRHistorycorrectly uses a constant object ID and leverages themttr_json_path()helper, following the established pattern.
| /// Get MTTR history from storage | ||
| async fn get_mttr_history(&self) -> Result<Option<MTTRHistory>, MetastoreError> { | ||
| let path = RelativePathBuf::from_iter([ALERTS_ROOT_DIRECTORY, "mttr.json"]); | ||
| match self.storage.get_object(&path).await { | ||
| Ok(bytes) => { | ||
| if let Ok(history) = serde_json::from_slice::<MTTRHistory>(&bytes) { | ||
| Ok(Some(history)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } | ||
| Err(ObjectStorageError::NoSuchKey(_)) => Ok(None), | ||
| Err(e) => Err(MetastoreError::ObjectStorageError(e)), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the mttr_json_path() helper for consistency.
Line 311 hardcodes the path construction, while put_mttr_history (Line 327) uses obj.get_object_path() which calls the mttr_json_path() helper. This creates an inconsistency and violates DRY. Consider using the helper directly for better maintainability.
Apply this diff:
- async fn get_mttr_history(&self) -> Result<Option<MTTRHistory>, MetastoreError> {
- let path = RelativePathBuf::from_iter([ALERTS_ROOT_DIRECTORY, "mttr.json"]);
+ async fn get_mttr_history(&self) -> Result<Option<MTTRHistory>, MetastoreError> {
+ let path = mttr_json_path();
match self.storage.get_object(&path).await {Note: You'll need to import mttr_json_path from crate::storage::object_storage at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/metastore/metastores/object_store_metastore.rs around lines 309 to 323,
the get_mttr_history function hardcodes the path with
RelativePathBuf::from_iter([ALERTS_ROOT_DIRECTORY, "mttr.json"]) instead of
using the existing mttr_json_path helper; replace the hardcoded path with a call
to mttr_json_path() (or obj.get_object_path()/equivalent helper used elsewhere)
and add an import for mttr_json_path from crate::storage::object_storage at the
top of the file so the same path construction is reused consistently.
1 participant
add structs for daily mttr history
add helper functions in object store to save mttr history
Summary by CodeRabbit