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

fix: handle valid JSON messages which are not GreengrassLogMessages #214

Merged
merged 2 commits into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -188,21 +188,21 @@

// Need to read more lines until we get a complete log line. Let's add this to the SB.
data.append(partialLogLine);
} catch (ClosedByInterruptException e) {
Thread.currentThread().interrupt();
logger.atInfo().log("Interrupted while reading log");
componentLogFileInformation.getLogFileInformationList().remove(0);
break;

Check warning on line 195 in src/main/java/com/aws/greengrass/logmanager/CloudWatchAttemptLogsProcessor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/aws/greengrass/logmanager/CloudWatchAttemptLogsProcessor.java#L191-L195

Added lines #L191 - L195 were not covered by tests
} catch (IOException e) {
logger.atError().cause(e).log("Unable to read file {}", logFile.getAbsolutePath());
componentLogFileInformation.getLogFileInformationList().remove(0);
break;
}
}
} catch (ClosedByInterruptException e) {
Thread.currentThread().interrupt();
logger.atInfo().log("Interrupted while reading log");
componentLogFileInformation.getLogFileInformationList().remove(0);

Check warning on line 205 in src/main/java/com/aws/greengrass/logmanager/CloudWatchAttemptLogsProcessor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/aws/greengrass/logmanager/CloudWatchAttemptLogsProcessor.java#L202-L205

Added lines #L202 - L205 were not covered by tests
} catch (IOException e) {
// File probably does not exist.
logger.atError().cause(e).log("Unable to read file {}", logFile.getAbsolutePath());
Expand Down Expand Up @@ -331,7 +331,11 @@
return Optional.empty();
}
try {
return Optional.ofNullable(DESERIALIZER.readValue(data, GreengrassLogMessage.class));
GreengrassLogMessage message = DESERIALIZER.readValue(data, GreengrassLogMessage.class);
if (message == null || Utils.isEmpty(message.getLevel())) {
return Optional.empty();
}
return Optional.of(message);
} catch (JsonProcessingException ignored) {
// If unable to deserialize, then we treat it as a normal log line and do not need to smartly upload.
return Optional.empty();
Expand Down Expand Up @@ -359,7 +363,7 @@
GreengrassLogMessage logMessage) {
Level currentLogLevel = Level.valueOf(logMessage.getLevel());
if (currentLogLevel.toInt() < desiredLogLevel.toInt()) {
return new Pair(false, new AtomicInteger());
return new Pair<>(false, new AtomicInteger());
}
return addNewLogEvent(totalBytesRead, attemptLogInformation, data, dataSize,
Instant.ofEpochMilli(logMessage.getTimestamp()));
Expand Down Expand Up @@ -439,7 +443,7 @@

currChunk++;
}
return new Pair(reachedMaxBatchSize, currBytesRead);
return new Pair<>(reachedMaxBatchSize, currBytesRead);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,8 @@ void GIVEN_one_components_two_file_less_than_max_WHEN_merge_THEN_reads_and_merge
String logStream2 = "/2020/12/18/thing/testThing";
assertTrue(attempt.getLogStreamsToLogEventsMap().containsKey(logStream));
assertTrue(attempt.getLogStreamsToLogEventsMap().containsKey(logStream2));
CloudWatchAttemptLogInformation logEventsForStream1 = attempt.getLogStreamsToLogEventsMap().get(logStream);
CloudWatchAttemptLogInformation logEventsForStream2 = attempt.getLogStreamsToLogEventsMap().get(logStream2);
CloudWatchAttemptLogInformation logEventsForStream1 = attempt.getLogStreamsToLogEventsMap().remove(logStream);
CloudWatchAttemptLogInformation logEventsForStream2 = attempt.getLogStreamsToLogEventsMap().remove(logStream2);
assertNotNull(logEventsForStream1.getLogEvents());
assertEquals(13, logEventsForStream1.getLogEvents().size());
assertTrue(logEventsForStream1.getAttemptLogFileInformationMap().containsKey(fileHash1));
Expand All @@ -494,10 +494,14 @@ void GIVEN_one_components_two_file_less_than_max_WHEN_merge_THEN_reads_and_merge
}

assertNotNull(logEventsForStream2.getLogEvents());
assertEquals(4, logEventsForStream2.getLogEvents().size());
assertEquals(3, logEventsForStream2.getLogEvents().size());
// Read the 1 remaining cloudwatch log stream which will have today's date because the log lines
// had no parsed timestamp.
assertEquals(2, attempt.getLogStreamsToLogEventsMap()
.get(attempt.getLogStreamsToLogEventsMap().keySet().stream().findFirst().get()).getLogEvents().size());
assertTrue(logEventsForStream2.getAttemptLogFileInformationMap().containsKey(fileHash2));
assertEquals(0, logEventsForStream2.getAttemptLogFileInformationMap().get(fileHash2).getStartPosition());
assertEquals(1239, logEventsForStream2.getAttemptLogFileInformationMap().get(fileHash2).getBytesRead());
assertEquals(1237, logEventsForStream2.getAttemptLogFileInformationMap().get(fileHash2).getBytesRead());
assertEquals("TestComponent", logEventsForStream2.getComponentName());
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{"contexts":{"component":"demo","device":"asdf"},"eventType":"th1-event","level":"INFO","loggerName":"com.aws.greengrass.logging.examples.LoggerDemo","message":"test th1 info","timestamp":1608292800000}
{"contexts":{"component":"demo","device":"asdf"},"eventType":"th1-event","level":null,"loggerName":"com.aws.greengrass.logging.examples.LoggerDemo","message":"test th1 info","timestamp":1608292800000}
{"contexts":{"component":"demo","device":"asdf"},"eventType":"th2-event","level":"INFO","loggerName":"com.aws.greengrass.logging.examples.LoggerDemo","message":"test th2 info","timestamp":1608292800000}
{"contexts":{"component":"th1-override","device":"asdf"},"level":"DEBUG","loggerName":"com.aws.greengrass.logging.examples.LoggerDemo","message":"test th1 debug","timestamp":1608292800000}
{"contexts":{"component":"demo","device":"asdf"},"level":"INFO","loggerName":"com.aws.greengrass.logging.examples.LoggerDemo","message":"test main info","timestamp":1608292800000}
{"cause":{"localizedMessage":"some error","message":"some error","stackTrace":[{"className":"com.aws.greengrass.logging.examples.LoggerDemo","fileName":"LoggerDemo.java","lineNumber":56,"methodName":"main","nativeMethod":false}],"suppressed":[]},"contexts":{"key2":"value2","component":"demo","device":"asdf"},"eventType":"error-event","level":"ERROR","loggerName":"com.aws.greengrass.logging.examples.LoggerDemo","message":"test error","timestamp":1608292800000}
{"something which parses": "as json, but isn't a GreengrassStructuredLogMessage"}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import org.junit.jupiter.api.io.TempDir;
import org.mockito.junit.jupiter.MockitoExtension;

import java.io.File;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.function.Consumer;
Expand All @@ -17,19 +16,18 @@ public class LogGeneratorTest {
@TempDir
static Path tempPath;
String logFileName = "localtest";
String logFileExtention = "log";
String logWriteFreqSeconds = "0.1";
String logWriteFreqMs = "100";
Copy link

Choose a reason for hiding this comment

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

Is someone going to think this is milliseconds and not messages per second?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is milliseconds. 0.1 seconds is 100 milliseconds.

String totalLogNumbers = "50";
String fileSizeBytes = "1024";
String fileSizeUnit = "KB";
String componentName = "com.aws.greengrass.artifacts.LogGenerator";
String activeFileName = logFileName + "." + logFileExtention;
String activeFileName = logFileName + ".log";
Copy link

Choose a reason for hiding this comment

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

Why are we moving from a variable to a hard coded string?

Copy link
Member Author

@MikeDombo MikeDombo Oct 26, 2023

Choose a reason for hiding this comment

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

This is a test (for a test), variable wasn't a variable but a constant.


@Test
void GIVEN_request_THEN_log_file_Created()
throws ClassNotFoundException, IllegalAccessException, InstantiationException {

String[] args = {logFileName, logFileExtention, fileSizeBytes, fileSizeUnit, logWriteFreqSeconds,
String[] args = {logFileName, fileSizeBytes, fileSizeUnit, logWriteFreqMs,
totalLogNumbers, tempPath.toString()};
((Consumer<String[]>) Class.forName(componentName).newInstance()).accept(args);

Expand All @@ -39,25 +37,4 @@ void GIVEN_request_THEN_log_file_Created()
assertTrue(Arrays.asList(pathnames).contains(activeFileName));
assertTrue(tempPath.resolve(activeFileName).toFile().length() > 0);
}

/*
if tempPath is empty string, then the log files would be generated in your local machine. '
This can be used for manually testing if logs are correctly written
*/
@Test
void GIVEN_empty_targetFilePath_in_Paras_THEN_default_path_is_local()
throws ClassNotFoundException, InstantiationException, IllegalAccessException {

String[] args = {logFileName, logFileExtention, fileSizeBytes, fileSizeUnit, logWriteFreqSeconds,
totalLogNumbers, ""};
((Consumer<String[]>) Class.forName(componentName).newInstance()).accept(args);

String localPath = System.getProperty("user.dir");
File directory = new File(localPath);
String[] pathnames = directory.list();
assertTrue(pathnames.length >= 1);
assertTrue(Arrays.asList(pathnames).contains(activeFileName));
assertTrue(new File(localPath + "/" + activeFileName).length() > 0);

}
}
Loading