-
Notifications
You must be signed in to change notification settings - Fork 859
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
EmfMetricPublisher class added #5752
base: feature/master/emf-metric-publisher
Are you sure you want to change the base?
EmfMetricPublisher class added #5752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some high level comments, still reviewing the implementation
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java
Show resolved
Hide resolved
import software.amazon.awssdk.utils.Logger; | ||
|
||
/** | ||
* A metric publisher implementation that converts metrics into CloudWatch Embedded Metric Format (EMF). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* | ||
* <p>Example usage:</p> | ||
* <pre> | ||
* EmfMetricPublisher publisher = PublisherBuilder.dimensions(CoreMetric.SERVICE_ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
* | ||
* <p>If this is not specified, {@code false} will be used. | ||
*/ | ||
public Builder unitTest(boolean unitTest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java
Outdated
Show resolved
Hide resolved
...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java
Outdated
Show resolved
Hide resolved
...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java
Outdated
Show resolved
Hide resolved
...sher/src/test/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisherTest.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
void testConvertMetricCollectionToEMF_EmptyCollection(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using parameterized tests if we are just testing with different input
Quality Gate failedFailed conditions |
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/EmfMetricPublisher.java
Outdated
Show resolved
Hide resolved
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java
Outdated
Show resolved
Hide resolved
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java
Outdated
Show resolved
Hide resolved
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java
Outdated
Show resolved
Hide resolved
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java
Outdated
Show resolved
Hide resolved
// Write CloudWatchMetrics array | ||
jsonWriter.writeFieldName("CloudWatchMetrics"); | ||
jsonWriter.writeStartArray(); | ||
jsonWriter.writeStartObject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? It seems this CloudWatchMetrics only has one object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java
Outdated
Show resolved
Hide resolved
...ublisher/src/main/java/software/amazon/awssdk/metrics/publishers/emf/MetricEmfConverter.java
Outdated
Show resolved
Hide resolved
<version>2.29.35-SNAPSHOT</version> | ||
</parent> | ||
|
||
<artifactId>emf-metric-publisher</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License