feat: wire EMF file output into NucleusEmitter publish flow#22
feat: wire EMF file output into NucleusEmitter publish flow#22utsavvyas wants to merge 2 commits into
Conversation
d915fbc to
31ea09a
Compare
| public class EmfFileWriter implements Closeable { | ||
|
|
||
| private static final DateTimeFormatter HOUR_FORMAT = | ||
| DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH").withZone(ZoneOffset.UTC); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
DateTimeFormatter.ofPattern
Note: “I18N doesn’t apply because this use case is internal” is a misconception. For formatting that isn’t user-facing: Set Locale to Locale.ROOT, set TimeZone to UTC, and check if your format is predefined in DateTimeFormatter.
Picture strings (for example, MM-dd-yyyy and HH:mm:ss) that you specify to represent the date won’t be appropriate for all locales, and should not be hardcoded. Different locales use different date and time formats, positioning, punctuation, and even numbers themselves. So, the developer or a translator has to adapt the string for each locale's specific needs. Otherwise, the results could look strange or wrong to users in another language or locale.
Also, DateTimeFormatter.ofPattern shouldn’t be used to fix issues with SimpleDateFormat, since the two produce similar results. The Internationalization channel on Broadcast has two videos on the challenges of using SimpleDateFormat for User-displayed customer experience (CX) and Application-internal timestamps and application-specific/application-defined values.
Suggested solutions:
For user-facing date and time:
- [PREFERRED] MessageFormat supports "skeletons" that can automatically select the right pattern for the customer's locale. To learn more, see How to format dates and the MessageFormat Cheat Sheet.
MessageFormat fmt = new MessageFormat(pattern, locale); Object[] args = { new Date() }; return fmt.format(args);
If you want to get the string translated, create a resource file for the date and time pattern. Use Panther to get the date and time pattern string translated. MessageFormat is built into Panther. See the DateTimeFormatter.ofPattern wiki.
- Consider using the ICU4J library
DateFormat.getPatternInstancemethod (which uses a date skeleton string) instead of the stock JavaDateFormatclass. Skeleton-based date and time formats adapt to the locale automatically.import com.ibm.icu.text.DateFormat; // note different import! DateFormat df = DateFormat.getPatternInstance("jmsz", pageLocale);
For application-internal timestamps:
-
Use a predefined ISO 8601 format like
ISO_INSTANT(which looks like2011-12-03T10:15:30Z) from DateTimeFormatter.String now = DateTimeFormatter.ISO_INSTANT.format(new Date().toInstant()); -
If your format is not predefined in DateTimeFormatter and you must use a custom format:
- Make sure you pass in the Locale (for non-user-facing, internal-processing usage, use
Locale.ROOT) - Set the timezone (for example,
UTC), if you want to ensure it is a certain timezone.
Example:
// your custom pattern goes here final String somePattern = "yyyy MM dd HH mm ss z"; // $NON-NLS-L$ Custom hard-coded pattern DateTimeFormatter myFormatter = DateTimeFormatter.ofPattern(somePattern, Locale.ROOT) // $NON-NLS-L$ custom pattern, invariant locale // only add this line if you want to ensure UTC (or some other time zone) .withZone(ZoneId.of("UTC")); // $NON-NLS-L$ forcing UTC myFormatter.format(/* some value */); - Make sure you pass in the Locale (for non-user-facing, internal-processing usage, use
| if (config.isEmfEnabled()) { | ||
| String thingName = Coerce.toString( | ||
| deviceConfiguration.getThingName()); | ||
| this.emfFileWriter = new EmfFileWriter( |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Problem
Analysis of this package determined that this line of code assigns a resource to a class field and there is no method that closes or returns this resource. This can result in a resource leak that slows down or crashes your system.
Fix
Create a method in this class that closes the following resource: emfFileWriter. After you use this resource, use this method to prevent a resource leak.
More info
View resource management guidelines at oracle.com (external link).
| config.outputDirectory((String) entry.getValue()); | ||
| break; | ||
| } | ||
| logger.error(OUTPUT_DIRECTORY_CONFIG_PARSE_ERROR_LOG, entry.getValue()); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against a allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
| break; | ||
| } | ||
| } | ||
| logger.error(OUTPUT_MODE_CONFIG_PARSE_ERROR_LOG, entry.getValue()); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against a allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
| break; | ||
| } | ||
| } | ||
| logger.error(METRICS_LEVEL_CONFIG_PARSE_ERROR_LOG, entry.getValue()); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against a allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
6b22832 to
9571992
Compare
bb89013 to
4817f2d
Compare
| close(); | ||
| Files.createDirectories(outputDirectory); | ||
| Path file = outputDirectory.resolve(fileName); | ||
| writer = Files.newBufferedWriter(file, StandardOpenOption.CREATE, StandardOpenOption.APPEND); |
There was a problem hiding this comment.
Since we are checking writer != null in line 96, we also need to make sure in here when we create this writer is not null for whatever reason so next check it will go to the line 96 check correctly.
| for (String line : context.serialize()) { | ||
| writer.write(line); | ||
| writer.newLine(); |
There was a problem hiding this comment.
Do we know if context.serialize() already contains the newline?
| assertTrue(pubsubLog.await(30, TimeUnit.SECONDS), "Pub/sub publish log detected."); | ||
|
|
||
| // Poll for EMF file creation (publish fires immediately with initialDelay=0) | ||
| Path emfDir = Paths.get("/tmp/greengrass/telemetry"); |
There was a problem hiding this comment.
will this tmp file get cleanup after the test?
4817f2d to
6e289fb
Compare
6e289fb to
d24c6b4
Compare
| if (outputModeChanged || outputDirectoryChanged) { | ||
| if (newConfiguration.isEmfEnabled()) { | ||
| this.emfFileWriter = new EmfFileWriter(getThingName(), | ||
| Paths.get(newConfiguration.getOutputDirectory())); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Path traversal vulnerability detected. User-controlled input in file paths
can allow attackers to access files outside intended directories using
../ sequences. Secure your code by using framework functions like
getCanonicalPath(), normalize(), or validating paths with
.startsWith() checks. Learn more:
https://cwe.mitre.org/data/definitions/22.html
be86d0c to
242ec95
Compare
Issue #, if available:
N/A — PR 4 of 4 in the extended metrics stacked series.
Description of changes:
Wire EMF file output into
NucleusEmitterpublish flow and update documentation:DeviceConfigurationto constructor forthingNameEmfFileWriterinhandleConfiguration()whenoutputModeoroutputDirectorychangesEmfFileWriterinpublishTelemetry()whenisEmfEnabled()EmfFileWriteron shutdown🤖 Assisted by AI
Why is this change necessary:
Completes the end-to-end flow: config (PR 1) → collect metrics (PR 2) → EMF writer (PR 3) → wire into publish loop (this PR). The emitter can now simultaneously publish via IPC/MQTT and write EMF files.
How was this change tested:
config_emf.yamltest configurationoutputMode: both— PubSub and EMF files working simultaneouslyAny additional information or context required to review the change:
Depends on PR #21. Final PR in the stack.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.