Integrated code lifecycle: Limit build logs size
#9861
Conversation
github-actions
bot
added
server
WalkthroughThe changes primarily involve significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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.6.0)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.javaThe 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. src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.javaThe 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. src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.javaThe 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. 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (3)
400-403: Consider combining retrieval and removal operations for better thread safetyThe current implementation makes two separate calls to
buildLogsMapwhich could lead to race conditions in a concurrent environment. Consider combining these operations into a single atomic operation inBuildLogsMap.-List<BuildLogEntry> buildLogs = buildLogsMap.getAndTruncateBuildLogs(buildJob.id()); -buildLogsMap.removeBuildLogs(buildJob.id()); +List<BuildLogEntry> buildLogs = buildLogsMap.getAndTruncateAndRemoveBuildLogs(buildJob.id());
Line range hint
400-441: Good architectural decision on log size managementThe implementation correctly delegates the log truncation responsibility to
BuildLogsMapwhile maintaining the service's primary responsibility of build processing. This separation of concerns aligns well with SOLID principles and effectively addresses the PR's objective of preventing system crashes during Hazelcast transfers.
Migration to getAndTruncateBuildLogs is incomplete
The codebase still contains usages of
getBuildLogs()that should be migrated togetAndTruncateBuildLogs(). The new method includes important truncation logic for both the number of log lines and characters per line, which is missing in the old implementation. Specific locations that need updating:
BuildJobExecutionService.java:buildResult.setBuildLogEntries(buildLogsMap.getBuildLogs(buildJob.id()))RepositoryProgrammingExerciseParticipationResource.java: Contains an API endpoint usinggetBuildLogs🔗 Analysis chain
Line range hint
400-441: Verify complete migration to new build logs methodLet's ensure all occurrences of
getBuildLogshave been migrated togetAndTruncateBuildLogsthroughout the codebase.
Let me analyze the implementation of both methods to understand if they serve different purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of getBuildLogs rg "getBuildLogs\(" --type java # Search for usage of new method to confirm migration rg "getAndTruncateBuildLogs\(" --type javaLength of output: 1189
Script:
#!/bin/bash # Get the implementation of both methods from BuildLogsMap ast-grep --pattern 'public List<BuildLogEntry> getBuildLogs($_) { $$$ }' ast-grep --pattern 'public List<BuildLogEntry> getAndTruncateBuildLogs($_) { $$$ }'Length of output: 3846
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/main/resources/config/application-buildagent.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.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/SharedQueueProcessingService.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/SharedQueueProcessingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Outdated
Show resolved
Hide resolved
github-actions
bot
added
deployment-error
az108
added
deploy:artemis-test2
and removed
deployment-error
github-actions
bot
added
deployment-error
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java (1)
33-43: Enhance Javadoc to clarify newline behaviorThe method automatically appends a newline character to messages, but this behavior isn't explicitly mentioned in the Javadoc. This implicit behavior could surprise developers.
Add this clarification to the Javadoc:
/** * Appends a build log entry to the list of build logs for the specified build log ID. * If the list does not exist, it is created. If the list exceeds the maximum number of log lines, * the log entry is not added. If the log entry exceeds the maximum number of characters per line, * it is truncated. + * Note: A newline character is automatically appended to the message. * * @param buildLogId the ID of the build log to append to * @param message the message to append */src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (2)
174-174: Consider limiting stack trace size before loggingWhile the BuildLogEntry creation is correct, concatenating the full stack trace with the message could potentially create very large log entries. Consider truncating the stack trace if it exceeds a certain length.
Example implementation:
-buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJobItem.id(), new BuildLogEntry(ZonedDateTime.now(), msg + "\n" + stackTrace)); +String truncatedStackTrace = stackTrace.length() > 800 ? stackTrace.substring(0, 800) + "..." : stackTrace; +buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJobItem.id(), new BuildLogEntry(ZonedDateTime.now(), msg + "\n" + truncatedStackTrace));
Line range hint
1-290: Consider adding monitoring for truncated logsTo better understand the impact of log size limitations in production, consider adding monitoring metrics for:
- Number of truncated log entries
- Number of times max log lines limit is reached
- Size of truncated data
This will help in fine-tuning the limits and understanding the impact on users.
Would you like me to provide an example implementation using your metrics system?
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java (1)
223-223: Consider structuring error messages for better parsingWhile the log limiting changes are correct, consider structuring the error messages in a machine-parseable format. This would help in automated error analysis and monitoring.
Example format:
-String msg = "~~~~~~~~~~~~~~~~~~~~ Pulling docker image " + imageName + " with a lock after error " + e.getMessage() + " ~~~~~~~~~~~~~~~~~~~~"; +String msg = String.format("[DOCKER_PULL_ERROR] image=%s error='%s'", imageName, e.getMessage());Also applies to: 234-234, 244-244, 264-264
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (2)
Line range hint
416-424: Consider enhancing error logging for truncated entries.While the log size limitation is correctly implemented, it would be beneficial to log a warning when entries are truncated. This would help in debugging scenarios where important information might be cut off.
Consider adding a warning log when entries are truncated:
if (buildJobId != null) { - buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJobId, buildLogEntry); + boolean wasAdded = buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJobId, buildLogEntry); + if (!wasAdded) { + log.warn("Build log entry truncated or skipped for buildJobId: {}", buildJobId); + } }
Line range hint
507-511: Strengthen path validation regex.The current path validation regex might be too permissive. Consider strengthening it to prevent potential security issues.
Consider updating the path validation:
- if (path == null || path.contains("..") || !path.matches("[a-zA-Z0-9_*./-]+")) { + if (path == null || path.contains("..") || !path.matches("^[a-zA-Z0-9][a-zA-Z0-9_.*/-]*$")) { throw new LocalCIException("Invalid path: " + path); }The updated regex:
- Ensures paths start with alphanumeric characters
- Maintains the same allowed characters
- Provides better security against malformed paths
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (4)
145-145: Consider prioritizing critical error messages in the log.While the change to limit log size is good, critical error messages (like Docker image pull failures or commit hash retrieval failures) might be dropped if the log limit is reached. Consider implementing a priority system for logs to ensure critical error messages are always included.
- buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJob.id(), msg); + buildLogsMap.addBuildLogEntryWithPriority(buildJob.id(), msg, LogPriority.ERROR);Also applies to: 153-153, 175-175, 189-189
Line range hint
266-267: Address the TODO comment about method parameters.The method has too many parameters which makes it hard to maintain. Consider creating a record class to encapsulate these parameters.
public record BuildJobParameters( BuildJobQueueItem buildJob, String containerName, String containerId, VcsRepositoryUri assignmentRepositoryUri, VcsRepositoryUri testRepositoryUri, VcsRepositoryUri solutionRepositoryUri, VcsRepositoryUri[] auxiliaryRepositoriesUris, Path assignmentRepositoryPath, Path testsRepositoryPath, Path solutionRepositoryPath, Path[] auxiliaryRepositoriesPaths, String assignmentRepoCommitHash, String testRepoCommitHash ) {}
517-517: Consider rate limiting retry log messages.The retry messages in the
cloneRepositorymethod might consume a significant portion of the log limit. Consider implementing rate limiting for retry messages or logging only the first and last retry attempts.- buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJobId, - "Attempt " + attempt + " to clone repository " + repositoryUri.repositorySlug() + " failed due to " + e.getMessage() + ". Retrying..."); + if (attempt == 1 || attempt == MAX_CLONE_RETRIES - 1) { + buildLogsMap.addBuildLogEntryIfBelowMaxLines(buildJobId, + "Attempt " + attempt + " to clone repository " + repositoryUri.repositorySlug() + " failed due to " + e.getMessage() + ". Retrying..."); + }Also applies to: 520-520, 538-538
Line range hint
1-580: Overall implementation of log size limiting is well done.The changes consistently implement log size limiting across all logging points in the service. However, there are a few areas for improvement:
- Consider implementing a priority system for critical error messages
- Refactor the
runScriptAndParseResultsmethod to use a parameter object- Implement rate limiting for retry messages
These improvements would make the code more maintainable and ensure important log messages are not lost.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java(6 hunks)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(12 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.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/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
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.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 (5)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java (3)
184-184: LGTM: Log size limiting properly implemented in callback methods
The changes correctly implement the new log size limiting mechanism in both the progress and completion callbacks, which aligns with the PR's objective to prevent system crashes from large build logs.
Also applies to: 192-192
285-285: LGTM: Proper error handling with size-limited logging
The architecture validation correctly implements the new log size limiting while maintaining proper error handling through LocalCIException.
Line range hint 184-285: Verify log size limits configuration
Let's ensure that the log size limits are properly configured in the application properties and that they're being respected across the build process.
✅ Verification successful
Log size limits are properly configured and enforced
The verification shows that log size limits are properly configured and enforced through:
-
Configuration properties in
application-buildagent.yml:build-logs: max-lines-per-job: 10000 max-chars-per-line: 1024
-
These limits are injected into
BuildLogsMapclass using Spring's@Value:@Value("${artemis.continuous-integration.build-logs.max-lines-per-job:10000}") private int maxLogLinesPerBuildJob; @Value("${artemis.continuous-integration.build-logs.max-chars-per-line:1024}") private int maxCharsPerLine;
-
All log entries are consistently added through
addBuildLogEntryIfBelowMaxLinesmethod, which enforces these limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for log size limit configuration and usage
# Look for log size limit configuration
echo "Searching for log size limit configuration..."
rg -l "maxLogLinesPerBuildJob|maxCharsPerLine" --type properties
# Check for other instances of log appending that might bypass the limits
echo "Checking for potential bypass of log limits..."
rg "appendBuildLogEntry|addBuildLogEntry" --type java
# Look for any TODO comments related to logging
echo "Checking for logging-related TODOs..."
rg -i "todo.*log" --type java
Length of output: 8118
Script:
#!/bin/bash
# Let's check the BuildLogsMap implementation to verify the log size limits
echo "Checking BuildLogsMap implementation..."
ast-grep --pattern 'class BuildLogsMap {
$$$
}'
# Let's also check for any configuration properties files
echo "Searching for configuration files..."
fd -e yml -e yaml -e properties
# Let's check for any constant definitions related to log limits
echo "Checking for log limit constants..."
rg "MAX_LOG_LINES|MAX_CHARS" --type java
Length of output: 7420
Script:
#!/bin/bash
# Let's check the BuildLogsMap class implementation directly
echo "Checking BuildLogsMap class content..."
rg -A 20 "class BuildLogsMap" --type java
# Let's check application configuration files for log-related properties
echo "Checking application configuration files for log properties..."
cat src/main/resources/config/application.yml
cat src/main/resources/config/application-buildagent.yml
# Let's look for any log-related configuration in BuildLogsMap constructor or field declarations
echo "Checking for log-related configurations in BuildLogsMap..."
rg "private.*final.*MAX|private.*static.*final.*MAX" src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Length of output: 20207
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
419-419: LGTM: Log size limitation implementation looks correct.
The change from appendBuildLogEntry to addBuildLogEntryIfBelowMaxLines aligns with the PR objective to limit build log sizes. The implementation properly handles the log entry addition within the Docker command execution callback.
Let's verify the log size limitation implementation:
✅ Verification successful
LGTM: Log size limitation is properly implemented with configurable limits
The implementation is correct and well-designed:
- The
BuildLogsMapclass properly implements size limits with configurable properties:maxLogLinesPerBuildJob(default: 10000 lines)maxCharsPerLine(default: 1024 chars)
- The
addBuildLogEntryIfBelowMaxLinesmethod enforces these limits by:- Checking the number of lines before adding new entries
- Truncating long lines that exceed the character limit
- The change from
appendBuildLogEntrytoaddBuildLogEntryIfBelowMaxLinesensures consistent application of these limits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the log size limitation implementation
# Test 1: Check if BuildLogsMap class properly implements the size limits
ast-grep --pattern 'class BuildLogsMap {
$$$
private $_ maxLogLinesPerBuildJob;
private $_ maxCharsPerLine;
$$$
}'
# Test 2: Check for any direct usage of appendBuildLogEntry that might bypass the size limits
rg "appendBuildLogEntry" -A 2
Length of output: 278
Script:
#!/bin/bash
# Let's try a different approach to verify the implementation
# Check the BuildLogsMap class implementation
fd "BuildLogsMap.java" --exec cat {}
# Check for any references to appendBuildLogEntry or addBuildLogEntryIfBelowMaxLines
rg -l "appendBuildLogEntry|addBuildLogEntryIfBelowMaxLines"
# Look for the configuration of max lines/chars
fd "application.yml|application.properties" --exec cat {}
Length of output: 28752
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (1)
280-280: LGTM: Log limiting changes are consistently applied.
The changes to limit log size are applied consistently throughout the method.
Also applies to: 285-285, 292-292, 298-298, 314-314, 339-339, 352-352, 358-358
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
Outdated
Show resolved
Hide resolved
krusche
force-pushed
the
chore/integrated-code-lifecycle/limit-build-logs-size
branch
from
d2c9ca6 to
77a4cd0
Compare
github-actions
bot
added
tests
programming
az108
added
deploy:artemis-test2
and removed
deployment-error
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java (1)
10-13: LGTM! Consider adding documentation.The record structure is well-designed with appropriate annotations and field types. The use of
ZonedDateTimeandSerializableis good practice.Consider adding Javadoc to document the record's purpose and field constraints:
@JsonInclude(JsonInclude.Include.NON_EMPTY) +/** + * Data Transfer Object for build log entries. + * Used for transferring build logs between services while ensuring serialization compatibility. + * + * @param time The timestamp when the log entry was created + * @param log The actual log message content + */ public record BuildLogDTO(@NotNull ZonedDateTime time, @NotNull String log) implements Serializable {src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java (1)
21-25: Make configuration fields finalThe configuration fields should be marked as final since they are only set once during initialization. This enforces immutability and makes the code's intent clearer.
- private int maxLogLinesPerBuildJob; + private final int maxLogLinesPerBuildJob; - private int maxCharsPerLine; + private final int maxCharsPerLine;src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildResult.java (1)
30-30: Track TODO for future record conversion.The TODO indicates this class should be converted to a record in the future. This would improve immutability and reduce boilerplate code.
Would you like me to create a GitHub issue to track this technical debt item?
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
Line range hint
195-204: Consider adding error handling for oversized logsWhen saving build logs to file, we should consider handling cases where logs might exceed size limits:
- Add logging to track when logs are truncated
- Consider notifying users when their logs are truncated
if (!buildLogs.isEmpty()) { if (savedBuildJob != null) { + log.debug("Saving {} build logs for job {}", buildLogs.size(), savedBuildJob.getBuildJobId()); buildLogEntryService.saveBuildLogsToFile(buildLogs, savedBuildJob.getBuildJobId(), participation.getProgrammingExercise()); + if (buildLogs.size() >= 10000) { + log.warn("Build logs were truncated for job {} due to size limits", savedBuildJob.getBuildJobId()); + programmingMessagingService.notifyUserAboutBuildLogsTruncated(participation); + } } else { log.warn("Couldn't save build logs as build job {} was not saved", buildJob.id()); } }src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
Line range hint
335-341: Test should verify build log size limitsThe test should be enhanced to verify the new size limits introduced in this PR:
- Maximum number of logs (10,000)
- Maximum characters per log (1,024)
Consider adding these test cases:
- BuildLogDTO buildLogEntry = new BuildLogDTO(ZonedDateTime.now(), "Dummy log"); - buildLogEntryService.saveBuildLogsToFile(List.of(buildLogEntry), "6", programmingExercise); - var response = request.get("/api/build-log/6", HttpStatus.OK, String.class); - assertThat(response).contains("Dummy log"); + // Test max characters per log + String longLog = "a".repeat(1025); + BuildLogDTO oversizedLogEntry = new BuildLogDTO(ZonedDateTime.now(), longLog); + buildLogEntryService.saveBuildLogsToFile(List.of(oversizedLogEntry), "6", programmingExercise); + var response1 = request.get("/api/build-log/6", HttpStatus.OK, String.class); + assertThat(response1).hasSize(1024); // Verify truncation + + // Test max number of logs + List<BuildLogDTO> manyLogs = IntStream.range(0, 10001) + .mapToObj(i -> new BuildLogDTO(ZonedDateTime.now(), "Log " + i)) + .collect(Collectors.toList()); + buildLogEntryService.saveBuildLogsToFile(manyLogs, "6", programmingExercise); + var response2 = request.get("/api/build-log/6", HttpStatus.OK, String.class); + assertThat(response2.lines().count()).isEqualTo(10000); // Verify log count limitAdditionally, consider renaming the test to better describe what it's testing:
- void testGetBuildLogsForResult() + void testGetBuildLogsForResult_respectsSizeLimits()src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java (1)
Line range hint
1-479: Consider architectural improvements for better separation of concernsThe service currently handles both database and file operations. Consider:
- Extracting file operations to a dedicated
BuildLogFileService- Adding metrics to monitor:
- Total log size per build
- Number of truncated logs
- Storage space usage
This would improve maintainability and provide better observability of the log size limits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildResult.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/ResultQueueItem.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
🧰 Additional context used
📓 Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.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/dto/BuildResult.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/dto/ResultQueueItem.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/BuildLogsMap.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/SharedQueueProcessingService.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/programming/service/BuildLogEntryService.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/programming/service/localci/LocalCIResultProcessingService.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/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (12)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/ResultQueueItem.java (2)
Line range hint 1-10: LGTM! Well-structured DTO with proper annotations
The class follows DTO best practices with appropriate serialization support and JSON handling.
11-11: Document this breaking change in release notes
The change from BuildLogEntry to BuildLogDTO is well-aligned with DTO patterns, but as noted in the comment, this is a breaking change affecting shared data structures.
Let's verify the impact and usage of this change:
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java (1)
21-82: Implementation successfully addresses log size limitations
The implementation effectively achieves the PR's objective of preventing system crashes from large build logs by:
- Limiting the number of log entries per build job
- Restricting the character length of each log entry
- Using thread-safe data structures
The suggested improvements for parameter validation and thread safety will further enhance the robustness of this solution.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
30-30: LGTM: Clean import addition
The import follows Java best practices by avoiding star imports and maintaining a clear organization.
136-136: Verify BuildLogDTO size limits implementation
The change from BuildLogEntry to BuildLogDTO aligns with the PR's objective to limit build log sizes. However, we should verify that the DTO properly implements the size constraints (10,000 logs max, 1,024 chars per log).
✅ Verification successful
Let's search more specifically for the BuildLogDTO implementation and any size-related constraints.
Let's check the actual implementation of BuildLogDTO and the BuildLogsMap class which seems to handle the size constraints.
Size constraints are properly implemented in BuildLogsMap
The size constraints are correctly implemented in the BuildLogsMap service, which handles BuildLogDTO objects:
- Maximum 10,000 logs per build job (
maxLogLinesPerBuildJob) - Maximum 1,024 characters per log line (
maxCharsPerLine) - Truncation is properly implemented for both limits with appropriate handling
The change from BuildLogEntry to BuildLogDTO maintains these size constraints through the BuildLogsMap service, which enforces the limits before storing any logs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BuildLogDTO implementation contains size constraints
# Expected: Find size limit constants and truncation logic in BuildLogDTO
# Search for size limit constants
echo "Searching for size limit constants..."
rg -l "10000|1024" --type java
# Search for BuildLogDTO implementation
echo "Searching for BuildLogDTO implementation..."
ast-grep --pattern 'class BuildLogDTO {
$$$
}'
Length of output: 2660
Script:
#!/bin/bash
# Search for BuildLogDTO file
echo "Searching for BuildLogDTO file..."
fd BuildLogDTO
# If found, examine its content
echo "Searching for size-related constants and methods in BuildLogDTO..."
rg -A 5 "class BuildLogDTO" --type java
# Search for any build log size limits in the codebase
echo "Searching for build log size limit references..."
rg -A 5 "(?i)(build.*log.*size|log.*size.*limit)" --type java
Length of output: 8452
Script:
#!/bin/bash
# Check BuildLogDTO implementation
echo "Content of BuildLogDTO:"
cat src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java
# Check BuildLogsMap implementation for size constants
echo "Content of BuildLogsMap:"
cat src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Length of output: 4117
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
32-32: LGTM!
The import of BuildLogDTO is correctly placed and aligns with the PR's objective of managing build log sizes.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (4)
52-52: LGTM: Import change aligns with build log size limitation.
The addition of BuildLogDTO import is consistent with the PR's objective to implement build log size limitations.
400-403: LGTM: Build log size limitation implemented correctly in success path.
The implementation correctly retrieves truncated logs and cleans up afterward, preventing memory leaks.
438-441: LGTM: Build log size limitation implemented consistently in failure path.
The implementation maintains consistency with the success path, ensuring log size limitations are applied uniformly.
400-403: Extract duplicated build log handling logic.
The build log retrieval and removal logic is duplicated between success and failure paths.
Also applies to: 438-441
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java (2)
26-26: LGTM: Import follows Java best practices
The specific import of BuildLogDTO follows Java best practices by avoiding wildcard imports.
304-311: LGTM: Well-documented method signature change
The updated method signature and Javadoc are thorough, clearly documenting the parameter changes and possible exceptions.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
deployment-error
az108
removed
the
deployment-error
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java (1)
1-13: Consider validation at the DTO level vs. service level.While the current implementation is clean, consider whether size validation should be:
- Embedded in the DTO using annotations (fail-fast approach)
- Handled at the service level (more flexible but less enforced)
- Both (defense in depth)
This decision impacts where truncation occurs and how invalid data is handled across the system.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/ResultQueueItem.java (1)
8-9: Document breaking change in release notes.The comment indicates this is a breaking change affecting shared data structures in Hazelcast/Redis. This change should be prominently documented in release notes.
Consider adding a detailed migration guide in the release notes covering:
- Impact on existing Hazelcast/Redis data
- Required steps for system administrators
- Potential downtime during migration
- Rollback procedures
Also applies to: 11-11
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java (2)
21-25: Document configuration properties in application properties fileThe configuration properties should be documented in the application properties file with descriptions of their purpose and impact on system behavior.
Add the following to your application properties documentation:
# Maximum number of build log lines stored per job (default: 10000) artemis.continuous-integration.build-logs.max-lines-per-job=10000 # Maximum characters per log line to prevent memory issues (default: 1024) artemis.continuous-integration.build-logs.max-chars-per-line=1024
58-59: Enhance removeBuildLogs method with validation and feedbackThe method should validate input and provide feedback about the operation's success.
Consider this improvement:
- public void removeBuildLogs(String buildJobId) { - buildLogsMap.remove(buildJobId); + public boolean removeBuildLogs(String buildJobId) { + if (buildJobId == null || buildJobId.isBlank()) { + throw new IllegalArgumentException("buildJobId cannot be null or blank"); + } + return buildLogsMap.remove(buildJobId) != null; }src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildResult.java (2)
30-30: Consider implications of record conversionWhile converting to a record would be ideal for a DTO, note that this class extends
AbstractBuildResultNotificationDTO. Java records don't support inheritance, so this would require architectural changes:
- Extract common fields to an interface
- Make this class a record implementing that interface
- Update the inheritance hierarchy
Would you like me to propose a detailed refactoring plan?
127-128: Add null safety to the stream operationWhile the stream operation is efficient, consider adding null checks to prevent NullPointerException.
Consider this safer implementation:
- return buildLogEntries.stream().map(log -> new BuildLogEntry(log.time(), log.log())).toList(); + return buildLogEntries == null ? List.of() : buildLogEntries.stream() + .map(log -> new BuildLogEntry(log.time(), log.log())) + .toList();src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
Line range hint
192-199: Add error handling for build logs file saving operationWhile the code handles the case of a missing build job, it should also handle potential errors during the file saving operation to ensure robustness.
Consider wrapping the file operation in a try-catch block:
if (!buildLogs.isEmpty()) { if (savedBuildJob != null) { - buildLogEntryService.saveBuildLogsToFile(buildLogs, savedBuildJob.getBuildJobId(), participation.getProgrammingExercise()); + try { + buildLogEntryService.saveBuildLogsToFile(buildLogs, savedBuildJob.getBuildJobId(), participation.getProgrammingExercise()); + } catch (Exception e) { + log.error("Failed to save build logs to file for build job {}: {}", savedBuildJob.getBuildJobId(), e.getMessage()); + } } else { log.warn("Couldn't save build logs as build job {} was not saved", buildJob.id()); } }src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
Line range hint
335-341: Enhance test coverage for build log size limitsThe test should be extended to verify the new size limits mentioned in the PR objectives (10,000 logs and 1,024 characters per log). Consider adding test cases for:
- Maximum number of logs (10,000)
- Maximum characters per log (1,024)
- Truncation behavior when limits are exceeded
Here's a suggested enhancement:
- BuildLogDTO buildLogEntry = new BuildLogDTO(ZonedDateTime.now(), "Dummy log"); - buildLogEntryService.saveBuildLogsToFile(List.of(buildLogEntry), "6", programmingExercise); - var response = request.get("/api/build-log/6", HttpStatus.OK, String.class); - assertThat(response).contains("Dummy log"); + // Test max characters per log + String longLog = "a".repeat(1500); + BuildLogDTO oversizedLogEntry = new BuildLogDTO(ZonedDateTime.now(), longLog); + buildLogEntryService.saveBuildLogsToFile(List.of(oversizedLogEntry), "6", programmingExercise); + var response = request.get("/api/build-log/6", HttpStatus.OK, String.class); + assertThat(response).hasSize(1024); // Verify truncation to 1024 chars + + // Test max number of logs + List<BuildLogDTO> manyLogs = new ArrayList<>(); + for (int i = 0; i < 11000; i++) { + manyLogs.add(new BuildLogDTO(ZonedDateTime.now(), "Log " + i)); + } + buildLogEntryService.saveBuildLogsToFile(manyLogs, "6", programmingExercise); + response = request.get("/api/build-log/6", HttpStatus.OK, String.class); + assertThat(response.lines().count()).isEqualTo(10000); // Verify max logs limitsrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
400-403: Consider adding logging for truncation eventsTo help monitor the impact of log truncation, consider adding debug logging when logs are truncated. This would help track how often and by how much logs are being truncated.
List<BuildLogDTO> buildLogs = buildLogsMap.getAndTruncateBuildLogs(buildJob.id()); +if (log.isDebugEnabled()) { + log.debug("Retrieved {} build logs for job {}", buildLogs.size(), buildJob.id()); +} buildLogsMap.removeBuildLogs(buildJob.id());Also applies to: 438-441
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java (1)
327-328: Optimize StringBuilder usagePre-size the StringBuilder to improve performance and reduce memory reallocations. Calculate the estimated size based on the number of entries and maximum log length.
Apply this diff:
+// Pre-calculate buffer size: entries * (timestamp + tab + max log length) +int estimatedSize = buildLogEntries.size() * (24 + 1 + 1024); -StringBuilder logsStringBuilder = new StringBuilder(); +StringBuilder logsStringBuilder = new StringBuilder(estimatedSize);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildResult.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/ResultQueueItem.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
🧰 Additional context used
📓 Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.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/dto/BuildResult.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/dto/ResultQueueItem.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/BuildLogsMap.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/SharedQueueProcessingService.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/programming/service/BuildLogEntryService.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/programming/service/localci/LocalCIResultProcessingService.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/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (13)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java (2)
1-9: LGTM! Package and imports are well-organized.
The package naming and import statements follow best practices with specific imports and logical grouping.
11-11: Verify ZonedDateTime serialization across the system.
The use of ZonedDateTime might require special handling during JSON serialization/deserialization, especially when transmitted over Hazelcast.
✅ Verification successful
Let me generate another script to specifically look for ZonedDateTime serialization configuration and usage.
Based on the analysis of the shell script results, I can now provide a final response:
ZonedDateTime serialization is properly configured and safe to use.
The codebase has proper configuration for handling ZonedDateTime serialization:
- JavaTimeModule is registered in JacksonConfiguration for proper datetime handling
- DateTimeFormatConfiguration ensures ISO format for dates
- Multiple services consistently use DateTimeFormatter for string conversions
- The system uses standard Jackson serialization with proper modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Jackson configuration related to ZonedDateTime handling
rg -l "ObjectMapper|JavaTimeModule|DateTimeFormatter" src/
# Search for other DTOs using ZonedDateTime to ensure consistent handling
ast-grep --pattern 'record $NAME($_) { $$$ ZonedDateTime $$$ }'
Length of output: 9475
Script:
#!/bin/bash
# Check for Jackson configuration specifically for ZonedDateTime
rg -A 5 "JavaTimeModule|ZonedDateTime.*serializ|DateTimeFormatter" src/main/
# Look for ObjectMapper configuration
rg -A 5 "ObjectMapper.*configure" src/main/
# Check JacksonConfiguration class specifically
cat src/main/java/de/tum/cit/aet/artemis/core/config/JacksonConfiguration.java
Length of output: 23617
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/ResultQueueItem.java (1)
11-11: LGTM! Verify data migration strategy.
The change from BuildLogEntry to BuildLogDTO aligns well with DTO patterns and the PR's objective to limit build log sizes. The record structure is appropriate for this use case.
Let's verify the migration strategy and usage:
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildResult.java (2)
136-139: LGTM! Method follows best practices
The setter implementation:
- Follows the least access principle (package-private)
- Maintains the hasLogs state correctly
- Accepts the new DTO type consistently
45-45: LGTM! Verify BuildLogDTO usage across the codebase
The change from BuildLogEntry to BuildLogDTO aligns with the PR's objective to limit build log size and follows the DTO pattern correctly.
Let's verify the consistency of BuildLogDTO usage:
✅ Verification successful
Let me try a different approach to verify the BuildLogDTO usage.
Change from BuildLogEntry to BuildLogDTO is correctly implemented
The verification shows that:
BuildLogDTOis properly defined as a record insrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildLogDTO.java- The DTO is consistently used across the build agent codebase
- The remaining
BuildLogEntryreferences are in different components (main Artemis application, tests) and are not affected by this build agent change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining BuildLogEntry usages that might need updating
# and verify that BuildLogDTO is properly defined
# Check for BuildLogEntry direct usage
echo "Checking for BuildLogEntry usage patterns..."
rg "BuildLogEntry(?!.*import)" -g "!*.java.orig" -g "!*.java.bak"
# Verify BuildLogDTO definition
echo "Verifying BuildLogDTO structure..."
ast-grep --pattern 'record BuildLogDTO($_, $_) { $$$ }'
Length of output: 670
Script:
#!/bin/bash
# Search for BuildLogEntry and BuildLogDTO usages with proper regex
echo "Checking BuildLogEntry usages..."
rg --pcre2 "BuildLogEntry(?!\s*import)" -l
echo "Checking BuildLogDTO usages and definition..."
rg "BuildLogDTO" -A 3
# Find all Java files that might contain the DTO
echo "Looking for BuildLogDTO definition file..."
fd "BuildLogDTO.java"
Length of output: 19009
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
30-30: LGTM!
The import is correctly placed and follows the Java import guidelines.
136-136: Verify BuildLogDTO size limits implementation
The type change aligns with the PR objective to limit build logs size. Let's verify that BuildLogDTO properly implements the size constraints.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
32-32: LGTM!
The BuildLogDTO import is correctly placed and follows the alphabetical ordering convention.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (3)
52-52: LGTM: Import statement follows guidelines
The BuildLogDTO import is correctly placed in the dto section, maintaining clean code organization.
400-403: Previous comment about duplicated build log handling still applies
The build log retrieval and removal logic is duplicated between success and failure paths.
Line range hint 438-444: LGTM: Consistent log handling in error path
The error path correctly uses the truncated logs and properly updates the BuildResult, maintaining consistency with the success path.
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java (2)
26-26: LGTM: Import of BuildLogDTO is appropriate
The introduction of BuildLogDTO aligns with the PR's objective of limiting build logs size and follows proper DTO pattern for data transfer.
304-311: LGTM: Well-documented method signature change
The method signature change to use BuildLogDTO is appropriate, and the Javadoc is comprehensive with proper parameter and exception documentation.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java (6)
27-28: Enhance map documentation with size constraintsThe comment should include information about the size constraints to better document the behavior.
- // buildJobId --> List of build logs + // Maps buildJobId to a size-constrained list of build logs (max ${maxLogLinesPerBuildJob} entries per job)
36-37: Consider using @nonnull annotationSince this method always returns a non-null value, consider adding
@Nonnullannotation to make the contract explicit.+ @Nonnull public void appendBuildLogEntry(String buildJobId, String message) {
48-55: Add logging for dropped entriesConsider logging when entries are dropped due to size constraints to help with debugging and monitoring.
public void appendBuildLogEntry(String buildJobId, BuildLogDTO buildLog) { List<BuildLogDTO> buildLogs = buildLogsMap.computeIfAbsent(buildJobId, k -> new ArrayList<>()); - if (buildLogs.size() < maxLogLinesPerBuildJob) { + synchronized (buildLogs) { + if (buildLogs.size() >= maxLogLinesPerBuildJob) { + log.warn("Dropping build log entry for job {} as maximum size {} is reached", buildJobId, maxLogLinesPerBuildJob); + return; + } if (buildLog.log() != null && buildLog.log().length() > maxCharsPerLine) { + log.debug("Truncating build log entry for job {} from {} to {} characters", buildJobId, buildLog.log().length(), maxCharsPerLine); buildLog = new BuildLogDTO(buildLog.time(), buildLog.log().substring(0, maxCharsPerLine) + "\n"); } buildLogs.add(buildLog); } }
58-59: Consider returning boolean to indicate successThe method could return a boolean to indicate whether logs were actually removed, which could be useful for the caller.
- public void removeBuildLogs(String buildJobId) { - buildLogsMap.remove(buildJobId); + public boolean removeBuildLogs(String buildJobId) { + return buildLogsMap.remove(buildJobId) != null; }
77-79: Improve truncation message handlingThe truncation message should be more informative and possibly include the number of dropped entries.
List<BuildLogDTO> truncatedBuildLogs = new ArrayList<>(buildLogs.subList(0, maxLogLinesPerBuildJob)); - truncatedBuildLogs.add(new BuildLogDTO(ZonedDateTime.now(), "Truncated build logs...\n")); + int droppedEntries = buildLogs.size() - maxLogLinesPerBuildJob; + truncatedBuildLogs.add(new BuildLogDTO(ZonedDateTime.now(), + String.format("Truncated %d build log entries to stay within limit of %d entries\n", + droppedEntries, maxLogLinesPerBuildJob)));
21-82: Consider adding metrics collectionTo better monitor the system's behavior, consider adding metrics collection for:
- Number of truncated logs
- Number of dropped entries
- Current size of logs per job
- Total number of jobs with logs
This would help in tuning the size constraints and capacity planning.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/BuildLogEntryService.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildLogsMap.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
krusche
deleted the
chore/integrated-code-lifecycle/limit-build-logs-size
branch
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
There is an issue where an exercise can generate huge build logs, which when transferred over hazelcast, could crash the system with out of memory errors. This PR limits the build logs size to max 10.000 logs and max 1.024 chars per log.
Description
Before sending the buildlogs over hazelcast, we truncate them to max 10.000 logs and 1.024 chars per log. Numbers are configurable via application properties (yml settings of the build agents)
Please note that a log can consist of multiple lines. So it's normal if the lines per log file exceed 10000. But each log should not have more than 1024 chars. The number of logs can be counted by counting the timestamps (Ctrl+F
[Europe/Berlin])Steps for Testing
Prerequisites:
[Europe/Berlin]and view count)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
Code Review
Manual Tests
Performance Tests
Summary by CodeRabbit
New Features
BuildLogDTOclass to improve the structure of log entries.Bug Fixes
Refactor
BuildLogEntrytoBuildLogDTO.