Skip to content

Comments

more error logs and return correct msg#1219

Merged
giurgiur99 merged 2 commits intomainfrom
767-add-error-logs-for-streamable-logs-command
Feb 24, 2026
Merged

more error logs and return correct msg#1219
giurgiur99 merged 2 commits intomainfrom
767-add-error-logs-for-streamable-logs-command

Conversation

@giurgiur99
Copy link
Contributor

@giurgiur99 giurgiur99 commented Feb 23, 2026

Fixes #767

Changes proposed in this PR:

  • Added more error logs and parse error msgs
image

@giurgiur99
Copy link
Contributor Author

/run-security-scan

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This pull request significantly improves error handling, logging, and client-facing error messages for the getStreamableLogs functionality. It introduces more informative error responses for 404 and 500 status codes in the command handler and the HTTP route. Additionally, robust error logging is added in the compute_engine_docker.ts component and the getStreamableLogs.ts command handler. Unit tests for the ComputeGetStreamableLogsHandler validation have also been added.

Comments:
• [INFO][other] Good addition of an error log here. It helps with debugging when the streamable logs command fails within the C2D engine, providing context about the specific job.
• [INFO][other] Adding a specific error message for 404 cases (Job not found or not running) is a good improvement for client clarity.
• [INFO][other] This is a robust way to handle unknown error types in catch blocks, ensuring that error.message is accessed safely and a fallback string conversion is used. Consistency with the error message in the returned status object is also good.
• [INFO][other] The logic to construct the response body for non-streamable responses is well-thought-out. It correctly prioritizes the specific error message from the command handler and provides sensible defaults for 404 and other errors. This significantly improves the API's usability for clients.
• [INFO][other] Good job adding unit tests for the ComputeGetStreamableLogsHandler validation. Covering cases with missing consumerAddress and jobId ensures the basic command validation is working as expected.

@giurgiur99 giurgiur99 marked this pull request as ready for review February 23, 2026 12:13
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@giurgiur99 giurgiur99 merged commit 86c62dc into main Feb 24, 2026
12 checks passed
@giurgiur99 giurgiur99 deleted the 767-add-error-logs-for-streamable-logs-command branch February 24, 2026 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add error logs for streamable logs command

2 participants