Alerts updates #1398
Alerts updates #1398
Conversation
- separated out enums, traits, and structs - added ability to pause alert evals (new endpoints to pause and resume) - added snooze for notifications (time dependent) - removed masking from targets fow now
WalkthroughThis change is a major refactor and extension of the alerting system. It modularizes alert-related code into submodules, introduces new enums and structs for alert configuration and state management, adds notification state handling, updates trait definitions and implementations, and revises HTTP handlers and target logic to align with the new design. Several new endpoints and methods are introduced for pausing, resuming, and snoozing alerts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTPHandler
participant AlertsManager
participant Alert
participant Target
User->>HTTPHandler: PUT /alerts/{id}/pause or /resume
HTTPHandler->>AlertsManager: update_state(alert_id, Paused/NotTriggered)
AlertsManager->>Alert: update_state(alert_state, ...)
Alert-->>AlertsManager: State updated, notification state managed
AlertsManager-->>HTTPHandler: Updated AlertConfig
HTTPHandler-->>User: Response
User->>HTTPHandler: PUT /alerts/{id}/notification_state
HTTPHandler->>AlertsManager: update_notification_state(alert_id, NotificationState)
AlertsManager->>Alert: update_notification_state(new_state)
Alert-->>AlertsManager: Notification state updated
AlertsManager-->>HTTPHandler: Updated AlertConfig
HTTPHandler-->>User: Response
Note over Alert,Target: On alert trigger, Alert calls Target with Context including NotificationConfig
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
parmesant
force-pushed
the
alerts-updates
branch
from
727ab36 to
a8e2053
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 (4)
src/alerts/target.rs (2)
224-264: Consider adding defensive logging for the Paused state.The refactoring to use context-based notification config and the new state handling looks good. However, since the comment indicates that evaluations shouldn't run for paused alerts, consider adding a warning log to help diagnose if this case is unexpectedly hit.
- // do not send out any notifs - // (an eval should not have run!) - AlertState::Paused => {} + // do not send out any notifs + // (an eval should not have run!) + AlertState::Paused => { + warn!("Alert evaluation called for paused alert: {}", context.alert_info.alert_id); + }
574-582: Remove commented out Silenced state handling code.Since the Silenced state has been replaced with Paused, the commented code is no longer needed and should be removed for clarity.
- AlertState::Paused => alert["labels"]["status"] = "paused".into(), // AlertState::Silenced => { - // alert["labels"]["status"] = "silenced".into(); - // alert["annotations"]["reason"] = - // serde_json::Value::String(payload.default_silenced_string()); - // // alert["endsAt"] = Utc::now() - // // .to_rfc3339_opts(chrono::SecondsFormat::Millis, true) - // // .into(); - // } + AlertState::Paused => alert["labels"]["status"] = "paused".into(),src/alerts/alert_structs.rs (2)
85-94: Inconsistent status formatting in message strings.The
default_resolved_stringuses backticks aroundnot-triggeredwhiledefault_paused_stringdoesn't use them aroundpaused. Consider making the formatting consistent.pub(crate) fn default_resolved_string(&self) -> String { - format!("{} is now `not-triggered` ", self.alert_info.alert_name) + format!("{} is now `not-triggered`", self.alert_info.alert_name) } pub(crate) fn default_paused_string(&self) -> String { format!( - "{} is now `paused`. No more evals will be run till it is `paused`.", + "{} is now `paused`. No more evals will be run while it is paused.", self.alert_info.alert_name ) }Also, there's a typo in line 91: "till it is
paused" should probably be "while it is paused" or "until it is resumed".
96-101: Remove commented out code.The
default_silenced_stringmethod is no longer needed since Silenced state has been replaced with Paused.- // fn default_silenced_string(&self) -> String { - // format!( - // "Notifications for {} have been silenced ", - // self.alert_info.alert_name - // ) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/alerts/alert_enums.rs(1 hunks)src/alerts/alert_structs.rs(1 hunks)src/alerts/alert_types.rs(7 hunks)src/alerts/mod.rs(14 hunks)src/alerts/target.rs(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/alerts/alert_enums.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/alerts/alert_types.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.
📚 Learning: in the parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlle...
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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/target.rssrc/alerts/mod.rssrc/alerts/alert_structs.rs
📚 Learning: datafusion's parquet reader defaults to using view types (utf8view, binaryview) when reading parquet...
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:31-41
Timestamp: 2025-06-18T11:15:10.836Z
Learning: DataFusion's parquet reader defaults to using view types (Utf8View, BinaryView) when reading parquet files via the schema_force_view_types configuration (default: true). This means StringViewArray and BinaryViewArray downcasting is required when processing Arrow arrays from DataFusion parquet operations, even though these types are behind nightly feature flags.
Applied to files:
src/alerts/mod.rs
⏰ 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). (9)
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- 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: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (17)
src/alerts/target.rs (5)
39-43: LGTM! Import reorganization looks good.The imports are properly structured and the addition of
CallableTargettrait aligns with the modularization effort.
47-47: Import change aligns with module restructuring.The import of
AlertStateandContextfrom the parent module is consistent with the new modular structure.
266-266: Method signature update is consistent with struct rename.The parameter rename from
target_timeouttonotification_configproperly reflects theTimeouttoNotificationConfigstruct rename.
595-612: Good naming improvement from Timeout to NotificationConfig.The rename better reflects the struct's purpose in managing notification configurations. The default implementation is appropriate.
441-463: Alert state handling in SlackWebHook looks good.The implementation correctly handles all three alert states (Triggered, NotTriggered, Paused) with appropriate message generation.
src/alerts/mod.rs (8)
39-56: Excellent modularization of alert-related code.The separation into
alert_enums,alert_structs, andalert_traitsmodules provides better code organization and maintainability. The public re-exports maintain a clean API surface.
132-136: Migration properly handles new notification fields.The v1 to v2 migration correctly initializes
notification_statetoNotifyandnotification_configto default values, ensuring backward compatibility.
559-567: State mapping correctly updated for new alert states.The migration properly maps "resolved" and unknown states to
NotTriggered, which is a safe default for the new state model.
606-618: Good validation of notification timing configuration.The validation ensures that the total notification duration doesn't exceed the evaluation frequency, preventing notification spam and timing conflicts.
1056-1075: Well-designed alert pause/resume logic.The state transition handling correctly manages scheduled tasks - deleting them when pausing and recreating them when resuming. The order of operations ensures consistency.
1077-1111: New notification state update method properly implemented.The
update_notification_statemethod follows established patterns and enables the snooze feature for notifications without affecting alert evaluation.
1187-1247: Summary generation correctly updated for new alert states.The changes properly handle the Paused state in alert summaries, including sorting by severity and limiting to top 5 alerts per state.
825-832: Error enum updates are appropriate.The simplification of
InvalidQueryParameterand addition ofUnimplementedvariant provide better error handling flexibility.src/alerts/alert_structs.rs (4)
177-219: Well-implemented filter message generation.The
generate_filter_messagemethod provides clear, human-readable descriptions of filter conditions with proper handling of logical operators.
260-286: Robust AlertRequest to AlertConfig conversion.The async
into()method properly validates target IDs and extracts datasets from the query before creating the config. Good error handling and appropriate defaults.
288-309: Complete and well-structured AlertConfig definition.The struct includes all necessary fields for the enhanced alert system, with proper serialization attributes and the new notification-related fields.
311-330: Well-designed summary structures for alert statistics.The summary structs provide comprehensive alert statistics with proper support for the new Paused state and severity-based sorting.
parmesant
force-pushed
the
alerts-updates
branch
from
a8e2053 to
a331597
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/alerts/mod.rs (1)
949-953: Critical issue resolved: Early return bug fixed.This code segment correctly uses
continueinstead ofreturn Ok(())when processing paused alerts, which fixes the critical bug identified in previous reviews where loading would stop after the first paused alert.
🧹 Nitpick comments (1)
src/alerts/alert_structs.rs (1)
89-94: Fix typo in paused string message.Line 91 has a logical error in the message - it says "till it is
paused" but should say "till it isresumed" or "unpaused".- "{} is now `paused`. No more evals will be run till it is `paused`.", + "{} is now `paused`. No more evals will be run till it is `resumed`.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/alerts/alert_enums.rs(1 hunks)src/alerts/alert_structs.rs(1 hunks)src/alerts/alert_types.rs(7 hunks)src/alerts/mod.rs(14 hunks)src/alerts/target.rs(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/alerts/alert_enums.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/alerts/target.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.
📚 Learning: in the parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlle...
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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/mod.rssrc/alerts/alert_types.rssrc/alerts/alert_structs.rs
📚 Learning: datafusion's parquet reader defaults to using view types (utf8view, binaryview) when reading parquet...
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:31-41
Timestamp: 2025-06-18T11:15:10.836Z
Learning: DataFusion's parquet reader defaults to using view types (Utf8View, BinaryView) when reading parquet files via the schema_force_view_types configuration (default: true). This means StringViewArray and BinaryViewArray downcasting is required when processing Arrow arrays from DataFusion parquet operations, even though these types are behind nightly feature flags.
Applied to files:
src/alerts/mod.rs
⏰ 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). (9)
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (18)
src/alerts/alert_types.rs (6)
60-61: LGTM: Notification state fields added appropriately.The addition of
notification_stateandnotification_configfields integrates well with the broader notification management system introduced in this PR.
69-85: LGTM: Improved eval_alert method with message generation.The refactored method now returns an optional message string instead of a boolean/value tuple, which is more semantically clear and integrates well with the MessageCreation trait. The conditional message generation based on threshold evaluation is correct.
109-120: Validation logic correctly updated for centralized notification config.The validation now properly uses the alert's own
notification_configinstead of iterating over target configs, which aligns with the architectural change to centralize notification configuration at the alert level.
279-313: LGTM: MessageCreation trait implementation is well-structured.The trait implementation correctly provides threshold-specific message creation while appropriately rejecting anomaly and forecast message creation for threshold alerts.
328-329: LGTM: From implementations updated for new notification fields.The bidirectional conversion between AlertConfig and ThresholdAlert correctly handles the new notification_state and notification_config fields.
Also applies to: 350-351
158-207: Audit ISO8601 format for Snoozed timestampsThe
update_statemethod useslet till = DateTime::<Utc>::from_str(till_time) .map_err(|e| AlertError::CustomError(e.to_string()))?;which relies on RFC 3339 parsing. Please verify that wherever you construct a
NotificationState::Snoozed(till_time)value, you serialize the timestamp using an RFC 3339 formatter (e.g.to_rfc3339()) so thatfrom_strwill always succeed. Consider adding a unit test exercising the snooze path inupdate_stateto catch any mismatches in format.src/alerts/mod.rs (7)
39-56: LGTM: Excellent modularization of alert system components.The separation into dedicated submodules (alert_enums, alert_structs, alert_traits) greatly improves code organization and maintainability while preserving the public API through re-exports.
132-133: LGTM: Migration correctly adds new notification fields.The migration logic properly initializes new alerts with
NotificationState::Notifyand default notification config, ensuring backward compatibility.
563-566: LGTM: State mapping updated for new alert state semantics.The state extraction correctly maps legacy states to the new enum values, with both "resolved" and unknown states mapping to
NotTriggered.
607-618: LGTM: Validation updated for centralized notification config.The validation logic correctly uses the alert's own notification_config instead of per-target configs, consistent with the architectural changes.
1056-1075: LGTM: Task management properly handles alert state transitions.The logic correctly manages scheduled tasks when alerts are paused (removes task) or resumed (creates task), ensuring proper resource management.
1077-1111: LGTM: New notification state update method is well-implemented.The method follows the same pattern as the main state update method, properly handling different alert types and updating both memory and persistent storage.
1187-1242: LGTM: Summary generation updated for new alert states.The summary correctly uses the new state names (Paused instead of Silenced) and maintains the same severity-based sorting and limiting logic.
src/alerts/alert_structs.rs (5)
40-44: LGTM: Helper struct is appropriately scoped.BasicAlertFields provides a clean way to group common alert fields during migration, improving code clarity.
46-50: LGTM: Alerts struct properly designed for concurrent access.The use of RwLock for the alerts HashMap and mpsc::Sender for task management provides appropriate concurrency control.
177-219: LGTM: Filter message generation is comprehensive.The method properly handles both logical operators (AND/OR) and single conditions, with appropriate null value handling.
260-286: LGTM: AlertRequest conversion is well-structured.The async conversion method properly validates target IDs, resolves dataset names, and initializes all required fields with appropriate defaults.
305-306: LGTM: New notification fields properly integrated.The AlertConfig struct correctly includes the new notification_state and notification_config fields, completing the notification management integration.
Signed-off-by: parmesant <[email protected]>
add serde default Signed-off-by: parmesant <[email protected]>
Added endpoints-
- PATCH /{alertid}/enable -> enable a disabled alert
- PATCH /{alertid}/disable -> disable an alert
- PATCH /{alertid}/update_notification_state -> update the notification state (notify, mute)
- PUT /{alertid}/evaluate_alert -> run manual eval for an alert
- PUT /{alertid} -> modify an alert
parmesant
force-pushed
the
alerts-updates
branch
from
2ddebe1 to
15d8fa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
src/handlers/livetail.rs (1)
283-283: Do not panic on malformed cookies..expect("cookie is parseable") can crash the server if a client sends an invalid Cookie header. Handle parse errors gracefully.
Apply this safer change:
- .map(|value| value.expect("cookie is parseable")) + .filter_map(|res| res.ok())src/handlers/http/audit.rs (1)
49-51: Avoid panics: replace header/JSON unwraps with fallible parsing
to_str().unwrap()andserde_json::from_str(...).unwrap()on untrusted headers can panic and DoS the service. Handle errors gracefully and log at warn level.Suggested change (outside the changed hunk, shown for clarity):
if let Some(kinesis_common_attributes) = req.request().headers().get(KINESIS_COMMON_ATTRIBUTES_KEY) { if let Ok(attribute_value) = kinesis_common_attributes.to_str() { match serde_json::from_str::<Message>(attribute_value) { Ok(message) => { log_builder = log_builder.with_stream(message.common_attributes.x_p_stream); } Err(e) => { tracing::warn!( "invalid {} header JSON: {e}", KINESIS_COMMON_ATTRIBUTES_KEY ); } } } else { tracing::warn!("non-UTF8 {} header", KINESIS_COMMON_ATTRIBUTES_KEY); } }
🧹 Nitpick comments (7)
src/handlers/livetail.rs (1)
278-289: Avoid intermediate Vec allocation; stream the search.You can remove the collect and find directly on the iterator to reduce allocations and improve performance.
Apply this refactor:
- let cookies = header.get_all("cookie"); - let cookies: Vec<_> = cookies - .iter() - .filter_map(|value| value.to_str().ok()) - .flat_map(Cookie::split_parse) - .filter_map(|res| res.ok()) - .collect(); - - cookies - .into_iter() - .find(|cookie| cookie.name() == SESSION_COOKIE_NAME) + header + .get_all("cookie") + .iter() + .filter_map(|value| value.to_str().ok()) + .flat_map(Cookie::split_parse) + .filter_map(|res| res.ok()) + .find(|cookie| cookie.name() == SESSION_COOKIE_NAME)src/about.rs (1)
155-155: Typo in comment: “Cargol.toml” → “Cargo.toml”Fix the filename spelling in the comment.
- // CARGO_PKG_VERSION is set from Cargol.toml file at build time + // CARGO_PKG_VERSION is set from Cargo.toml file at build timesrc/migration/metadata_migration.rs (1)
79-81: Nit: typo in panic message"arrray" → "array".
- panic!("privileges is an arrray") + panic!("privileges is an array")src/catalog/mod.rs (1)
295-296: Nit: variable name typo"mainfest_file_name" → "manifest_file_name".
- let mainfest_file_name = manifest_path("").to_string(); - let path = partition_path(stream_name, lower_bound, upper_bound).join(&mainfest_file_name); + let manifest_file_name = manifest_path("").to_string(); + let path = partition_path(stream_name, lower_bound, upper_bound).join(&manifest_file_name);src/handlers/http/modal/query/querier_logstream.rs (1)
39-41: Nit: function name typo in import
merge_quried_statslooks misspelled. Consider renaming tomerge_queried_statsacross the codebase.I can open a follow-up PR to rename and update call sites.
src/query/stream_schema_provider.rs (1)
175-185: Nit: function name typo
get_hottier_exectuion_plancontains typos (“hottier”, “exectuion”). Consider renaming toget_hot_tier_execution_planfor clarity.I can generate a patch and a script to update all references safely.
src/alerts/alerts_utils.rs (1)
277-293: Avoid sending empty notification messages
update_alert_stateusesSome("".into())to mark a resolution path.
Becausetrigger_notif.is_some()is later checked, this still fires a notification – but with an empty body.Consider using
Nonefor “no message” or provide a meaningful “resolved” message instead, e.g.:- .update_state(*alert.get_id(), AlertState::NotTriggered, Some("".into())) + .update_state(*alert.get_id(), AlertState::NotTriggered, None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/about.rs(1 hunks)src/alerts/alert_enums.rs(1 hunks)src/alerts/alert_structs.rs(1 hunks)src/alerts/alert_traits.rs(3 hunks)src/alerts/alert_types.rs(8 hunks)src/alerts/alerts_utils.rs(9 hunks)src/alerts/mod.rs(16 hunks)src/alerts/target.rs(9 hunks)src/catalog/manifest.rs(1 hunks)src/catalog/mod.rs(2 hunks)src/event/format/json.rs(1 hunks)src/handlers/http/alerts.rs(5 hunks)src/handlers/http/audit.rs(1 hunks)src/handlers/http/cluster/mod.rs(2 hunks)src/handlers/http/logstream.rs(1 hunks)src/handlers/http/modal/query/querier_logstream.rs(1 hunks)src/handlers/http/modal/server.rs(2 hunks)src/handlers/http/modal/ssl_acceptor.rs(1 hunks)src/handlers/http/modal/utils/ingest_utils.rs(1 hunks)src/handlers/http/query.rs(1 hunks)src/handlers/livetail.rs(1 hunks)src/hottier.rs(1 hunks)src/livetail.rs(1 hunks)src/metadata.rs(1 hunks)src/migration/metadata_migration.rs(3 hunks)src/otel/otel_utils.rs(1 hunks)src/parseable/streams.rs(2 hunks)src/query/mod.rs(2 hunks)src/query/stream_schema_provider.rs(1 hunks)src/stats.rs(2 hunks)src/storage/object_storage.rs(4 hunks)
✅ Files skipped from review due to trivial changes (8)
- src/handlers/http/modal/ssl_acceptor.rs
- src/query/mod.rs
- src/livetail.rs
- src/handlers/http/cluster/mod.rs
- src/metadata.rs
- src/event/format/json.rs
- src/hottier.rs
- src/parseable/streams.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/handlers/http/modal/server.rs
- src/alerts/target.rs
- src/storage/object_storage.rs
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.
📚 Learning: 2025-02-14T09:49:25.818Z
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
Applied to files:
src/handlers/http/modal/query/querier_logstream.rssrc/handlers/http/logstream.rssrc/query/stream_schema_provider.rssrc/catalog/mod.rssrc/stats.rssrc/handlers/http/audit.rs
📚 Learning: 2025-06-16T02:04:58.990Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/handlers/http/health_check.rs:81-90
Timestamp: 2025-06-16T02:04:58.990Z
Learning: In the shutdown function in src/handlers/http/health_check.rs, the design approach is to log errors from sync operations rather than propagate them. This is intentional because the shutdown function is called on SIGTERM/SIGINT signals, and the goal is to perform best-effort cleanup (syncing pending files to object storage) while allowing the shutdown to proceed regardless of sync failures. Logging provides debugging information without blocking the shutdown process.
Applied to files:
src/handlers/http/modal/query/querier_logstream.rssrc/handlers/http/logstream.rs
📚 Learning: 2025-06-16T09:50:38.636Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:319-331
Timestamp: 2025-06-16T09:50:38.636Z
Learning: In Parseable's Ingest or Query mode, the node_id is always available because it's generated during server initialization itself, before the get_node_id_string() function in streams.rs would be called. This makes the .expect() calls on QUERIER_META.get() and INGESTOR_META.get() safe in this context.
Applied to files:
src/handlers/http/modal/query/querier_logstream.rssrc/catalog/mod.rs
📚 Learning: 2025-07-28T17:10:39.448Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1392
File: src/migration/stream_metadata_migration.rs:303-322
Timestamp: 2025-07-28T17:10:39.448Z
Learning: In Parseable's migration system (src/migration/stream_metadata_migration.rs), each migration function updates the metadata to the current latest format using CURRENT_OBJECT_STORE_VERSION and CURRENT_SCHEMA_VERSION constants, rather than producing incremental versions. For example, v5_v6 function produces v7 format output when these constants are set to "v7", not v6 format.
Applied to files:
src/catalog/mod.rssrc/migration/metadata_migration.rs
📚 Learning: 2025-06-16T05:20:18.593Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:351-355
Timestamp: 2025-06-16T05:20:18.593Z
Learning: In the Parseable codebase, arrow files are expected to always have valid creation or modified timestamps as a basic system assumption. The conversion flow uses expect() on file metadata operations to enforce this invariant with fail-fast behavior rather than graceful error handling, as violations represent fundamental system issues that should cause immediate failures.
Applied to files:
src/catalog/mod.rs
📚 Learning: 2025-06-18T06:45:37.070Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:832-843
Timestamp: 2025-06-18T06:45:37.070Z
Learning: Stats calculation for parquet files in Parseable is done synchronously during the upload process because files are deleted from staging after upload. This prevents race conditions and ensures stats are calculated while files are still available locally.
Applied to files:
src/stats.rs
📚 Learning: 2025-05-01T10:27:56.858Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Applied to files:
src/handlers/http/query.rs
📚 Learning: 2025-06-18T12:44:31.983Z
Learnt from: parmesant
PR: parseablehq/parseable#1347
File: src/handlers/http/query.rs:0-0
Timestamp: 2025-06-18T12:44:31.983Z
Learning: The counts API in src/handlers/http/query.rs does not currently support group_by functionality in COUNT queries, so the hard-coded fields array ["start_time", "end_time", "count"] is appropriate for the current scope.
Applied to files:
src/handlers/http/query.rssrc/alerts/alert_types.rssrc/alerts/alerts_utils.rssrc/alerts/alert_structs.rs
📚 Learning: 2025-07-24T11:09:21.781Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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_traits.rssrc/alerts/alert_enums.rssrc/alerts/alert_types.rssrc/alerts/alerts_utils.rssrc/handlers/http/alerts.rssrc/alerts/mod.rssrc/alerts/alert_structs.rs
📚 Learning: 2025-06-18T06:39:04.775Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
Applied to files:
src/alerts/alerts_utils.rssrc/alerts/mod.rs
⏰ 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). (9)
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (20)
src/handlers/livetail.rs (1)
276-276: Explicit lifetime on Cookie return looks correct.Returning Option<Cookie<'_>> ties the cookie to the request metadata and avoids unnecessary owning conversions. This aligns with the borrowing model used elsewhere.
src/otel/otel_utils.rs (1)
172-176: Let-chain refactor is correct and keeps NaN/inf filtered outLogic remains unchanged and is concise. Please ensure the MSRV supports let-chains.
You can reuse the MSRV check script from the previous comment.
src/catalog/manifest.rs (1)
115-121: Refactor is equivalent; confirm desired “vacuous truth” behaviorThe chain correctly sets
sort_order_idwhen all row-group sort orders match the last popped one. Note this also holds for a single row group (remainder is empty,.all(...)is true). If that is intended, this is good to go. Otherwise, guard for length > 1.Would you like a focused unit test covering:
- 1 row group (sets sort order)
- N>1 identical sort orders (sets)
- mixed sort orders (does not set)?
src/handlers/http/audit.rs (1)
54-58: Header parsing let-chain looks goodConcise and equivalent to the previous nested form.
src/stats.rs (2)
163-167: Let-chain for conditional stats persistence is correctSemantics preserved: only logs when stats exist and
put_statsfails.Ensure MSRV supports let-chains (reuse the script from earlier).
212-214: Let-chain for metric removal keeps prior semanticsShort-circuits on the prefix check and only warns on actual removal errors.
src/migration/metadata_migration.rs (2)
188-192: LGTM: concise nested object matchThe chained pattern cleanly ensures both "permission" and "resource" are JSON objects before removing "tag". Behavior unchanged.
88-92: MSRV Already Covers Let-Chains
The repository’s Cargo.toml setsrust-version = "1.88.0"(edition 2024), and let-chains have been stable since Rust 1.65. No further action needed—the current toolchain already supports these constructs.Likely an incorrect or invalid review comment.
src/catalog/mod.rs (3)
271-287: Logic preserved; consider MSRV for let-chainsCombining the checks reads better and avoids an extra unwrap. Ensure repo MSRV supports let-chains.
(See MSRV verification script posted in metadata_migration.rs comment.)
288-291: LGTM: error handling preserved when setting first_event_atOn failure to update streaminfo, you log and proceed. Matches existing behavior.
369-371: LGTM: simpler Option handlingReplacing is_some/unwrap with
if let Some(event)is clearer and safer.src/handlers/http/modal/utils/ingest_utils.rs (2)
177-181: MSRV note: let-chains in header extractionThe chained condition (
starts_with&&!IGNORE_HEADERS&&let Ok(value) = ...) relies on let-chains. Ensure toolchain compatibility.(See MSRV verification script posted in metadata_migration.rs comment.)
202-206: LGTM: LOG_SOURCE_KEY propagated to FORMAT_KEYCleanly maps the header when present and valid UTF-8.
src/handlers/http/modal/query/querier_logstream.rs (1)
79-83: LGTM: concise hot tier guardCombining the global manager fetch and existence check is clear and preserves behavior.
Note: uses let-chains; ensure MSRV compatibility (see verification script in metadata_migration.rs).
src/query/stream_schema_provider.rs (1)
566-579: LGTM: hot tier execution plan gated via let-chainThe refactor reads well and behavior is unchanged. Ensure MSRV for let-chains.
(See MSRV verification script posted in metadata_migration.rs comment.)
src/handlers/http/query.rs (1)
349-350: Nice micro-optimisationSwitching to
std::slice::from_ref(&body.stream)removes an unnecessaryclone()and is clearer. 👍src/alerts/alert_traits.rs (1)
32-46: Interfaces look solidThe new
MessageCreationabstraction cleanly separates message formatting from evaluation logic and keeps the trait object-safe. No concerns here.src/handlers/http/alerts.rs (1)
95-102: Clarify notification-retry calculationWhen
eval_freq < notif_freq, integer division yields 0 and you forcetimes = 1. Consider rejecting this configuration instead of silently coercing it – it likely indicates a mis-configured alert.[ suggest_optional_refactor ]
src/alerts/mod.rs (1)
1018-1023: Early-return bug fixed – looks goodThe previous
return Ok(())that aborted loading on the first disabled alert has been replaced withcontinue, ensuring the rest of the alerts are processed.src/alerts/alert_structs.rs (1)
178-207: Potential panic ingenerate_filter_messageWhen
operatorisSome, the code indexescondition_config[1]but never checks that at least two conditions exist. An empty or single-element array will cause a panic.Add a length check:
-let expr1 = &self.condition_config[0]; -let expr2 = &self.condition_config[1]; +if self.condition_config.len() < 2 { + return "<invalid filter config>".to_string(); +} +let expr1 = &self.condition_config[0]; +let expr2 = &self.condition_config[1];
| if let Some(latest_release) = latest_release | ||
| && latest_release.version > current_version | ||
| { | ||
| print_latest_release(latest_release); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Let-chain refactor looks correct; confirm MSRV supports it
The combined if let ... && ... is equivalent to the previous nested checks. Please confirm our MSRV/toolchain supports let-chains.
Run this to check declared toolchain/MSRV:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==> rust-toolchain files (if any)"
fd -a -H -t f 'rust-toolchain*' | while read -r f; do
echo "----- $f"
cat "$f"
done
echo "==> rust-version declared in Cargo.toml files"
fd -a -H -t f Cargo.toml | while read -r f; do
echo "----- $f"
rg -n '^\s*rust-version\s*=\s*"(\d+\.\d+(\.\d+)?)"' "$f" || true
doneLength of output: 485
MSRV supports let-chains; please fix the typo in the Cargo.toml reference
Our Cargo.toml declares rust-version = "1.88.0", and let-chains have been stable since Rust 1.65.0, so the refactor is safe under the MSRV.
Please address the remaining typo:
• In src/about.rs (around lines 115–119), correct the misspelled reference to “Cargo.toml” in the nearby comment.
🤖 Prompt for AI Agents
In src/about.rs around lines 115 to 119, there is a typo in the comment
referencing "Cargo.toml". Locate the comment near these lines and correct the
spelling of "Cargo.toml" to ensure it is accurate and consistent. This fix is
safe as let-chains are supported by the declared Rust version.
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| NotificationState::Notify => write!(f, "notify"), | ||
| NotificationState::Mute(till_time) => { | ||
| let till = match till_time.as_str() { | ||
| "indefinite" => DateTime::<Utc>::MAX_UTC.to_rfc3339(), | ||
| _ => DateTime::<Utc>::from_str(till_time) | ||
| .map_err(|e| std::fmt::Error::custom(e.to_string()))? | ||
| .to_rfc3339(), | ||
| }; |
There was a problem hiding this comment.
Compilation error in Display implementation
fmt::Error has no custom constructor; the ? operator therefore cannot convert chrono::ParseError into fmt::Error.
- _ => DateTime::<Utc>::from_str(till_time)
- .map_err(|e| std::fmt::Error::custom(e.to_string()))?
+ _ => DateTime::<Utc>::from_str(till_time)
+ .map_err(|_| fmt::Error)?Also, the use serde::ser::Error; import is now unused and can be removed.
🤖 Prompt for AI Agents
In src/alerts/alert_enums.rs around lines 270 to 279, the Display implementation
incorrectly tries to use fmt::Error::custom, which does not exist, causing a
compilation error. Replace the error mapping to convert the chrono::ParseError
into a plain fmt::Error by returning Err(fmt::Error) directly instead of using
custom. Also, remove the unused import of serde::ser::Error from the file to
clean up unused code.
| target::Retry::Infinite => {} | ||
| target::Retry::Finite(repeat) => { | ||
| let notif_duration = | ||
| Duration::from_secs(60 * self.notification_config.interval) * *repeat as u32; | ||
| if (notif_duration.as_secs_f64()).gt(&((eval_frequency * 60) as f64)) { | ||
| return Err(AlertError::Metadata( | ||
| "evalFrequency should be greater than target repetition interval", |
There was a problem hiding this comment.
std::time::Duration cannot be multiplied directly
Duration * u32 is not implemented. The line will not compile:
let notif_duration =
Duration::from_secs(60 * self.notification_config.interval) * *repeat as u32;Rewrite to compute seconds first:
-let notif_duration =
- Duration::from_secs(60 * self.notification_config.interval) * *repeat as u32;
+let notif_duration = Duration::from_secs(
+ 60 * self.notification_config.interval * (*repeat as u64),
+);🤖 Prompt for AI Agents
In src/alerts/alert_types.rs around lines 128 to 134, the code attempts to
multiply a Duration by a u32, which is not supported. To fix this, calculate the
total seconds as a u32 by multiplying 60, the notification interval, and the
repeat count first, then create the Duration from that total seconds value using
Duration::from_secs.
| /// Extract numeric value from an Arrow array at the given row index | ||
| fn extract_numeric_value(column: &dyn Array, row_index: usize) -> f64 { | ||
| if let Some(float_array) = column.as_any().downcast_ref::<Float64Array>() { | ||
| if !float_array.is_null(row_index) { | ||
| return float_array.value(row_index); | ||
| } | ||
| } else if let Some(int_array) = column.as_any().downcast_ref::<Int64Array>() | ||
| && !int_array.is_null(row_index) | ||
| { | ||
| return int_array.value(row_index) as f64; | ||
| } | ||
| 0.0 | ||
| } |
There was a problem hiding this comment.
Fix compile-error caused by if let … && … pattern
if let Some(arr) = … && guard is not valid Rust syntax (the “extra &&” form is still unstable).
Both extract_numeric_value and extract_string_value use it and will prevent the crate from compiling.
-} else if let Some(int_array) = column.as_any().downcast_ref::<Int64Array>()
- && !int_array.is_null(row_index)
-{
- return int_array.value(row_index) as f64;
+} else if let Some(int_array) = column.as_any().downcast_ref::<Int64Array>() {
+ if !int_array.is_null(row_index) {
+ return int_array.value(row_index) as f64;
+ }
}Apply the same pattern to extract_string_value.
Without this fix the whole project will fail to build.
Also applies to: 236-252
🤖 Prompt for AI Agents
In src/alerts/alerts_utils.rs around lines 221 to 233 and 236 to 252, the use of
`if let Some(arr) = ... && guard` is causing a compile error because this
pattern is not stable in Rust. To fix this, separate the pattern matching and
the guard condition into nested if statements: first use `if let Some(arr) =
...` and then inside it check the guard condition with a separate `if`
statement. Apply this fix to both `extract_numeric_value` and
`extract_string_value` functions to ensure the crate compiles successfully.
| _ => { | ||
| // Use inputs() method to get all input plans | ||
| plan.inputs() | ||
| .iter() | ||
| .any(|input| is_logical_plan_aggregate(input)) | ||
| .map(|input| _get_number_of_agg_exprs(input)) | ||
| .sum() | ||
| } |
There was a problem hiding this comment.
Iterator::sum() over Result values does not compile
_get_number_of_agg_exprs tries to sum() an iterator of Result<usize, _>, but Result does not implement Sum.
plan.inputs()
.iter()
.map(|input| _get_number_of_agg_exprs(input))
.sum()Refactor with try_fold (propagates the first error):
-let total = plan.inputs()
- .iter()
- .map(|input| _get_number_of_agg_exprs(input))
- .sum();
-Ok(total)
+plan.inputs()
+ .iter()
+ .try_fold(0usize, |acc, input| {
+ _get_number_of_agg_exprs(input).map(|n| acc + n)
+ })🤖 Prompt for AI Agents
In src/alerts/mod.rs around lines 845 to 851, the code attempts to sum an
iterator of Result values using Iterator::sum(), which does not compile because
Result does not implement Sum. To fix this, replace the sum() call with try_fold
to iterate over the results, accumulate the usize values on success, and
propagate the first error encountered. This change will correctly handle the
Result types and maintain error propagation.
| if let Some(hot_tier_manager) = HotTierManager::global() | ||
| && hot_tier_manager.check_stream_hot_tier_exists(&stream_name) | ||
| { | ||
| hot_tier_manager.delete_hot_tier(&stream_name).await?; |
There was a problem hiding this comment.
Fix compile-blocking if let … && guard
if let Some(hot_tier_manager) = … && … is not valid Rust syntax; the guard must be a separate if. This will fail to compile.
- if let Some(hot_tier_manager) = HotTierManager::global()
- && hot_tier_manager.check_stream_hot_tier_exists(&stream_name)
- {
- hot_tier_manager.delete_hot_tier(&stream_name).await?;
- }
+ if let Some(hot_tier_manager) = HotTierManager::global() {
+ if hot_tier_manager.check_stream_hot_tier_exists(&stream_name) {
+ hot_tier_manager.delete_hot_tier(&stream_name).await?;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(hot_tier_manager) = HotTierManager::global() | |
| && hot_tier_manager.check_stream_hot_tier_exists(&stream_name) | |
| { | |
| hot_tier_manager.delete_hot_tier(&stream_name).await?; | |
| if let Some(hot_tier_manager) = HotTierManager::global() { | |
| if hot_tier_manager.check_stream_hot_tier_exists(&stream_name) { | |
| hot_tier_manager.delete_hot_tier(&stream_name).await?; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/handlers/http/logstream.rs around lines 71 to 74, the use of `if let
Some(hot_tier_manager) = ... && ...` is invalid Rust syntax and causes a compile
error. To fix this, separate the conditions by first using `if let
Some(hot_tier_manager) = HotTierManager::global()` and then nest an additional
`if` statement to check
`hot_tier_manager.check_stream_hot_tier_exists(&stream_name)`. This ensures
valid syntax and correct conditional logic.
| if header_name.starts_with("x-p-") | ||
| && !IGNORE_HEADERS.contains(&header_name) | ||
| && let Ok(value) = header_value.to_str() | ||
| { | ||
| let key = header_name.trim_start_matches("x-p-"); | ||
| if !key.is_empty() { | ||
| // Truncate value if it exceeds the maximum length | ||
| let truncated_value = if value.len() > MAX_FIELD_VALUE_LENGTH { | ||
| warn!( | ||
| "Ignoring header with empty key after prefix: {}", | ||
| "Header value for '{}' exceeds maximum length, truncating", | ||
| header_name | ||
| ); | ||
| } | ||
| &value[..MAX_FIELD_VALUE_LENGTH] | ||
| } else { | ||
| value | ||
| }; | ||
| p_custom_fields.insert(key.to_string(), truncated_value.to_string()); | ||
| } else { | ||
| warn!( | ||
| "Ignoring header with empty key after prefix: {}", | ||
| header_name | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe UTF-8 slicing on header truncation; fix to avoid panics
&value[..MAX_FIELD_VALUE_LENGTH] can panic if the cut is not on a UTF-8 boundary (non-ASCII headers). Use char-based truncation to keep semantics safe.
- let key = header_name.trim_start_matches("x-p-");
+ let key = header_name.trim_start_matches("x-p-");
if !key.is_empty() {
- // Truncate value if it exceeds the maximum length
- let truncated_value = if value.len() > MAX_FIELD_VALUE_LENGTH {
- warn!(
- "Header value for '{}' exceeds maximum length, truncating",
- header_name
- );
- &value[..MAX_FIELD_VALUE_LENGTH]
- } else {
- value
- };
- p_custom_fields.insert(key.to_string(), truncated_value.to_string());
+ // Truncate by characters to avoid invalid UTF-8 boundaries
+ let truncated_value: String = {
+ let char_count = value.chars().count();
+ if char_count > MAX_FIELD_VALUE_LENGTH {
+ warn!(
+ "Header value for '{}' exceeds maximum length, truncating",
+ header_name
+ );
+ value.chars().take(MAX_FIELD_VALUE_LENGTH).collect()
+ } else {
+ value.to_string()
+ }
+ };
+ p_custom_fields.insert(key.to_string(), truncated_value);
} else {If you intended MAX_FIELD_VALUE_LENGTH to be bytes, I can adjust this to a safe byte-based truncation that respects UTF-8 boundaries using .char_indices().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if header_name.starts_with("x-p-") | |
| && !IGNORE_HEADERS.contains(&header_name) | |
| && let Ok(value) = header_value.to_str() | |
| { | |
| let key = header_name.trim_start_matches("x-p-"); | |
| if !key.is_empty() { | |
| // Truncate value if it exceeds the maximum length | |
| let truncated_value = if value.len() > MAX_FIELD_VALUE_LENGTH { | |
| warn!( | |
| "Ignoring header with empty key after prefix: {}", | |
| "Header value for '{}' exceeds maximum length, truncating", | |
| header_name | |
| ); | |
| } | |
| &value[..MAX_FIELD_VALUE_LENGTH] | |
| } else { | |
| value | |
| }; | |
| p_custom_fields.insert(key.to_string(), truncated_value.to_string()); | |
| } else { | |
| warn!( | |
| "Ignoring header with empty key after prefix: {}", | |
| header_name | |
| ); | |
| } | |
| } | |
| if header_name.starts_with("x-p-") | |
| && !IGNORE_HEADERS.contains(&header_name) | |
| && let Ok(value) = header_value.to_str() | |
| { | |
| let key = header_name.trim_start_matches("x-p-"); | |
| if !key.is_empty() { | |
| // Truncate by characters to avoid invalid UTF-8 boundaries | |
| let truncated_value: String = { | |
| let char_count = value.chars().count(); | |
| if char_count > MAX_FIELD_VALUE_LENGTH { | |
| warn!( | |
| "Header value for '{}' exceeds maximum length, truncating", | |
| header_name | |
| ); | |
| value.chars().take(MAX_FIELD_VALUE_LENGTH).collect() | |
| } else { | |
| value.to_string() | |
| } | |
| }; | |
| p_custom_fields.insert(key.to_string(), truncated_value); | |
| } else { | |
| warn!( | |
| "Ignoring header with empty key after prefix: {}", | |
| header_name | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/handlers/http/modal/utils/ingest_utils.rs lines 177 to 200, the code
slices the header value string using byte indices which can cause panics if the
slice does not align with UTF-8 character boundaries. To fix this, replace the
byte-based slicing with a safe truncation method that respects UTF-8 boundaries,
such as using char-based truncation or iterating with .char_indices() to find
the correct cut-off point before MAX_FIELD_VALUE_LENGTH bytes, then slice safely
without risking invalid UTF-8.
| if let Some(value) = privilege_value | ||
| && value.as_str().unwrap() == "ingester" | ||
| { | ||
| *value = JsonValue::String("ingestor".to_string()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unwrap on JSON string; use a guard instead
Current code will panic if "privilege" exists but is not a string. Preserve behavior while avoiding panic:
- if let Some(value) = privilege_value
- && value.as_str().unwrap() == "ingester"
- {
+ if let Some(value) = privilege_value
+ && matches!(value.as_str(), Some("ingester"))
+ {
*value = JsonValue::String("ingestor".to_string());
}🤖 Prompt for AI Agents
In src/migration/metadata_migration.rs around lines 88 to 92, the code uses
unwrap on a JSON value assuming it is a string, which can cause a panic if the
value is not a string. To fix this, replace the unwrap with a guard that checks
if the value is a string before comparing it to "ingester". Only perform the
replacement if the value is confirmed to be a string, thus preserving behavior
without risking a panic.
| if let Some(value) = privilege_value | ||
| && value.as_str().unwrap() == "ingester" | ||
| { | ||
| *value = JsonValue::String("ingestor".to_string()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same unwrap risk here; apply the safer guard
Mirror the safer pattern from v2_v3 to avoid panic if the JSON value isn't a string.
- if let Some(value) = privilege_value
- && value.as_str().unwrap() == "ingester"
- {
+ if let Some(value) = privilege_value
+ && matches!(value.as_str(), Some("ingester"))
+ {
*value = JsonValue::String("ingestor".to_string());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(value) = privilege_value | |
| && value.as_str().unwrap() == "ingester" | |
| { | |
| *value = JsonValue::String("ingestor".to_string()); | |
| } | |
| if let Some(value) = privilege_value | |
| && matches!(value.as_str(), Some("ingester")) | |
| { | |
| *value = JsonValue::String("ingestor".to_string()); | |
| } |
🤖 Prompt for AI Agents
In src/migration/metadata_migration.rs around lines 127 to 131, the code uses
unwrap on a JSON value without checking its type, risking a panic if the value
is not a string. Replace the unwrap with a safer pattern by using if let
Some(value) = privilege_value and then matching or using if let Some(str_val) =
value.as_str() to safely check the string content before modifying it. This
mirrors the safer guard pattern used in v2_v3 to prevent panics.
Description
This PR has:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
API Changes