Add Tests for Storage Extension and File Rotation #1427

vasireddy99 · 2023-10-06T23:22:11Z

Description:
PR to add Tests for Storage Extension Restart scenarios and File Rotation

Thread.sleep is used to give enough time for file operations, collector(stop/start).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

rapphil · 2023-10-12T06:06:21Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+        } catch (IOException e) {
+            throw new RuntimeException("Error writing to File A.", e);
+        }
+        Thread.sleep(5000);


you van validate the log entries so far here so that you don't need to add a sleep. that will block in a safe way before you move with the rest of the tests.

I agree that validation can be done, but the thread here is making sure to give enough time for the file operations to be done. So even if we do the validation without the thread, since the log file is not generated it would lead FileNotFoundException.

Is writing to the file a blocking operation? Why do you need to wait? What file operations are you referring to?

what operations needs to be done?

File operations i meant here are to create and write data to File A etc. it takes considerable time for creating and writing to file.
So without the thread.sleep the collector is not able to parse log line, since it seems the file testlogA.log didn't have the line written to it yet.

java.lang.AssertionError: Expecting actual: ["Message in renamed file - line 1", "Message in renamed file - line 2", "Message in renamed file - line 3"] to contain exactly in any order: ["Message in File A", "Message in renamed file - line 1", "Message in renamed file - line 2", "Message in renamed file - line 3"] but could not find the following elements: ["Message in File A"]

that is why I suggested that you add a validateLogs instead of a Thread.sleep to validate only "Message in File A". It will block until the lines are sent to cwlogs. This will guarantee that it is safe to continue with the tests.

Committed the changes.

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

adot-testbed/app/src/test/resources/configurations/config-fileRotation.yaml

bryan-aguilar · 2023-10-12T16:09:36Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+    private Path logDirectory;
    private GenericContainer<?> collector;


How does test parallelism work with the framework we are using? Does having these in the Class scope have a potential to cause conflicts if test cases are ran in parallel?

I have tried running with ./gradlew test --rerun-tasks --info --parallel to have optimal number of threads to use no conlicts were seen. We are also using specific path/file within the directory so parallel wouldn't be a problem IMO

bryan-aguilar · 2023-10-12T16:12:23Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+        String logStreamName = "storageExtension-logstream-" + uniqueID;
+        collector = createAndStartCollector("/configurations/config-storageExtension.yaml", logStreamName);
+        File tempFile = new File(logDirectory.toString(), "storageExtension.log");
+        Thread.sleep(5000);


Why sleep here?

The sleep is similar as here, its just to wait for the collector to start watching the file.

bryan-aguilar · 2023-10-12T16:14:05Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+            fileWriter.write("First Message after collector is stopped" + "\n");
+            fileWriter.write("Second Message after the collector is stopped" + "\n");
+            fileWriter.write("Third Message after the collector is stopped" + "\n");
+            fileWriter.flush();


Why is flush called inside the try block here but outside of it on line 144?

Actually no specific reason, modified .

bryan-aguilar · 2023-10-12T16:14:59Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+        String expectedLogPath = logDirectory.toString();
+        logFilePaths.add(expectedLogPath + "/storageExtension.log");
+
+        validateLogs(logStreamName , logFilePaths);


Why do we need to validate logs twice? Could we run through the start -> write -> stop -> write -> start -> stop cycle then validate once?

validate is behaving like a sleep here. we need to make sure that logs were read before stopping the collector.

we need to make sure that logs were read before stopping the collector.
I tend to agree with that. After changes now it is doing - start -> write -> validate->stop -> write -> start -> stop and then it validates again.

bryan-aguilar · 2023-10-12T16:16:43Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+        } catch (IOException e) {
+            throw new RuntimeException("Error writing to File A.", e);
+        }
+        Thread.sleep(5000);


Is writing to the file a blocking operation? Why do you need to wait? What file operations are you referring to?

bryan-aguilar · 2023-10-12T16:20:58Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+                System.out.println("Actual Logs - Log lines From Cloudwatch");
+                messageToValidate.forEach(System.out::println);
+
+                System.out.println("Expected logs- Log lines from log file");
+                lines.forEach(System.out::println);


Let's be more thoughtful about what we are printing to std out. We should only need to print if there is a failure. And in the case of the failure it should be descriptive to tell us what was expected and what was actually given.

Sure, I have added it for easy lookout on whats' validated. if we should only need to print during failure then the assertions will log the actual/expected when in failure, removed the print statements

rapphil · 2023-10-12T16:29:14Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+        for (String logFilePath : logFilePaths) {
+            InputStream inputStream;
+            //Check whether the filePath is from resource folder.
+            if (getClass().getResource(logFilePath) != null) {


this is still the boolean flag behaviour disguised.....

my suggestion was to change the function signature to use

void validateLogs(String testLogStreamName, List<InputStream> inputStreams) throws Exception {

Another option is to create two functions, one that should only be used for resources paths and another that is used only for local paths.

Oh, now I see the suggestion. Modified.

rapphil · 2023-10-13T04:09:51Z

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java

+        printWriter.println("Third Message, collector is running");
+        printWriter.flush();
+
+        Thread.sleep(5000);


I bet this could be removed and the tests would still succeed.

vasireddy99 requested a review from a team as a code owner October 6, 2023 23:22

vasireddy99 marked this pull request as draft October 6, 2023 23:22

vasireddy99 added 2 commits October 10, 2023 15:22

Add Storage Extension test case, Changes to validator

287678e

Rebased the branch

4e64893

vasireddy99 force-pushed the Add-logstests branch from 5523d4e to 4e64893 Compare October 10, 2023 23:27

Modified Storage extension test case, modifed thread wait time

f5e5d3f

vasireddy99 marked this pull request as ready for review October 11, 2023 18:45

remove privilaged access start, handle exception

5e76506

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java Outdated Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java Outdated Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java Outdated Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java Outdated Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/java/software/amazon/adot/testbed/LogsTests.java Outdated Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/resources/configurations/config-fileRotation.yaml Outdated Show resolved Hide resolved

rapphil reviewed Oct 12, 2023

View reviewed changes

adot-testbed/app/src/test/resources/configurations/config-fileRotation.yaml Outdated Show resolved Hide resolved

vasireddy99 added 2 commits October 11, 2023 23:57

Modify Usage of Validator - no boolean, tweak configs

497618c

Remove extra threads

473629e

bryan-aguilar reviewed Oct 12, 2023

View reviewed changes

rapphil reviewed Oct 12, 2023

View reviewed changes

vasireddy99 added 4 commits October 12, 2023 12:57

Remove Exception block, send input stream to validator

f283813

Use PrintWriter

06ba690

Validate 2 in filerotation test case

8e241fc

Addback the validation 2 in storage extension test case

f79acb4

rapphil reviewed Oct 13, 2023

View reviewed changes

rapphil approved these changes Oct 13, 2023

View reviewed changes

Rm extra thread, Use right feature gates.

25b705a

vasireddy99 merged commit c688310 into aws-observability:terraform Oct 13, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tests for Storage Extension and File Rotation #1427

Add Tests for Storage Extension and File Rotation #1427

vasireddy99 commented Oct 6, 2023 •

edited

Loading

rapphil Oct 12, 2023

vasireddy99 Oct 12, 2023

bryan-aguilar Oct 12, 2023

rapphil Oct 12, 2023

vasireddy99 Oct 12, 2023

rapphil Oct 12, 2023

vasireddy99 Oct 13, 2023

bryan-aguilar Oct 12, 2023

vasireddy99 Oct 12, 2023

bryan-aguilar Oct 12, 2023

vasireddy99 Oct 12, 2023

bryan-aguilar Oct 12, 2023

vasireddy99 Oct 12, 2023

bryan-aguilar Oct 12, 2023

rapphil Oct 12, 2023

vasireddy99 Oct 13, 2023 •

edited

Loading

bryan-aguilar Oct 12, 2023

bryan-aguilar Oct 12, 2023

vasireddy99 Oct 12, 2023

rapphil Oct 12, 2023

vasireddy99 Oct 12, 2023

rapphil Oct 13, 2023

		private Path logDirectory;
		private GenericContainer<?> collector;

Add Tests for Storage Extension and File Rotation #1427

Add Tests for Storage Extension and File Rotation #1427

Conversation

vasireddy99 commented Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasireddy99 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasireddy99 commented Oct 6, 2023 •

edited

Loading

vasireddy99 Oct 13, 2023 •

edited

Loading