Skip to content
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

Integrated code lifecycle: Improve logging when build job times out #9955

Conversation

BBesrour
Copy link
Member

@BBesrour BBesrour commented Dec 5, 2024

Checklist

General

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).

Motivation and Context

Resolves #9877

A new line is now added at the end of the logs

Timed out after X seconds. This may be due to an infinite loop or inefficient code. Please review your code for potential issues. If the problem persists, contact your instructor for assistance. (Build job ID: XYZ) 

Steps for Testing

Prerequisites:

  • 1 Instructor
  • Server with ICL (TS1-4)
  1. Create a programming exercise (or edit a existing one)
  2. Edit the timeout to lowest possible number. This can be found when checking Edit build script and you will find the timeout slider there (make sure to use exercise creation advanced mode). Save
  3. Submit to that exercise
  4. Build should fail. Check that the last log is the similar to the string above

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Screenshots

image

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced logging for container management and build job execution processes.
    • Added detailed warnings for timeout exceptions during build job execution, providing actionable guidance for users.
  • Bug Fixes

    • Improved error handling for stopping containers and collecting test results, ensuring better traceability and feedback.
  • Documentation

    • Updated logging messages to clarify operations and enhance user experience during build job execution.

@BBesrour BBesrour requested a review from a team as a code owner December 5, 2024 20:09
@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) buildagent Pull requests that affect the corresponding module labels Dec 5, 2024
Copy link

coderabbitai bot commented Dec 5, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 pmd (7.7.0)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java

The following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.

Walkthrough

The pull request introduces modifications to three service classes within the build agent module. Enhancements include improved logging for container management in BuildJobContainerService, better tracking of test result handling in BuildJobExecutionService, and refined error handling for timeouts in BuildJobManagementService. These changes aim to provide clearer visibility into operations and enhance the overall robustness of the build job execution process.

Changes

File Path Change Summary
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java Added logging to stopContainer and stopUnresponsiveContainer methods for better traceability and error handling.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java Enhanced logging for test result handling in runScriptAndParseResults, improving visibility and context.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java Improved error handling in executeBuildJob for TimeoutException, adding detailed logging for timeouts.

Assessment against linked issues

Objective Addressed Explanation
Improve feedback on build job timeout (#[9877])
Include actionable items in the feedback for timeout issues (#[9877])

Possibly related PRs

Suggested labels

tests, ready for review, programming, documentation

Suggested reviewers

  • krusche
  • az108

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (1)

180-186: LGTM! Consider adding error tracking metadata.

The timeout handling implementation provides clear user guidance and proper logging. The message effectively communicates:

  • The timeout duration
  • Potential causes (infinite loop, inefficient code)
  • Next steps (code review, instructor contact)

Consider adding structured metadata for error tracking:

 if (e instanceof TimeoutException) {
+    String errorCode = "BUILD_TIMEOUT";
     String msg = "Timed out after " + buildJobTimeoutSeconds + " seconds. "
             + "This may be due to an infinite loop or inefficient code. Please review your code for potential issues. "
             + "If the problem persists, contact your instructor for assistance. (Build job ID: " + buildJobItem.id() + ")";
     buildLogsMap.appendBuildLogEntry(buildJobItem.id(), msg);
-    log.warn(msg);
+    log.warn("{} - {}", errorCode, msg);
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8036d43 and 559e16e.

📒 Files selected for processing (3)
  • src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (3 hunks)
  • src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (6)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (2)

19-19: LGTM!

The TimeoutException import is required for the new timeout handling functionality.


Line range hint 377-377: LGTM! Enhanced log traceability.

Adding the build job ID to the duration log message improves traceability and debugging capabilities.

src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)

209-210: LGTM! Enhanced container operation logging.

Adding container ID logging before the stop operation improves observability and debugging capabilities.

src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (3)

311-314: LGTM! Enhanced test results handling observability.

Adding logging for test results directory movement improves traceability and debugging capabilities.


319-322: LGTM! Enhanced test results collection observability.

Adding logging for test results collection from the container improves traceability and debugging capabilities.


360-363: LGTM! Enhanced test results parsing observability.

Adding logging for test results parsing improves traceability and debugging capabilities.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 5, 2024
SimonEntholzer
SimonEntholzer previously approved these changes Dec 6, 2024
Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

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

Timeout log makes sense (code + tested on ts1)

EneaGore
EneaGore previously approved these changes Dec 6, 2024
Copy link
Contributor

@EneaGore EneaGore left a comment

Choose a reason for hiding this comment

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

Tested on TS1 during testing session. works as expected

coolchock
coolchock previously approved these changes Dec 6, 2024
Copy link
Contributor

@coolchock coolchock left a comment

Choose a reason for hiding this comment

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

tested on ts1, LGTM

@magkue magkue self-requested a review December 6, 2024 17:58
magkue
magkue previously approved these changes Dec 6, 2024
Copy link
Contributor

@magkue magkue left a comment

Choose a reason for hiding this comment

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

Tested on TS1 (as part of a testing session). Worked fine. No problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 559e16e and ed26c49.

📒 Files selected for processing (1)
  • src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (3)

19-19: LGTM: Import statement is appropriate

The TimeoutException import is required for the new timeout handling functionality.


180-182: LGTM: Timeout handling is well-placed

The timeout detection and logging are appropriately placed within the exception handling block, maintaining clean separation of concerns.


196-199: ⚠️ Potential issue

Revise the user-facing message content

According to previous discussions, build logs are only accessible to instructors and admins. The current message is written as if addressing students directly.

Consider revising the message to be instructor-focused:

-        String msg = "Timed out after " + buildJobTimeoutSeconds + " seconds. "
-                + "This may be due to an infinite loop or inefficient code. Please review your code for potential issues. "
-                + "If the problem persists, contact your instructor for assistance. (Build job ID: " + buildJobItem.id() + ")";
+        String msg = "Build job timed out after " + buildJobTimeoutSeconds + " seconds. "
+                + "Student code may contain an infinite loop or inefficient implementation. "
+                + "Consider reviewing the submission for these issues. (Build job ID: " + buildJobItem.id() + ")";

Copy link

github-actions bot commented Dec 7, 2024

⚠️ Unable to deploy to test servers ⚠️

Testserver "artemis-test1.artemis.cit.tum.de" is already in use by PR #9965.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Dec 7, 2024
@BBesrour BBesrour added ready to merge deploy:artemis-test1 and removed deployment-error Added by deployment workflows if an error occured labels Dec 7, 2024
@BBesrour BBesrour temporarily deployed to artemis-test1.artemis.cit.tum.de December 8, 2024 15:38 — with GitHub Actions Inactive
@krusche krusche added this to the 7.7.5 milestone Dec 8, 2024
@krusche krusche changed the title Integrated code lifecycle: Add a log when buildjob times out Integrated code lifecycle: Improve logging when build job times out Dec 8, 2024
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

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

After discussing the changes with @BBesrour I agree that they make sense because students would see the logs when a build fails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildagent Pull requests that affect the corresponding module ready to merge server Pull requests that update Java code. (Added Automatically!)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Programming Exercise: Improve feedback on build job timeout
6 participants