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

Athena: Add LLM token usage tracking #9548

Conversation

FelixTJDietrich
Copy link
Contributor

@FelixTJDietrich FelixTJDietrich commented Oct 21, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

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).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

We want to track the token usage for Athena.

Description

Stores token usage of Athena from the response meta field for feedback generation requests.

Follow-up Issues

  • Extend Athena to send costPerMillionInputToken, costPerMillionOutputToken and a pipelineId
  • Update Artemis Athena adapter to also store those fields
  • Refactor to save meta for each request

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 2 Students
  • 1 Programming Exercise with Complaints enabled
  1. Log in to Artemis
  2. Navigate to Course Administration
  3. ...

Exam Mode Testing

Prerequisites:

  • 1 Instructor
  • 2 Students
  • 1 Exam with a Programming Exercise
  1. Log in to Artemis
  2. Participate in the exam as a student
  3. Make sure that the UI of the programming exercise in the exam mode stays unchanged. You can use the exam mode documentation as reference.
  4. ...

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

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

  • New Features

    • Introduced several new Data Transfer Objects (DTOs) for handling response metadata and token usage.
    • Enhanced services to track and manage LLM token usage effectively.
    • Updated websocket communication to accommodate new token-related data.
  • Bug Fixes

    • Improved handling of status updates to ensure token usage is tracked accurately.
  • Tests

    • Added integration tests for chat token tracking functionality to validate the new features.
    • Updated existing tests to accommodate changes in method signatures and DTO structures.

…na/track-token-usage

# Conflicts:
#	src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java
@FelixTJDietrich FelixTJDietrich marked this pull request as ready for review October 21, 2024 09:49
@FelixTJDietrich FelixTJDietrich requested a review from a team as a code owner October 21, 2024 09:49
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. athena Pull requests that affect the corresponding module core Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module ready for review labels Oct 21, 2024
@FelixTJDietrich FelixTJDietrich marked this pull request as draft October 21, 2024 09:50
@FelixTJDietrich FelixTJDietrich changed the base branch from develop to feature/track-usage-of-iris-requests October 21, 2024 09:51
Copy link

coderabbitai bot commented Oct 21, 2024

Walkthrough

This pull request introduces several new data transfer objects (DTOs) and modifications to existing services and DTOs within the Artemis application. Key additions include ResponseMetaDTO, ResponseMetaLLMCallDTO, and ResponseMetaTotalUsageDTO, which encapsulate metadata related to LLM usage. The AthenaFeedbackSuggestionsService has been updated to integrate these new DTOs, enhancing the response structure and enabling token usage tracking. Additionally, several classes and methods across the application have been modified to accommodate these changes, improving the overall handling of LLM requests and responses.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaDTO.java New record ResponseMetaDTO added with fields totalUsage (type ResponseMetaTotalUsageDTO) and llmCalls (list of ResponseMetaLLMCallDTO).
src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaLLMCallDTO.java New record ResponseMetaLLMCallDTO added with fields modelName, inputTokens, outputTokens, and totalTokens.
src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaTotalUsageDTO.java New record ResponseMetaTotalUsageDTO added with fields inputTokens, outputTokens, and totalTokens.
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaFeedbackSuggestionsService.java Multiple updates including new field llmTokenUsageService, constructor modification, addition of meta field to response DTOs, and new method storeTokenUsage.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMRequest.java New record LLMRequest added with fields for model, input/output tokens, costs, and pipeline ID.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java New enum LLMServiceType added with constants IRIS and ATHENA.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageRequest.java New class LLMTokenUsageRequest added as a JPA entity with several fields and relationships.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageTrace.java New class LLMTokenUsageTrace added as a JPA entity with fields for tracking token usage.
src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageRequestRepository.java New interface LLMTokenUsageRequestRepository added.
src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageTraceRepository.java New interface LLMTokenUsageTraceRepository added.
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java New service class LLMTokenUsageService added with methods for managing token usage.
src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java Updated to include new parameter List<LLMRequest> tokens.
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java Constructor updated to include LLMTokenUsageService, and method handleStatusUpdate modified to accept a CompetencyExtractionJob object.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java Method handleStatusUpdate updated to accept a CompetencyExtractionJob object instead of separate parameters.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java Updated to include new field List<LLMRequest> tokens.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java Updated to include new field List<LLMRequest> tokens.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java New record PyrisLLMCostDTO added with fields for cost data related to LLMs.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/CompetencyExtractionJob.java Updated to replace userLogin parameter with a User object.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/AbstractIrisChatSessionService.java New field traces added to store LLMTokenUsageTrace objects.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java Constructor updated to include LLMTokenUsageService, with modifications to the handleStatusUpdate method for token usage management.
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java Constructor updated to include LLMTokenUsageService, with modifications to the handleStatusUpdate method for token usage management.
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java Method signatures updated to include List<LLMRequest> tokens for status updates.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java Method sendStatus updated to accept an additional parameter.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java New integration test class added for testing token tracking functionality.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatWebsocketTest.java Updated sendMessage test method to include an additional parameter in the IrisChatWebsocketDTO constructor.
src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java Modified test method to invoke handleStatusUpdate with a CompetencyExtractionJob object.
src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java Method sendStatus updated to accept an additional parameter.

Possibly related PRs

  • Programming exercises: Improve preliminary AI feedback  #9324: The changes in this PR involve modifications to the feedback process in the Artemis platform, which includes the integration of AI-generated feedback results. This is related to the ResponseMetaDTO introduced in the main PR, as both involve handling metadata and results related to responses in the Athena service.

Suggested labels

client, ready for review, assessment, exercise, programming, priority:critical

Suggested reviewers

  • FelixTJDietrich
  • krusche
  • LeonWehrhahn
  • eceeeren
  • MaximilianAnzinger

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.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 13

🧹 Outside diff range and nitpick comments (27)
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (1)

1-8: LGTM with a minor suggestion for JavaDoc improvement.

The LLMServiceType enum is well-structured and follows the coding guidelines. It has a clear purpose, uses proper naming conventions, and adheres to the single responsibility principle.

Consider expanding the JavaDoc to include a brief description of each enum constant:

/**
 * Enum representing different types of LLM (Large Language Model) services used in the system.
 */
public enum LLMServiceType {
    /** Represents the IRIS LLM service. */
    IRIS,
    /** Represents the ATHENA LLM service. */
    ATHENA
}

This addition would provide more context about each service type, enhancing code documentation.

src/main/java/de/tum/cit/aet/artemis/core/domain/LLMRequest.java (1)

1-4: LGTM! Consider adding documentation.

The LLMRequest record is well-structured and adheres to the coding guidelines. It uses appropriate types for its fields and follows the single responsibility principle by encapsulating data related to an LLM request.

Consider adding Javadoc comments to describe the purpose of the record and its fields. This would enhance code readability and maintainability. Here's an example:

/**
 * Represents a request to a Large Language Model (LLM).
 * This record encapsulates all necessary information for tracking LLM usage and costs.
 */
public record LLMRequest(
    /** The name or identifier of the LLM model. */
    String model,
    /** The number of input tokens used in the request. */
    int numInputTokens,
    /** The cost per million input tokens. */
    float costPerMillionInputToken,
    /** The number of output tokens generated. */
    int numOutputTokens,
    /** The cost per million output tokens. */
    float costPerMillionOutputToken,
    /** The identifier of the pipeline used for this request. */
    String pipelineId
) {}
src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaTotalUsageDTO.java (1)

5-7: Consider enhancing the class documentation.

While the current documentation provides a basic description, it could be more informative. Consider adding details about the purpose of each field and their significance in the context of token usage tracking.

Here's a suggested improvement:

/**
 * DTO representing the total usage metrics for LLM token consumption.
 * 
 * @param inputTokens The number of tokens in the input text.
 * @param outputTokens The number of tokens generated in the output.
 * @param totalTokens The total number of tokens processed (input + output).
 */
src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaLLMCallDTO.java (2)

5-7: Consider enhancing the class documentation.

While the current JavaDoc comment provides a clear description of the DTO's purpose, it could be improved by adding more details about the fields it contains.

Consider expanding the documentation as follows:

/**
 * DTO representing an individual LLM call.
 * 
 * @param modelName The name of the LLM model used.
 * @param inputTokens The number of input tokens processed.
 * @param outputTokens The number of output tokens generated.
 * @param totalTokens The total number of tokens (input + output) for this call.
 */

9-10: LGTM: Well-structured record declaration with a minor suggestion.

The use of a record for this DTO is excellent. It provides immutability and automatic generation of necessary methods. The field names and types are appropriate, and the use of Integer allows for null values, which is consistent with the @JsonInclude annotation.

Consider adding a validation check for the totalTokens field to ensure it equals the sum of inputTokens and outputTokens. This could be done in a compact form within the record:

public record ResponseMetaLLMCallDTO(String modelName, Integer inputTokens, Integer outputTokens, Integer totalTokens) {
    public ResponseMetaLLMCallDTO {
        if (inputTokens != null && outputTokens != null && totalTokens != null) {
            assert totalTokens.equals(inputTokens + outputTokens) : "Total tokens should equal the sum of input and output tokens";
        }
    }
}

This addition would ensure data integrity within the DTO.

src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaDTO.java (2)

7-9: Enhance class documentation for clarity.

While the current JavaDoc provides a basic description, it could be more informative. Consider adding details about the fields totalUsage and llmCalls to provide a comprehensive overview of the DTO's structure and purpose.

Suggested improvement:

/**
 * DTO representing the meta information in the Athena response.
 * <p>
 * This record encapsulates:
 * <ul>
 *   <li>totalUsage: Aggregated usage metrics for the response.</li>
 *   <li>llmCalls: List of individual LLM call details made during the response generation.</li>
 * </ul>
 */

11-12: LGTM: Well-structured record declaration with a minor suggestion.

The ResponseMetaDTO record is well-defined and adheres to the coding guidelines for DTOs. It uses appropriate field names and types, following the single responsibility principle.

For improved type safety and to prevent potential null pointer exceptions, consider using an immutable list type for llmCalls.

Suggested improvement:

import java.util.List;
import java.util.Collections;

public record ResponseMetaDTO(ResponseMetaTotalUsageDTO totalUsage, List<ResponseMetaLLMCallDTO> llmCalls) {
    public ResponseMetaDTO {
        llmCalls = llmCalls == null ? Collections.emptyList() : Collections.unmodifiableList(llmCalls);
    }
}

This change ensures that llmCalls is never null and cannot be modified after creation.

src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageRequestRepository.java (1)

11-14: LGTM: Interface declaration and annotations are correct.

The interface is well-defined and follows coding guidelines:

  • Proper use of @Profile and @repository annotations
  • Follows CamelCase naming convention
  • Extends ArtemisJpaRepository for code reuse

Consider adding a brief Javadoc comment to describe the purpose of this repository interface, enhancing code documentation:

/**
 * Repository interface for managing LLMTokenUsageRequest entities.
 */
@Profile(PROFILE_CORE)
@Repository
public interface LLMTokenUsageRequestRepository extends ArtemisJpaRepository<LLMTokenUsageRequest, Long> {
}
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (1)

20-20: LGTM: Record definition updated correctly with a minor suggestion.

The PyrisCompetencyStatusUpdateDTO record has been appropriately updated to include the new tokens parameter for LLM token usage tracking. This change aligns with the PR objectives and follows the DTO guidelines.

Consider renaming tokens to llmRequests for improved clarity, as it more accurately describes the content of the list:

-public record PyrisCompetencyStatusUpdateDTO(List<PyrisStageDTO> stages, List<PyrisCompetencyRecommendationDTO> result, List<LLMRequest> tokens) {
+public record PyrisCompetencyStatusUpdateDTO(List<PyrisStageDTO> stages, List<PyrisCompetencyRecommendationDTO> result, List<LLMRequest> llmRequests) {

Don't forget to update the corresponding documentation if you make this change.

src/main/java/de/tum/cit/aet/artemis/iris/service/session/AbstractIrisChatSessionService.java (1)

19-20: Consider using Map interface and adding a comment.

The new field traces is well-declared as protected and final. However, consider the following improvements:

  1. Use the Map interface instead of the concrete HashMap class for better flexibility:

    protected final Map<String, LLMTokenUsageTrace> traces = new HashMap<>();
  2. Add a brief comment explaining the purpose of this field to improve code readability.

src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageTrace.java (3)

22-26: LGTM: Class declaration and annotations are appropriate.

The class declaration and annotations follow the coding guidelines, including proper naming, entity mapping, caching, and JSON serialization configuration.

Consider adding a brief Javadoc comment to describe the purpose of this class, which would enhance code documentation and maintainability.


28-51: Field declarations are well-structured, but consider using a primitive for userId.

The field declarations follow the coding guidelines with appropriate JPA annotations, nullable markings, and relationship configurations. However, there's room for improvement:

Consider changing the userId field to a primitive long instead of Long to align with the guideline of preferring primitives. This change would prevent potential null values and improve type safety. If userId can be null, make sure to document this requirement clearly.


1-108: Overall, well-structured class with minor improvements suggested.

The LLMTokenUsageTrace class is well-designed and adheres to most of the coding guidelines. It demonstrates good use of JPA annotations, follows the single responsibility principle, and provides proper encapsulation.

To further improve the class:

  1. Add a class-level Javadoc comment explaining the purpose and usage of this class.
  2. Resolve the userId type inconsistency as mentioned in previous comments.
  3. Consider adding validation logic in setters, especially for fields like courseId, exerciseId, and userId to ensure data integrity.
  4. You might want to override the toString() method to provide a meaningful string representation of the object, which can be helpful for logging and debugging.

Example of a class-level Javadoc and toString() method:

/**
 * Represents a trace of LLM token usage in the system.
 * This entity is used to track and store information about token consumption
 * across different services, courses, exercises, and users.
 */
@Entity
@Table(name = "llm_token_usage_trace")
// ... other annotations ...
public class LLMTokenUsageTrace extends DomainObject {
    // ... existing code ...

    @Override
    public String toString() {
        return "LLMTokenUsageTrace{" +
               "id=" + getId() +
               ", serviceType=" + serviceType +
               ", courseId=" + courseId +
               ", exerciseId=" + exerciseId +
               ", userId=" + userId +
               ", time=" + time +
               ", irisMessageId=" + irisMessageId +
               '}';
    }
}

These improvements will enhance the overall quality, maintainability, and usability of the class.

src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (2)

35-37: Update constructor Javadoc to include the new parameter.

The constructor has been correctly updated to include the new tokens parameter. However, the Javadoc comment for the constructor should be updated to include a description of this new parameter.

Please update the constructor's Javadoc to include:

* @param tokens        the list of LLM requests with token usage information

Line range hint 63-66: Update equals method to include the new tokens field.

The equals method should be updated to include the comparison of the new tokens field. This ensures that equality checks consider all fields of the record.

Please update the equals method as follows:

@Override
public boolean equals(Object o) {
    if (this == o) {
        return true;
    }
    if (o == null || getClass() != o.getClass()) {
        return false;
    }
    IrisChatWebsocketDTO that = (IrisChatWebsocketDTO) o;
    return type == that.type 
        && Objects.equals(message, that.message) 
        && Objects.equals(rateLimitInfo, that.rateLimitInfo) 
        && Objects.equals(stages, that.stages)
        && Objects.equals(suggestions, that.suggestions)
        && Objects.equals(tokens, that.tokens);
}
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatWebsocketTest.java (2)

56-56: Update test to reflect new IrisChatWebsocketDTO structure

The test has been updated to include a new parameter (List.of()) in the IrisChatWebsocketDTO constructor. This change aligns with the modifications made to the DTO structure.

However, there are a few points to consider:

  1. The test method name "sendMessage" could be more descriptive (e.g., "testSendMessageUpdatesWebsocket").
  2. The test is using verify instead of assertThat. Consider using assertThat for more specific assertions.
  3. The new parameter is an empty list. Consider adding a test case with non-empty token data to ensure proper handling.

To improve the test, consider the following changes:

- @Test
- @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
- void sendMessage() {
+ @Test
+ @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
+ void testSendMessageUpdatesWebsocketWithCorrectDTO() {
     // ... existing code ...
-    verify(websocketMessagingService, times(1)).sendMessageToUser(eq(TEST_PREFIX + "student1"), eq("/topic/iris/" + irisSession.getId()),
-            eq(new IrisChatWebsocketDTO(message, new IrisRateLimitService.IrisRateLimitInformation(0, -1, 0), List.of(), List.of(), List.of())));
+    IrisChatWebsocketDTO expectedDTO = new IrisChatWebsocketDTO(message, new IrisRateLimitService.IrisRateLimitInformation(0, -1, 0), List.of(), List.of(), List.of());
+    verify(websocketMessagingService, times(1)).sendMessageToUser(eq(TEST_PREFIX + "student1"), eq("/topic/iris/" + irisSession.getId()), eq(expectedDTO));
+    
+    // Additional assertion for more specific checking
+    assertThat(expectedDTO.tokens()).isEmpty();
 }

Also, consider adding a new test case to verify behavior with non-empty token data.


Line range hint 1-77: Improve adherence to coding guidelines

The test class partially adheres to the provided coding guidelines. Here are some suggestions for improvement:

  1. Use assertThat for assertions instead of verify (assert_use: assertThat).
  2. Implement DB query count tracking for performance monitoring (db_query_count_tests: track_performance).
  3. Consider using ArchUnit to enforce package rules (archunit_use: enforce_package_rules).
  4. Implement a utility service factory pattern for better test organization (util_service_factory_pattern: true).
  5. Add explicit measures to minimize context restarts (context_restart_minimize: true).

Positive aspects:

  • JUnit 5 features are used (@BeforeEach, @Test).
  • DB access is avoided using mock users.
  • Fixed data is used (mock content).
  • The test size is appropriate and specific.
  • Static mocks are used.

Consider implementing these improvements to fully align with the coding guidelines and enhance the overall quality of the test suite.

🧰 Tools
🪛 ast-grep

[warning] 55-55: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new IrisChatWebsocketDTO(message, new IrisRateLimitService.IrisRateLimitInformation(0, -1, 0), List.of(), List.of(), List.of()))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 55-55: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (message, new IrisRateLimitService.IrisRateLimitInformation(0, -1, 0), List.of(), List.of(), List.of())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 55-55: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new IrisChatWebsocketDTO(message, new IrisRateLimitService.IrisRateLimitInformation(0, -1, 0), List.of(), List.of(), List.of()))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 55-55: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (message, new IrisRateLimitService.IrisRateLimitInformation(0, -1, 0), List.of(), List.of(), List.of())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (1)

70-73: LGTM: Test method updated correctly with CompetencyExtractionJob.

The changes align with the new implementation using CompetencyExtractionJob. The test maintains its descriptive naming and specific focus, adhering to the coding guidelines.

Consider adding a brief comment explaining the purpose of the CompetencyExtractionJob for improved clarity:

// Create a CompetencyExtractionJob to simulate the competency generation process
CompetencyExtractionJob job = new CompetencyExtractionJob(jobId, course.getId(), userUtilService.getUserByLogin(userLogin));
🧰 Tools
🪛 ast-grep

[warning] 72-72: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (job, new PyrisCompetencyStatusUpdateDTO(stages, recommendations, null))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 72-72: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (job, new PyrisCompetencyStatusUpdateDTO(stages, recommendations, null))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java (2)

401-401: LGTM! Consider adding a TODO comment for token tracking.

The update to PyrisChatStatusUpdateDTO constructor aligns with the new token tracking feature. However, passing null for the new parameter might not fully test the new functionality.

Consider adding a TODO comment to remind about implementing a test case that includes non-null token data:

-        request.postWithoutResponseBody("/api/public/pyris/pipelines/text-exercise-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
+        // TODO: Add test case with non-null token data for LLM usage tracking
+        request.postWithoutResponseBody("/api/public/pyris/pipelines/text-exercise-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
                 HttpStatus.OK, headers);
🧰 Tools
🪛 ast-grep

[warning] 400-401: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: ("/api/public/pyris/pipelines/text-exercise-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
HttpStatus.OK, headers)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 400-401: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: ("/api/public/pyris/pipelines/text-exercise-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
HttpStatus.OK, headers)
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


Line range hint 1-401: Consider adding a new test case for token tracking

While the existing tests cover various scenarios, there's an opportunity to enhance the test suite by adding a specific test case for the new token tracking feature.

Consider adding a new test method to verify the behavior of the system when token data is provided. This will ensure that the new feature is properly tested. Here's a suggested outline for the new test case:

@Test
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
void sendMessageWithTokenTracking() throws Exception {
    var irisSession = createSessionForUser("student1");
    var messageToSend = createDefaultMockMessage(irisSession);
    
    List<LLMRequest> mockTokens = List.of(new LLMRequest(/* add necessary parameters */));
    
    irisRequestMockProvider.mockTextExerciseChatResponse(dto -> {
        assertThat(dto.execution().settings().authenticationToken()).isNotNull();
        
        assertThatNoException().isThrownBy(() -> sendStatus(
            dto.execution().settings().authenticationToken(),
            "Response with tokens",
            dto.execution().initialStages(),
            null,
            mockTokens
        ));
        
        pipelineDone.set(true);
    });
    
    request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED);
    
    await().until(pipelineDone::get);
    
    // Add assertions to verify token tracking behavior
    // For example, check if the tokens are correctly stored or processed
}

Would you like me to implement this test case or create a GitHub issue to track this task?

🧰 Tools
🪛 ast-grep

[warning] 399-399: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Map.of("Authorization", List.of("Bearer " + jobId)))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 400-401: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: ("/api/public/pyris/pipelines/text-exercise-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
HttpStatus.OK, headers)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 399-399: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (Map.of("Authorization", List.of("Bearer " + jobId)))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 400-401: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: ("/api/public/pyris/pipelines/text-exercise-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
HttpStatus.OK, headers)
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java (1)

448-449: LGTM! Consider adding a comment for the new parameter.

The changes look good. The new parameter in the PyrisChatStatusUpdateDTO constructor is correctly added. However, it might be helpful to add a comment explaining the purpose of this new parameter, especially since it's currently set to null.

Consider adding a comment like this:

 request.postWithoutResponseBody("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", 
-    new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
+    new PyrisChatStatusUpdateDTO(result, stages, suggestions, null), // null for tokens (not implemented/tested yet)
     HttpStatus.OK, headers);

This will help other developers understand the purpose of the new parameter and why it's currently set to null.

🧰 Tools
🪛 ast-grep

[warning] 448-449: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: ("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
HttpStatus.OK, headers)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 448-449: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: ("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, null),
HttpStatus.OK, headers)
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageRequest.java (1)

60-62: Ensure consistency in parameter names of setter methods

The parameter names in the setter methods setCostPerMillionInputTokens and setCostPerMillionOutputTokens are singular, whereas the field names are plural. For consistency and clarity, consider renaming the parameters to match the field names.

Apply this diff to rename the parameters:

 public void setCostPerMillionInputTokens(float costPerMillionInputToken) {
-    this.costPerMillionInputTokens = costPerMillionInputToken;
+    this.costPerMillionInputTokens = costPerMillionInputTokens;
 }

+public void setCostPerMillionInputTokens(float costPerMillionInputTokens) {
+    this.costPerMillionInputTokens = costPerMillionInputTokens;
+}

 public void setCostPerMillionOutputTokens(float costPerMillionOutputToken) {
-    this.costPerMillionOutputTokens = costPerMillionOutputToken;
+    this.costPerMillionOutputTokens = costPerMillionOutputTokens;
 }

+public void setCostPerMillionOutputTokens(float costPerMillionOutputTokens) {
+    this.costPerMillionOutputTokens = costPerMillionOutputTokens;
+}

Also applies to: 68-70

src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (1)

65-65: Correct grammatical error in JavaDoc comment

There's a grammatical error in the JavaDoc comment. Change "token usage and cost send by Pyris" to "token usage and cost sent by Pyris" to improve readability.

Apply this diff to fix the error:

- * @param tokens      token usage and cost send by Pyris
+ * @param tokens      token usage and cost sent by Pyris
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (2)

123-153: Suggestion: Narrow the exception types in test methods

The test methods testTokenTrackingHandledExerciseChat and testTokenTrackingExerciseChatWithPipelineFail currently declare throws Exception. It's better practice to specify the exact exceptions that might be thrown or handle them within the test to improve clarity and maintainability.

Apply this change to specify the exceptions:

- void testTokenTrackingHandledExerciseChat() throws Exception {
+ void testTokenTrackingHandledExerciseChat() throws IOException, URISyntaxException, GitAPIException {

And:

- void testTokenTrackingExerciseChatWithPipelineFail() throws Exception {
+ void testTokenTrackingExerciseChatWithPipelineFail() throws IOException, URISyntaxException, GitAPIException {

Adjust the exception types based on the actual exceptions that can be thrown.

Also applies to: 173-204

🧰 Tools
🪛 ast-grep

[warning] 127-127: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done"))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 127-127: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


225-229: Suggestion: Narrow the exception type in sendStatus method

The sendStatus method currently declares throws Exception, which is too broad. Consider specifying the exact exceptions that might be thrown (e.g., IOException) to improve error handling and make the code clearer.

Apply this change:

- private void sendStatus(String jobId, String result, List<PyrisStageDTO> stages, List<LLMRequest> tokens) throws Exception {
+ private void sendStatus(String jobId, String result, List<PyrisStageDTO> stages, List<LLMRequest> tokens) throws IOException {

Adjust the exception types based on the actual exceptions that can be thrown within the method.

🧰 Tools
🪛 ast-grep

[warning] 225-225: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (Map.of("Authorization", List.of("Bearer " + jobId)))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 226-227: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: ("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, null, tokens),
HttpStatus.OK, headers)
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 225-225: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Map.of("Authorization", List.of("Bearer " + jobId)))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 226-227: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: ("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, null, tokens),
HttpStatus.OK, headers)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaFeedbackSuggestionsService.java (2)

64-64: Fix typo in JavaDoc parameter description

There's a typo in the JavaDoc comment for athenaDTOConverterService. The word "exrcises" should be "exercises".

Apply this diff to correct the typo:

  /**
   * Create a new AthenaFeedbackSuggestionsService to receive feedback suggestions from the Athena service.
   *
   * @param athenaRestTemplate        REST template used for the communication with Athena
   * @param athenaModuleService       Athena module serviced used to determine the urls for different modules
-  * @param athenaDTOConverterService Service to convert exrcises and submissions to DTOs
+  * @param athenaDTOConverterService Service to convert exercises and submissions to DTOs
   * @param llmTokenUsageService      Service to store the usage of LLM tokens
   */

159-166: Use primitive boolean instead of wrapper Boolean for isPreliminaryFeedback

In the storeTokenUsage method, the parameter isPreliminaryFeedback is declared as a Boolean object. Since this value is always required and does not need to be null, it's better to use the primitive type boolean to avoid unnecessary object overhead.

Apply this diff to use the primitive type:

 /**
  * Store the usage of LLM tokens for a given submission
  *
  * @param exercise              the exercise the submission belongs to
  * @param submission            the submission for which the tokens were used
  * @param meta                  the meta information of the response from Athena
- * @param isPreliminaryFeedback whether the feedback is preliminary or not
+ * @param isPreliminaryFeedback whether the feedback is preliminary or not (true or false)
  */
-private void storeTokenUsage(Exercise exercise, Submission submission, ResponseMetaDTO meta, Boolean isPreliminaryFeedback) {
+private void storeTokenUsage(Exercise exercise, Submission submission, ResponseMetaDTO meta, boolean isPreliminaryFeedback) {

Ensure that you update any method calls to pass a boolean value instead of a Boolean object.

🛑 Comments failed to post (13)
src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaTotalUsageDTO.java (1)

9-9: 🛠️ Refactor suggestion

Consider using primitive types for the fields.

The coding guidelines suggest preferring primitive types. Since these fields represent counts, they should never be null. Consider using int instead of Integer for all three fields.

Here's the suggested change:

-public record ResponseMetaTotalUsageDTO(Integer inputTokens, Integer outputTokens, Integer totalTokens) {
+public record ResponseMetaTotalUsageDTO(int inputTokens, int outputTokens, int totalTokens) {

This change would also eliminate the need for the @JsonInclude annotation, as primitive types are always included in JSON serialization.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

public record ResponseMetaTotalUsageDTO(int inputTokens, int outputTokens, int totalTokens) {
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/CompetencyExtractionJob.java (1)

16-16: 💡 Codebase verification

Issues Found: Update userLogin References in Tests

  • IrisCompetencyGenerationIntegrationTest.java:72 is still using userLogin string. Update it to pass a User object.
🔗 Analysis chain

LGTM: Record declaration updated correctly. Verify impact on codebase.

The change from String userLogin to User user aligns with the single responsibility principle and follows the DTO guideline of using Java records. This change enhances the job's capability to handle user-related data.

However, this modification might impact other parts of the codebase that were previously using the userLogin string.

Please run the following script to identify potential areas that might need updates:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find usages of CompetencyExtractionJob constructor and potential references to userLogin

# Search for CompetencyExtractionJob constructor calls
echo "Searching for CompetencyExtractionJob constructor calls:"
rg -p "new CompetencyExtractionJob\(" --type java

# Search for potential references to userLogin within CompetencyExtractionJob context
echo "\nSearching for potential references to userLogin:"
rg -p "CompetencyExtractionJob.*userLogin" --type java

Length of output: 997

src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageTrace.java (1)

53-107: ⚠️ Potential issue

Inconsistency in userId getter and setter types.

The getter and setter methods are generally well-implemented, providing proper encapsulation. However, there's an inconsistency in the userId methods:

The userId field is of type Long, but the getUserId() method returns long. This mismatch could lead to potential NullPointerExceptions. To resolve this, either:

  1. Change the field type to long (preferred, as suggested earlier):
private long userId;
  1. Or, change the getter return type to match the field:
public Long getUserId() {
    return userId;
}

Make sure to adjust the setter parameter type accordingly:

public void setUserId(Long userId) {
    this.userId = userId;
}

Choose the option that best fits the requirements of whether userId can be null or not.

src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageRequest.java (2)

28-30: ⚠️ Potential issue

Use BigDecimal for monetary values instead of float

Using float for monetary values can lead to precision and rounding errors. It's recommended to use BigDecimal for precise monetary calculations.

Apply this diff to update the data types:

+import java.math.BigDecimal;

 @Column(name = "cost_per_million_input_tokens")
-private float costPerMillionInputTokens;
+private BigDecimal costPerMillionInputTokens;

 @Column(name = "cost_per_million_output_tokens")
-private float costPerMillionOutputTokens;
+private BigDecimal costPerMillionOutputTokens;

Remember to update the corresponding getters and setters to use BigDecimal.

Also applies to: 34-36


37-38: 🛠️ Refactor suggestion

Specify FetchType.LAZY in the @ManyToOne relationship

The default fetch type for @ManyToOne is FetchType.EAGER, which can lead to performance issues by loading related entities unnecessarily. Specify FetchType.LAZY to optimize performance.

Apply this diff to modify the fetch type:

+import jakarta.persistence.FetchType;

-@ManyToOne
+@ManyToOne(fetch = FetchType.LAZY)
 private LLMTokenUsageTrace trace;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    import jakarta.persistence.FetchType;

    @ManyToOne(fetch = FetchType.LAZY)
    private LLMTokenUsageTrace trace;
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (2)

56-56: 🛠️ Refactor suggestion

Refactor method to improve clarity by reducing null parameters

In sendStatusUpdate, passing null values for unused parameters can be confusing. Overloading the sendStatusUpdate method to handle different parameter sets or using default values can enhance code clarity and adhere to clean code practices.


45-45: 🛠️ Refactor suggestion

Consider refactoring to avoid passing multiple null arguments

Passing multiple null arguments to constructors reduces code readability and maintainability. Consider using the builder pattern or providing overloaded constructors for IrisChatWebsocketDTO to handle different parameter combinations more gracefully.

src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)

174-206: 🛠️ Refactor suggestion

Refactor handleStatusUpdate method to improve readability and maintainability

The nested if-else blocks within the handleStatusUpdate method from lines 186 to 206 increase complexity and reduce readability. Consider refactoring the token usage handling logic into separate helper methods or simplifying the conditional structures to adhere to the Single Responsibility Principle and improve code clarity.

🧰 Tools
🪛 ast-grep

[warning] 176-176: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new IrisTextMessageContent(statusUpdate.result()))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 176-176: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new IrisTextMessageContent(statusUpdate.result()))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)

64-67: 🛠️ Refactor suggestion

Consider refactoring the constructor to reduce parameter count

The constructor currently has ten parameters, which can make the class harder to maintain and test. To improve readability and adhere to the Single Responsibility Principle, consider grouping related parameters into parameter objects or using a builder pattern to manage object creation.

src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)

139-141: ⚠️ Potential issue

Fix: Replace getFirst() with get(0) when accessing list elements

The method getFirst() is not defined for List in Java. To access the first element of a list, use get(0) instead. This will prevent a compilation error.

Apply this diff to fix the issue:

- assertThat(savedTokenUsageTraces.getFirst().getServiceType()).isEqualTo(LLMServiceType.IRIS);
- assertThat(savedTokenUsageTraces.getFirst().getExerciseId()).isEqualTo(exercise.getId());
- assertThat(savedTokenUsageTraces.getFirst().getCourseId()).isEqualTo(course.getId());
+ assertThat(savedTokenUsageTraces.get(0).getServiceType()).isEqualTo(LLMServiceType.IRIS);
+ assertThat(savedTokenUsageTraces.get(0).getExerciseId()).isEqualTo(exercise.getId());
+ assertThat(savedTokenUsageTraces.get(0).getCourseId()).isEqualTo(course.getId());

And similarly for the second test:

- assertThat(savedTokenUsageTraces.getFirst().getServiceType()).isEqualTo(LLMServiceType.IRIS);
- assertThat(savedTokenUsageTraces.getFirst().getExerciseId()).isEqualTo(exercise.getId());
- assertThat(savedTokenUsageTraces.getFirst().getIrisMessageId()).isEqualTo(messageToSend.getId());
- assertThat(savedTokenUsageTraces.getFirst().getCourseId()).isEqualTo(course.getId());
+ assertThat(savedTokenUsageTraces.get(0).getServiceType()).isEqualTo(LLMServiceType.IRIS);
+ assertThat(savedTokenUsageTraces.get(0).getExerciseId()).isEqualTo(exercise.getId());
+ assertThat(savedTokenUsageTraces.get(0).getIrisMessageId()).isEqualTo(messageToSend.getId());
+ assertThat(savedTokenUsageTraces.get(0).getCourseId()).isEqualTo(course.getId());

Also applies to: 189-191

src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (2)

48-48: 🛠️ Refactor suggestion

Consider making database operations asynchronous

The // TODO comments at lines 48 and 78 indicate that saveLLMTokenUsage and appendRequestsToTrace should ideally be executed asynchronously to improve performance. Implementing asynchronous operations can enhance responsiveness, especially if these methods are called frequently or involve significant processing time.

Would you like assistance in refactoring these methods to be asynchronous or opening a GitHub issue to track this task?

Also applies to: 78-78


45-45: ⚠️ Potential issue

Correct the Javadoc parameter description

The Javadoc comment references IrisTokenUsageBuilder, but the correct class name is LLMTokenUsageBuilder. This inconsistency can cause confusion and should be updated.

Apply this diff to fix the Javadoc:

- * @param builderFunction of type Function<IrisTokenUsageBuilder, IrisTokenUsageBuilder> using IrisTokenUsageBuilder
+ * @param builderFunction of type Function<LLMTokenUsageBuilder, LLMTokenUsageBuilder> using LLMTokenUsageBuilder
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

     * @param builderFunction of type Function<LLMTokenUsageBuilder, LLMTokenUsageBuilder> using LLMTokenUsageBuilder
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaFeedbackSuggestionsService.java (1)

166-175: ⚠️ Potential issue

Handle potential ClassCastException when casting to StudentParticipation

In the storeTokenUsage method, the participation is cast to StudentParticipation without checking its actual type. If the participation is not an instance of StudentParticipation, this will result in a ClassCastException.

Consider checking the type of the participation before casting:

 private void storeTokenUsage(Exercise exercise, Submission submission, ResponseMetaDTO meta, Boolean isPreliminaryFeedback) {
     if (meta == null) {
         return;
     }
     Long courseId = exercise.getCourseViaExerciseGroupOrCourseMember().getId();
     Long exerciseId = exercise.getId();
-    Long userId = ((StudentParticipation) submission.getParticipation()).getStudent().map(User::getId).orElse(null);
+    Long userId = null;
+    if (submission.getParticipation() instanceof StudentParticipation studentParticipation) {
+        userId = studentParticipation.getStudent().map(User::getId).orElse(null);
+    }
     llmTokenUsageService.saveAthenaTokenUsage(courseId, exerciseId, userId, meta, isPreliminaryFeedback);
 }

This ensures that you safely handle cases where the participation might not be a StudentParticipation.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    private void storeTokenUsage(Exercise exercise, Submission submission, ResponseMetaDTO meta, Boolean isPreliminaryFeedback) {
        if (meta == null) {
            return;
        }
        Long courseId = exercise.getCourseViaExerciseGroupOrCourseMember().getId();
        Long exerciseId = exercise.getId();
        Long userId = null;
        if (submission.getParticipation() instanceof StudentParticipation studentParticipation) {
            userId = studentParticipation.getStudent().map(User::getId).orElse(null);
        }
        llmTokenUsageService.saveAthenaTokenUsage(courseId, exerciseId, userId, meta, isPreliminaryFeedback);
    }

@github-actions github-actions bot removed tests database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. core Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module labels Oct 21, 2024
* DTO representing an individual LLM call.
*/
@JsonInclude(JsonInclude.Include.NON_NULL)
public record ResponseMetaLLMCallDTO(String modelName, Integer inputTokens, Integer outputTokens, Integer totalTokens) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these numbers have to be nullable?

* DTO representing the total usage metrics.
*/
@JsonInclude(JsonInclude.Include.NON_NULL)
public record ResponseMetaTotalUsageDTO(Integer inputTokens, Integer outputTokens, Integer totalTokens) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these numbers have to be nullable?

bassner and others added 7 commits October 21, 2024 16:58
…' into feature/athena/track-token-usage

# Conflicts:
#	src/main/java/de/tum/cit/aet/artemis/athena/dto/ResponseMetaDTO.java
#	src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaFeedbackSuggestionsService.java
@github-actions github-actions bot added the tests label Oct 21, 2024
@github-actions github-actions bot added core Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module labels Oct 21, 2024
@FelixTJDietrich
Copy link
Contributor Author

I messed up with a rebase. Will reopen another PR

@FelixTJDietrich FelixTJDietrich deleted the feature/athena/track-token-usage branch October 21, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
athena Pull requests that affect the corresponding module core Pull requests that affect the corresponding module iris Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants