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

Azure load tests #1665

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Azure load tests #1665

wants to merge 39 commits into from

Conversation

somesylvie
Copy link
Contributor

Description

Describe what changed in this PR at a high level.

Issue

#1122

Checklist

  • I have added tests to cover my changes
  • I have added logging where useful (with appropriate log level)
  • I have added JavaDocs where required
  • I have updated the documentation accordingly

Note: You may remove items that are not applicable

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1122 - Partially compliant

Fully compliant requirements:

  • Sign Flexion and CDC up for the Locust preview program.
  • Implement load test in internal.

Not compliant requirements:

  • Set up a GH Action to run tests on a schedule.
  • Update existing ADR and/or create new one.
  • Update readme with how to run and modify load tests in deployed envs.
  • Configure test criteria (failure thresholds) in Azure.
  • Set up mock RS endpoints for load testing.
  • Rename internal load testing.
  • Set-up load test for staging.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Thread Safety
The use of ApplicationContext.clearThreadRegistrations() needs careful review to ensure thread safety and proper resource cleanup.

Conditional Logic
The conditional logic for registering MockRSEndpointClient based on the 'Load-Test' header and REPORT_STREAM_URL_PREFIX property should be validated for correctness and security implications.

THREAD_OBJECT_MAP.set(threadObjectMap);
}

public static void clearThreadRegistrations() {

Choose a reason for hiding this comment

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

Consider using a more robust thread-local cleanup strategy. The current use of THREAD_OBJECT_MAP.remove() might lead to memory leaks if not all paths that add to the thread-local store also ensure to clear it after use. [important]

@@ -226,6 +227,12 @@ protected DomainResponse handleMessageRequest(
boolean markMetadataAsFailed = false;
String errorMessage = "";

if ("True".equals(request.getHeaders().get("Load-Test"))

Choose a reason for hiding this comment

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

Refactor the conditional logic for registering the MockRSEndpointClient to ensure it's only applied under the correct conditions and does not interfere with production settings. [important]

@@ -118,6 +121,10 @@ def test_start(environment):

@events.quitting.add_listener
def assert_stats(environment):
if in_azure:

Choose a reason for hiding this comment

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

Ensure that the environment-specific paths and configurations, such as file paths and environment variables, are documented and validated to prevent runtime errors in different environments. [medium]

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure thread-local registrations are always cleared after use to prevent memory leaks or context pollution

Ensure that the ApplicationContext.registerForThread method is called within a
try-finally block to guarantee that ApplicationContext.clearThreadRegistrations is
called even if an exception occurs, preventing potential memory leaks or incorrect
context data during subsequent operations.

etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/EtorDomainRegistration.java [230-234]

 if ("True".equals(request.getHeaders().get("Load-Test"))
         && ApplicationContext.isPropertyPresent("REPORT_STREAM_URL_PREFIX")) {
-    ApplicationContext.registerForThread(
-            RSEndpointClient.class, MockRSEndpointClient.getInstance());
+    try {
+        ApplicationContext.registerForThread(
+                RSEndpointClient.class, MockRSEndpointClient.getInstance());
+        // processing code here
+    } finally {
+        ApplicationContext.clearThreadRegistrations();
+    }
 }
Suggestion importance[1-10]: 8

Why: The suggestion to use a try-finally block for thread-local registrations is crucial for ensuring that resources are cleaned up properly, especially in a multi-threaded environment. This prevents memory leaks and ensures that no stale or incorrect context data persists across different operations, which could lead to hard-to-track bugs.

8

Co-authored-by: pluckyswan <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: James Herr <[email protected]>
Co-authored-by: Sylvie <[email protected]>
Co-authored-by: jcrichlake <[email protected]>
Co-authored-by: pluckyswan <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: James Herr <[email protected]>
Co-authored-by: Sylvie <[email protected]>
Co-authored-by: jcrichlake <[email protected]>
jherrflexion and others added 7 commits January 7, 2025 13:58
Co-Authored-By: Sylvie <[email protected]>
Co-Authored-By: halprin <[email protected]>
Co-Authored-By: jcrichlake <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Sylvie <[email protected]>
Co-Authored-By: halprin <[email protected]>
Co-Authored-By: jcrichlake <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Sylvie <[email protected]>
Co-Authored-By: halprin <[email protected]>
Co-Authored-By: jcrichlake <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
@jherrflexion jherrflexion marked this pull request as ready for review January 10, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants