EmfMetricPublisher class added #5752

Fred1155 · 2024-12-13T23:01:41Z

Motivation and Context

The existing CloudWatchMetricPublisher has limitations when used with AWS Lambda functions. When current implementation be applied to lambda functions, it can results in metrics being lost when the lambda function terminates before the scheduled batch upload. EMF allows metrics to be published through CloudWatch Logs using a structured JSON format, which CloudWatch automatically processes to extract and publish metrics. The proposed EMFMetricPublisher will convert SDK metric collections into EMF-formatted log entries, leveraging Lambda's as well as other execution environment that has built-in integration with cloudwatch logs such as ECS.

Modifications

Add a new metric publisher that transforms an metricCollection to a list of emf format Strings and publish it to CouldWatch using cloudwatch agents

Testing

Unit test covering different cases of metricCollection in convertMetricCollectionToEMF method
End to End test (In process)

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

…cPublisher

zoewangg

Left some high level comments, still reviewing the implementation

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

zoewangg · 2024-12-13T23:38:17Z

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

+import software.amazon.awssdk.utils.Logger;
+
+/**
+ * A metric publisher implementation that converts metrics into CloudWatch Embedded Metric Format (EMF).


Can we try markdown for javadoc?

https://docs.oracle.com/en/java/javase/23/javadoc/using-markdown-documentation-comments.html#GUID-0E4D6135-AAE9-4641-8427-CEE7F65E4F6E

zoewangg · 2024-12-13T23:39:36Z

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

+ *
+ * <p>Example usage:</p>
+ * <pre>
+ * EmfMetricPublisher publisher = PublisherBuilder.dimensions(CoreMetric.SERVICE_ID)


We should use @snippet for code sample for javadoc

https://github.com/aws/aws-sdk-java-v2/blob/master/docs/design/APIReference.md#style-guideline

zoewangg · 2024-12-13T23:42:30Z

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

+         *
+         * <p>If this is not specified, {@code false} will be used.
+         */
+        public Builder unitTest(boolean unitTest) {


We should use dependency injection for testing if we need to mock a dependency. eg: https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/GenericS3TransferManager.java#L121

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java

zoewangg · 2024-12-13T23:47:47Z

...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java

+    }
+
+    @Test
+    void testConvertMetricCollectionToEMF_EmptyCollection(){


Consider using parameterized tests if we are just testing with different input

…-v2 into bole/EMFMetricPublisher

sonarqubecloud · 2024-12-17T20:15:25Z

Quality Gate failed

Failed conditions
2 Security Hotspots
3.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

metric-publishers/emf-metric-publisher/pom.xml

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java

zoewangg · 2024-12-18T00:25:36Z

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java

+        // Write CloudWatchMetrics array
+        jsonWriter.writeFieldName("CloudWatchMetrics");
+        jsonWriter.writeStartArray();
+        jsonWriter.writeStartObject();


Is this correct? It seems this CloudWatchMetrics only has one object

I think so. In the example https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Embedded_Metric_Format_Specification.html, CloudWatchMetrics should have one object, because we only have one namespace and one set of dimensions

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java

zoewangg · 2024-12-18T00:52:57Z

metric-publishers/emf-metric-publisher/pom.xml

+        <version>2.29.35-SNAPSHOT</version>
+    </parent>
+
+    <artifactId>emf-metric-publisher</artifactId>


We don't have to change it now, but we should discuss it in API surface area review. Should we rename it to emf-metric-logger, since technically it's not publishing metrics?

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

zoewangg · 2024-12-23T20:59:23Z

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

+
+
+    private EmfMetricPublisher(Builder builder) {
+        EmfMetricConfiguration config = new EmfMetricConfiguration(


Per our previous discussion, I thought we are going to get rid of EmfMetricConfiguration class and just initialize fields in this class.

private EmfMetricPublisher(Builder builder) { this.namespace = builder.namespace == null ? DEFAULT_NAMESPACE : builder.namespace; ... this.dimensionStrings = resolveDimensionStrings(builder); }

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

zoewangg · 2024-12-23T21:04:29Z

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

+         * Configure the LogGroupName key that will be put into the emf log to this publisher. This is a required key for
+         * using the CloudWatch agent to send embedded metric format logs that tells the agent which log group to use.
+         *
+         * <p>If this is not specified, {@code /aws/emf/metrics} will be used.


Just curious, why is /aws/emf/metrics the default log group?

I can't find LogGroup in the specification, could you point to me where it's defined? https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Embedded_Metric_Format_Specification.html

...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java

zoewangg · 2024-12-23T22:20:59Z

...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java

+import software.amazon.awssdk.testutils.LogCaptor;
+
+
+public class EmfMetricPublisherTest extends LogCaptor.LogCaptorTestBase{


Can we add a test case where metricCollector throws an exception?

zoewangg · 2024-12-23T22:22:56Z

...test/java/software/amazon/awssdk/metrics/publishers/emf/internal/MetricEmfConverterTest.java

+import software.amazon.awssdk.metrics.MetricCollector;
+import software.amazon.awssdk.metrics.MetricLevel;
+
+public class MetricEmfConverterTest {


Can we add a test case where metric records size is larger than 100?

zoewangg · 2024-12-23T22:24:43Z

...test/java/software/amazon/awssdk/metrics/publishers/emf/internal/MetricEmfConverterTest.java

+        metricCollector.reportMetric(HttpMetric.HTTP_STATUS_CODE, 404);
+        List<String> emfLogs = metricEmfConverterDefault.convertMetricCollectionToEmf(metricCollector.collect());
+
+        for (String emfLog : emfLogs) {


If we only expect one log statement, let's just assert that instead of for loop. Same for other test cases.

assertThat(emfLogs).containsOnly(...)

zoewangg · 2024-12-23T22:26:10Z

...test/java/software/amazon/awssdk/metrics/publishers/emf/internal/MetricEmfConverterTest.java

+
+    private MetricEmfConverter metricEmfConverterDefault;
+    private MetricEmfConverter metricEmfConverterCustom;
+    private final EmfMetricConfiguration testConfigDefault = new EmfMetricConfiguration("AwsSdk/Test/JavaSdk2",


Is this the default config though?

zoewangg · 2024-12-23T22:26:54Z

...test/java/software/amazon/awssdk/metrics/publishers/emf/internal/MetricEmfConverterTest.java

+    }
+
+    @Test
+    void ConvertMetricCollectionToEMF_metricLevel(){


How is this test different from ConvertMetricCollectionToEMF_metricCategory?

Fred1155 added 5 commits December 11, 2024 09:27

EmfMetricPublisher class with basic unit test added

06e8f84

Edge cases handled

1e08463

javadoc for class added

d073d86

merge master branch

cf0af99

Java doc for methods added

5e34796

Fred1155 requested a review from a team as a code owner December 13, 2024 23:01

Merge branch 'feature/master/emf-metric-publisher' into bole/EMFMetri…

9f221ff

…cPublisher

zoewangg reviewed Dec 13, 2024

View reviewed changes

Fred1155 added 5 commits December 13, 2024 16:51

minor unit test changes

c8ea4aa

Merge branch 'bole/EMFMetricPublisher' of github.com:aws/aws-sdk-java…

79cf7d4

…-v2 into bole/EMFMetricPublisher

Change from using jacksonCore directly to using jsonWriter

04c837f

EmfMetricConfiguration class added

2920596

MetricEmfConverter class added

c79e15f

Fred1155 added 3 commits December 17, 2024 13:02

new module checklist done

e9320b0

checkStyle fixed

94e70a8

minor build problem fixed

9254b13

zoewangg reviewed Dec 18, 2024

View reviewed changes

Fred1155 added 4 commits December 19, 2024 15:07

internal package added

980fdd6

fixed the issue passing raw objects

6b36d4e

added java clock to unit test

02a1036

added unit test for publish method

bb7f690

zoewangg reviewed Dec 23, 2024

View reviewed changes

minor changes

f3084f3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EmfMetricPublisher class added #5752

EmfMetricPublisher class added #5752

Fred1155 commented Dec 13, 2024 •

edited

Loading

zoewangg left a comment

zoewangg Dec 13, 2024

zoewangg Dec 13, 2024

zoewangg Dec 13, 2024

zoewangg Dec 13, 2024

sonarqubecloud bot commented Dec 17, 2024

zoewangg Dec 18, 2024

Fred1155 Dec 18, 2024

zoewangg Dec 18, 2024

zoewangg Dec 23, 2024

zoewangg Dec 23, 2024

zoewangg Dec 23, 2024

zoewangg Dec 23, 2024

zoewangg Dec 23, 2024

zoewangg Dec 23, 2024

zoewangg Dec 23, 2024



		private EmfMetricPublisher(Builder builder) {
		EmfMetricConfiguration config = new EmfMetricConfiguration(

		import software.amazon.awssdk.testutils.LogCaptor;


		public class EmfMetricPublisherTest extends LogCaptor.LogCaptorTestBase{

EmfMetricPublisher class added #5752

Are you sure you want to change the base?

EmfMetricPublisher class added #5752

Conversation

Fred1155 commented Dec 13, 2024 • edited Loading

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

zoewangg left a 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

sonarqubecloud bot commented Dec 17, 2024

Quality Gate failed

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

Fred1155 commented Dec 13, 2024 •

edited

Loading