-
-
Notifications
You must be signed in to change notification settings - Fork 148
updates for prism home api #1275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
updates for prism home api #1275
Conversation
WalkthroughThe pull request refines asynchronous operations and locking mechanisms across three modules. In the correlation module, a write lock is now acquired via a guard variable to manage insertions safely. In the home module, multiple asynchronous fetch operations and metadata collection tasks are now executed concurrently using Changes
Sequence Diagram(s)sequenceDiagram
participant GH as generate_home_response
participant TS as get_stream_titles
participant AS as get_alert_titles
participant CS as get_correlation_titles
participant DS as get_dashboard_titles
participant FS as get_filter_titles
participant MD as get_stream_metadata
participant SD as stats_for_date
GH->>+TS: Fetch stream titles
GH->>+AS: Fetch alert titles
GH->>+CS: Fetch correlation titles
GH->>+DS: Fetch dashboard titles
GH->>+FS: Fetch filter titles
Note right of GH: Initiates concurrent calls using tokio::join!
TS-->>-GH: Return list of streams
AS-->>-GH: Return list of alerts
CS-->>-GH: Return correlation titles
DS-->>-GH: Return dashboard titles
FS-->>-GH: Return filter titles
alt Process each stream concurrently
GH->>+MD: Request stream metadata (for each stream)
MD-->>-GH: Return metadata tuple
end
alt Concurrent date statistics
GH->>+SD: Fetch stats for a date (for each stream)
SD-->>-GH: Return statistics
end
sequenceDiagram
participant PL as get_prism_logstream_info
participant SI as get_stream_info
participant SC as get_schema
participant ST as get_stats
PL->>+SI: Fetch stream info concurrently
PL->>+SC: Fetch stream schema concurrently
PL->>+ST: Fetch stream stats concurrently
Note right of PL: Using tokio::join! for parallel execution
SI-->>-PL: Return stream info
SC-->>-PL: Return stream schema
ST-->>-PL: Return stats
PL->>PL: Construct PrismLogstreamInfo object
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/prism/logstream/mod.rs (1)
63-71
: Efficient concurrent fetching withtokio::join!
.
Replacing sequential async calls withtokio::join!
improves throughput for these I/O-bound tasks. If an early failure should abort remaining calls, considertry_join!
instead. Otherwise, this approach is valid.src/prism/home/mod.rs (1)
242-259
: Handling null dashboard IDs.
Dashboard IDs are unwrapped withok_or_else(...)
, which will fail if an ID is missing. This is valid if the system must strictly enforce ID presence. If partial loading is expected, consider skipping incomplete dashboard entries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/correlation.rs
(2 hunks)src/prism/home/mod.rs
(7 hunks)src/prism/logstream/mod.rs
(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/prism/logstream/mod.rs (1)
src/logstream/mod.rs (2)
get_stream_info_helper
(47-90)get_stream_schema_helper
(28-45)
src/prism/home/mod.rs (2)
src/alerts/mod.rs (1)
get_alerts_info
(893-930)src/handlers/http/logstream.rs (1)
get_stats_date
(201-223)
⏰ Context from checks skipped due to timeout of 90000ms (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 Default x86_64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (11)
src/correlation.rs (2)
59-60
: Consider scoping or early release of write lock.
Acquiring the write lock and storing it inguard
is appropriate for ensuring safe concurrent writes. If additional operations beyond insertion are needed, consider using a narrower scope or carefully releasing the lock after insertion to reduce the chance of blocking other writers.
71-71
: Correct usage of guarded insertion.
Usingguard.insert(...)
while holding the write lock ensures atomic insertion. This looks good for preventing data races.src/prism/home/mod.rs (9)
44-45
: Clearer type alias.
DefiningStreamMetadataResponse
as a type alias for anResult<..., ...>
simplifies readability and emphasizes the context of the returned data.
93-192
: Parallel fetch for home response.
Usingtokio::join!
to concurrently retrieve stream titles, alert titles, correlation titles, dashboards, filter titles, and alerts info is a strong step for efficiency. The code bails out early if any of these fail, which may be desirable. If you wish to collect partial results on error, consider manual error handling or a different concurrency strategy.
196-213
: Stream listing with RBAC filtering.
Filtering the listed streams by RBAC checks ensures proper authorization. If the number of streams grows large, consider streaming them or adding pagination.
214-226
: Retrieving alert titles.
Mapping alerts toTitleAndId
for display is straightforward. The approach looks good for authorized user access of alerts.
228-240
: Fetching correlation titles by user.
Listing correlations with matching user permissions appears correct. The code is consistent with how other titles are fetched; no issues found.
261-278
: Filter titles retrieval.
Similar to dashboards, this function unwraps thefilter_id
. The partial approach is consistent. No immediate concerns.
280-325
: Aggregating stream metadata.
Reading multiple.json
objects from storage, logging parse errors, and continuing allows partial success. This is good for resiliency. If desired, consider collecting parse errors in a single report for troubleshooting.
327-361
: Generating date-based stats.
Stats are fetched concurrently for each stream on each date. Any failures are logged and skipped. This non-blocking approach helps ensure partial success even if some streams fail.
362-375
: Consolidating querier and ingestor stats.
Combining stats fromget_stats_date
andfetch_daily_stats_from_ingestors
is coherent for building a comprehensive view. Implementation looks solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to merge
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
Refactor
New Features