Logging improvements #63
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe recent changes address the sorting issue of logs after a page refresh by adding timestamp fields to log entries, enhancing log parsing, and modifying the log rendering logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LogViewer
participant RemoteServer
participant WebSocket
participant Logger
User->>LogViewer: Refresh page
LogViewer->>WebSocket: Request logs
WebSocket->>RemoteServer: Fetch logs with timestamps
RemoteServer-->>WebSocket: Return logs
WebSocket->>LogViewer: Deliver logs
LogViewer->>Logger: Order logs chronologically
Logger-->>LogViewer: Return ordered logs
LogViewer-->>User: Display ordered logs
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- server/app/query/servers.tsx (3 hunks)
- server/app/query/view/[id]/components.tsx (1 hunks)
- server/app/query/view/[id]/page.tsx (5 hunks)
- sidecar/app/query/base.py (1 hunks)
- sidecar/app/routes/start.py (2 hunks)
Additional context used
Ruff
sidecar/app/query/base.py
18-18: Remove quotes from type annotation
Remove quotes
(UP037)
29-29: Use
X | Yfor type annotationsConvert to
X | Y(UP007)
31-31: Use
X | Yfor type annotationsConvert to
X | Y(UP007)
32-32: Use
X | Yfor type annotationsConvert to
X | Y(UP007)
68-68: Use
X | Yfor type annotationsConvert to
X | Y(UP007)
68-68: Remove quotes from type annotation
Remove quotes
(UP037)
161-164: Use
contextlib.suppress(ValueError)instead oftry-except-passReplace with
contextlib.suppress(ValueError)(SIM105)
Biome
server/app/query/servers.tsx
[error] 33-35: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 155-167: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
server/app/query/view/[id]/page.tsx
[error] 229-229: Using the role attribute 'list' on the 'ul' element is redundant, because it is implied by the semantic 'ul' element.
Unsafe fix: Remove the role attribute.
(lint/a11y/noRedundantRoles)
[error] 38-38: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 140-143: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 188-191: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 236-236: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
Additional comments not posted (5)
server/app/query/view/[id]/components.tsx (1)
91-91: UpdatedLogViewercomponent to handle logs with timestamps and server names.The implementation of the
LogViewercomponent now includes handling for logs with timestamps and server names, aligning with the PR's objectives to enhance log management. The use ofclsxfor conditional styling andsourceCodeProfont is noted, enhancing the visual presentation of logs.
- Correctness: The conversion from timestamp to ISO string (
date.toISOString()) ensures that logs are displayed with a standardized datetime format, which is crucial for consistency across different systems and locales.- Performance: Consider implementing a virtualized list if the number of logs can be very large to improve rendering performance and scroll handling.
- Security: Ensure that any user-generated content within logs is properly sanitized before display to prevent XSS attacks.
Also applies to: 96-129
sidecar/app/routes/start.py (1)
93-123: Introduced new API endpoint for log file retrieval.The new endpoint
get_ipa_helper_log_fileretrieves log files associated with a query ID. This implementation usesStreamingResponseto stream log files, which is efficient for handling potentially large log files.
- Correctness: The endpoint correctly handles cases where the query is not found by returning a
HTTPExceptionwith a 404 status code, which is good practice for REST APIs.- Performance: Streaming log files is a good approach as it avoids loading the entire file into memory. Ensure that file handles are properly closed after streaming.
- Security: Consider implementing rate limiting and authentication for this endpoint to prevent abuse.
sidecar/app/query/base.py (1)
46-46: Enabled serialization of log entries inQueryclass.The serialization of log entries is enabled to improve the structure and queryability of logs. This change aligns with the PR's objectives to enhance logging capabilities.
- Correctness: The filter condition ensures that only logs related to the specific query ID are processed, which is crucial to avoid cluttering the log file with irrelevant entries.
- Performance: Monitor the performance impact of serialization, as it might increase the processing time for each log entry.
- Security: Ensure that sensitive information is not inadvertently logged, especially since serialized logs can be easier to access and manipulate.
server/app/query/servers.tsx (1)
13-13: Enhancements in log handling and WebSockets inserver/app/query/servers.tsx.The addition of a
timestampfield to theServerLoginterface and the newlogURLmethod in theRemoteServerclass support the PR's objectives of improving log management. The WebSocket handling for logs is also enhanced to manage log ordering and retention.
- Correctness: The use of timestamps to order logs is a critical improvement. The handling of WebSocket messages ensures that logs are processed and displayed in real-time.
- Performance: The implementation limits the number of retained logs to 10,000, which is a good practice to prevent memory overflow. However, consider implementing more sophisticated log rotation or archiving strategies for long-term log management.
- Security: Ensure that the WebSocket connection is secure and that appropriate authentication and authorization checks are in place to prevent unauthorized access to logs.
Also applies to: 90-93, 130-168
server/app/query/view/[id]/page.tsx (1)
14-14: State management and UI enhancements inserver/app/query/view/[id]/page.tsx.The introduction of new state variables and functions for handling log filtering and display significantly enhances the user's ability to interact with log data. The use of WebSockets for real-time updates is effectively implemented.
- Correctness: The state management for logs, status, and statistics is well-handled, ensuring that the UI is responsive and accurate.
- Performance: The use of WebSockets for real-time updates is efficient, but ensure that the WebSocket connections are properly managed and closed to avoid memory leaks.
- Security: Implement CSRF protection for the form handling and ensure that all user inputs are validated and sanitized to prevent XSS and SQL injection attacks.
Also applies to: 31-40, 61-77, 96-275
|
wow this is lovely, thank you! |
| @@ -87,6 +90,38 @@ def get_ipa_helper_status( | |||
| return {"status": query.status.name} | |||
|
|
|||
|
|
|||
| @router.get("/{query_id}/log-file") | |||
| def get_ipa_helper_log_file( | |||
There was a problem hiding this comment.
I would recommend to guarantee ordering on the server side. JS operates in my browser and it is busy with other things, so we should delegate hard work to server side. Besides that, it also makes API consistent
There was a problem hiding this comment.
btw can you point me to the file where traces are written out of order? I'd like to take a look and see if we may have an issue on the MPC side
There was a problem hiding this comment.
oh I don't think they should ever be out of order on the server side. they are only out of order when merging the 4 files together. for example a log from Helper 1 might arrive after a log from Helper 2, but have an earlier timestamp.
this is particularly true when we're refreshing later and just consuming the file in it's entirety.
let me point you to where the ordering happens. It should be pretty efficient.
|
|
||
| // only retain last 10,000 logs | ||
| const maxNumLogs = 10000; | ||
| setLogs((prevLogs) => { |
There was a problem hiding this comment.
@akoshelev this is where the ordering happens. if the new log is already the last, it is just appended. if not, it uses lastIndexOf to find the right spot. this should be O(n) and I would imagine starts from the end of the list, where it will most likely find the right position.
the other performance knob here is the max lines kept. I upped it to 10k, but we could lower that now that the download option is available.
There was a problem hiding this comment.
is it server side or client side?
There was a problem hiding this comment.
client side. client connects directly via websocket to logs from every helper.
I don't think it would be possible to do server side while they query is running, because a stale log from one helper could always arrive after other logs have reached the client.
This fixes #45, sorting logs by the timestamp provided by the server.
Addtionally, it addresses #46 allowing for downloading log files from each server.
Finally, it also allows for filtering of displayed logs by each server.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
killfunction anduseEffect.API Updates