update: add other fields to alert request, config and response #1459
Conversation
also exclude alert state from loading alerts
WalkthroughAdds flattened Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as AlertsHandler
participant Service as AlertService
participant Store as Metastore
Note over Client,HTTP: GET /alerts with reserved + dynamic query params
Client->>HTTP: request (tags, offset, limit, region=us, foo=bar)
HTTP->>Service: parse params → ListQueryParams (reserved, other_fields_filters)
Service->>Store: list alert objects (filter ".json" AND NOT "alert_state_*")
Store-->>Service: object list
Service->>Service: deserialize -> AlertConfig (created uses fallback/default)
Service->>Service: to summaries (include other_fields)
Service->>Service: filter_by_other_fields (match all dynamic filters)
Service->>Service: sort_alerts → paginate_alerts
Service-->>HTTP: paginated, sorted summaries
HTTP-->>Client: 200 OK (response)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
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 (5)
src/alerts/alert_structs.rs(7 hunks)src/alerts/alert_types.rs(5 hunks)src/alerts/mod.rs(2 hunks)src/handlers/http/alerts.rs(3 hunks)src/metastore/metastores/object_store_metastore.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-24T11:54:20.269Z
Learnt from: parmesant
Repo: parseablehq/parseable PR: 1449
File: src/metastore/metastores/object_store_metastore.rs:83-98
Timestamp: 2025-10-24T11:54:20.269Z
Learning: In the `get_overviews` method in `src/metastore/metastores/object_store_metastore.rs`, using `.ok()` to convert all storage errors to `None` when fetching overview objects is the intended behavior. This intentionally treats missing files and other errors (network, permissions, etc.) the same way.
Applied to files:
src/metastore/metastores/object_store_metastore.rs
📚 Learning: 2025-07-24T11:09:21.781Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
Applied to files:
src/alerts/alert_types.rssrc/alerts/alert_structs.rssrc/alerts/mod.rs
🧬 Code graph analysis (3)
src/alerts/alert_types.rs (1)
src/alerts/alert_structs.rs (2)
default_created_time(84-86)deserialize_datetime_with_empty_string_fallback(52-80)
src/alerts/alert_structs.rs (1)
src/alerts/mod.rs (1)
serde_json(1034-1034)
src/alerts/mod.rs (1)
src/event/mod.rs (1)
map(137-137)
⏰ 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: Build Default x86_64-pc-windows-msvc
- GitHub Check: coverage
- 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-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (15)
src/metastore/metastores/object_store_metastore.rs (1)
183-185: LGTM! Proper separation of alert configs from alert state files.The filter correctly excludes alert state files when retrieving alerts, ensuring only alert configuration files are loaded. This aligns with the separate
get_alert_states()method for retrieving state-specific files.src/handlers/http/alerts.rs (2)
54-56: LGTM! Clean separation of reserved and dynamic query parameters.The reserved parameters list is well-defined, and the
other_fields_filtersmap provides a clean way to capture arbitrary additional filters.
94-100: LGTM! Correct collection of non-reserved query parameters.The loop properly filters out reserved parameters and collects the remaining fields for filtering alerts.
src/alerts/mod.rs (2)
137-137: LGTM! Correct initialization of other_fields during v1 migration.Setting
other_fields: Noneis appropriate for v1 alerts that didn't have this field, maintaining backward compatibility.
686-690: LGTM! Proper propagation of other_fields to alert summary.The implementation correctly includes dynamic fields in the summary when present, placed after standard fields to avoid accidental overwrites.
src/alerts/alert_types.rs (4)
22-22: LGTM! Necessary imports for other_fields and datetime handling.The imports support the new
other_fieldsfunctionality and robust datetime deserialization.Also applies to: 32-35
68-71: LGTM! Robust datetime handling for the created field.The serde attributes provide graceful handling of legacy data with empty timestamps while maintaining backward compatibility.
76-77: LGTM! Correct implementation of other_fields with serde flatten.The use of
#[serde(flatten)]properly captures arbitrary additional fields during deserialization while maintaining the optional nature of the field.
421-421: LGTM! Bidirectional propagation of other_fields in conversions.Both conversion implementations correctly preserve
other_fields, ensuring no data loss during type transformations betweenAlertConfigandThresholdAlert.Also applies to: 445-445
src/alerts/alert_structs.rs (6)
42-86: LGTM! Excellent implementation of backward-compatible datetime handling.The custom deserializer is well-documented and handles legacy data gracefully:
- Supports valid timestamps, empty strings, and missing fields
- Logs warnings for data quality issues
- Clear migration path documentation
This is a good example of defensive programming for data migration scenarios.
303-304: LGTM! Consistent addition of other_fields to AlertRequest.The flattened optional field allows clients to include arbitrary additional metadata in alert requests.
361-361: LGTM! Correct propagation of other_fields during request conversion.The implementation ensures that dynamic fields from the alert request are preserved in the resulting
AlertConfig.
386-394: LGTM! Consistent datetime handling and other_fields implementation in AlertConfig.The serde attributes match those in
ThresholdAlert, ensuring consistent behavior across alert types.
418-426: LGTM! Complete propagation of other_fields to API responses.The
AlertConfigResponsecorrectly includes the same datetime handling andother_fieldsasAlertConfig, ensuring API clients receive all alert metadata.
466-466: LGTM! Final link in other_fields propagation chain.The implementation ensures that dynamic fields are preserved all the way through to API responses, completing the end-to-end data flow.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/alerts/alert_structs.rs (1)
42-80: Well-designed compatibility layer for legacy timestamps.The deserializer correctly handles empty strings, missing fields, and valid timestamps. The warning logging is helpful for identifying data quality issues.
Note: If many legacy alerts contain empty
createdfields, the warning logs could be verbose during alert loading. Consider monitoring log volume and possibly implementing a summary message (e.g., "Found N alerts with empty created timestamps") if this becomes an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/alerts/alert_structs.rs(7 hunks)src/alerts/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/alerts/mod.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T11:09:21.781Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
Applied to files:
src/alerts/alert_structs.rs
🧬 Code graph analysis (1)
src/alerts/alert_structs.rs (1)
src/alerts/mod.rs (1)
serde_json(1034-1034)
⏰ 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 Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (6)
src/alerts/alert_structs.rs (6)
22-23: LGTM!The new imports are necessary for the custom deserializer and dynamic field handling introduced in this PR.
82-86: LGTM!Clean helper function that provides a sensible default for missing
createdtimestamps.
361-361: LGTM!Correctly propagates
other_fieldsfrom the request to the alert configuration.
386-394: LGTM!The combination of
defaultanddeserialize_withon thecreatedfield correctly handles missing fields, empty strings, and valid timestamps. Theother_fieldsaddition maintains consistency withAlertRequest.
418-426: LGTM!The datetime handling and
other_fieldsimplementation are consistent withAlertConfig.
466-466: LGTM!Correctly propagates
other_fieldsto the response, ensuring arbitrary fields are preserved through the entire request-config-response lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/handlers/http/alerts.rs (2)
154-191: Consider handling invalid enum parsing more explicitly.The sort function defaults to
AlertState::NotTriggeredandSeverity::Lowwhen parsing fails (lines 162, 174). While this ensures the sort never fails, it silently swallows parsing errors. If an alert summary contains malformed state/severity data, it will be sorted incorrectly without any indication.Consider logging a warning when parsing fails, or verifying that alert summaries always contain valid enum strings.
let state_a = a .get("state") .and_then(|v| v.as_str()) .and_then(|s| { s.parse::<AlertState>().ok().or_else(|| { tracing::warn!("Failed to parse alert state: {}", s); Some(AlertState::NotTriggered) }) }) .unwrap_or(AlertState::NotTriggered);
136-145: Document array/object filtering behavior for complex query parameters.While the current implementation correctly converts arrays/objects to JSON strings for comparison (e.g.,
[1,2,3]), users might expect more flexible matching. For example,?ids=[1, 2, 3](with spaces) won't match[1,2,3](no spaces) due to exact string comparison.Consider adding a comment or documentation explaining that:
- Array/object query parameters must match the exact JSON representation
- Whitespace matters in the comparison
- Alternatively, normalize JSON strings before comparison (e.g., by parsing and re-serializing both sides)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/alerts/alert_structs.rs(7 hunks)src/handlers/http/alerts.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T11:09:21.781Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
Applied to files:
src/alerts/alert_structs.rs
🧬 Code graph analysis (2)
src/handlers/http/alerts.rs (2)
src/alerts/mod.rs (6)
value(415-415)value(423-423)value(431-431)value(444-444)value(455-455)alerts(1329-1333)src/utils/actix.rs (1)
extract_session_key_from_req(51-71)
src/alerts/alert_structs.rs (1)
src/alerts/mod.rs (1)
serde_json(1034-1034)
⏰ 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: coverage
- 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 Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
🔇 Additional comments (8)
src/alerts/alert_structs.rs (4)
27-46: LGTM – Comprehensive reserved field protection.The
RESERVED_FIELDSconstant provides effective protection against field name collisions by covering all critical alert fields. This is a solid defensive measure.
73-101: LGTM – Robust compatibility layer for legacy datetime handling.The custom deserializer gracefully handles empty string timestamps from legacy data while logging warnings. The untagged enum approach correctly prioritizes valid datetime parsing before falling back to string handling.
329-348: LGTM – Validation effectively addresses previous security concerns.The validation logic successfully prevents key collision by checking against
RESERVED_FIELDSand enforces a reasonable 10-field limit to prevent abuse. This addresses the security concerns raised in previous reviews.Based on learnings
427-430: Implementation verified: all three scenarios are handled correctly.The code properly handles the interaction between
defaultanddeserialize_with:
- Missing field →
default_created_timeis invoked, returnsUtc::now()(per serde semantics, default applies only when field absent)- Empty string → custom deserializer processes it, logs warning (lines 91–93), returns
Utc::now()(line 96)- Valid timestamp → deserializer parses successfully (line 100)
The behavior matches the stated expectations. No issues found.
src/handlers/http/alerts.rs (4)
54-120: LGTM – Comprehensive query parameter parsing and validation.The parsing logic correctly validates:
- Empty tags after splitting (lines 79-83)
- Numeric offset and limit (lines 88-91, 95-97)
- Limit bounds (1 to 1000) (lines 100-104)
The separation of reserved vs. non-reserved parameters is clean and maintainable.
122-152: LGTM – Filtering now correctly handles non-string JSON types.The updated filtering logic (lines 136-145) successfully addresses the previous concern about non-string values by:
- Using
.as_str()for JSON strings to avoid quote wrapping- Using
.to_string()for numbers, booleans, arrays, and objectsThis ensures query parameters like
?count=5correctly match JSON numbers, and?active=truematches boolean values.
193-204: LGTM – Clean pagination implementation.The pagination logic correctly applies offset and limit after filtering and sorting, which is the correct order of operations.
206-244: LGTM – Well-structured list handler with clear flow.The refactored handler follows a logical pipeline:
- Parse and validate query parameters
- Fetch user-specific alerts
- Convert to summaries
- Filter by other_fields
- Sort by priority
- Paginate results
The separation of concerns makes the code maintainable and testable.
2 participants
also exclude alert state from loading alerts
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Change