Skip to content

Allow putDimensionSet after setDimensions #62

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow putDimensionSet after setDimensions #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/main/java/software/amazon/cloudwatchlogs/emf/model/MetricDirective.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ List<Set<String>> getAllDimensionKeys() {
*/
void setDimensions(List<DimensionSet> dimensionSets) {
shouldUseDefaultDimension = false;
dimensions = dimensionSets;
dimensions = new ArrayList<>(dimensionSets);
Copy link
Member

Choose a reason for hiding this comment

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

This will cause an array copy and an unnecessary allocation. If ArrayList is required here, then we should probably accept it in the method signature rather than allocate a new one. However, that is a breaking change. So, I'm fine with this fix for now.

https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/19fb8f93c59dfd791f62d41f332db9e306bc1422/src/java.base/share/classes/java/util/ArrayList.java#L178-L189

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should fix the issue at the MetricsContext.java class, as the issue was introduced by the usage of Arrays.asList. We can change to use Lists.newArrayList in the Guava library. We need add guava to our dependencies to use this library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will cause an array copy and an unnecessary allocation. If ArrayList is required here, then we should probably accept it in the method signature rather than allocate a new one. However, that is a breaking change. So, I'm fine with this fix for now.

ArrayList isn't required. List is required -- at least a List implementation that has all of its methods implemented. Plus, would you want to directly accept a List and then hold a reference to it? Then the person who gave you the list can modify it while you're holding on to it.

I think we should fix the issue at the MetricsContext.java class, as the issue was introduced by the usage of Arrays.asList. We can change to use Lists.newArrayList in the Guava library. We need add guava to our dependencies to use this library.

I considered making the change to MetricsContext. To me it's more brittle. Changing the setDimension implementation fixes the problem once and for all. Changing 1 call site fixes it just for that case. For example I have a test that does metricDirective.setDimensions(Collections.singletonList(DimensionSet.of("Version", "1"))); that would still break if I just change the call site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the prompt feedback. Let me know what y'all think and I'll send a new revision.

Copy link
Contributor

@yaozhaoy yaozhaoy Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That sounds reasonable to me. :+1 Please go ahead to submit a new revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I had to update the commit, and then make a new remote branch since the hash has changed. I don't know enough about pull requests to know how to reference the new branch in this pull request. So I made a new one #63

}

/**
Expand Down
16 changes: 16 additions & 0 deletions src/test/java/software/amazon/cloudwatchlogs/emf/model/MetricDirectiveTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.util.Arrays;
import java.util.Collections;

Copy link
Member

Choose a reason for hiding this comment

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

Build is failing due to the added whitespace. Do you mind removing this?

Starting a Gradle Daemon (subsequent builds will be faster)
--
115 | > Task :compileJava
116 | > Task :processResources NO-SOURCE
117 | > Task :classes
118 | > Task :jar
119 | > Task :javadoc
120 | > Task :javadocJar
121 | > Task :sourcesJar
122 | > Task :assemble
123 | > Task :spotlessJava
124 | > Task :spotlessJavaCheck FAILED
125 |  
126 | FAILURE: Build failed with an exception.
127 |  
128 | * What went wrong:
129 | Execution failed for task ':spotlessJavaCheck'.
130 | > The following files had format violations:
131 | src/test/java/software/amazon/cloudwatchlogs/emf/model/MetricDirectiveTest.java
132 | @@ -23,7 +23,6 @@
133 | import?com.fasterxml.jackson.databind.ObjectMapper;
134 | import?java.util.Arrays;
135 | import?java.util.Collections;
136 | -
137 | import?org.junit.Test;
138 |  
139 | public?class?MetricDirectiveTest?{
140 | Run './gradlew :spotlessApply' to fix these violations.
141 |  
142 | * Try:
143 | Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Fixed that. Thanks.

import org.junit.Test;

public class MetricDirectiveTest {
Expand Down Expand Up @@ -126,4 +128,18 @@ public void testPutDimensionsWhenDefaultDimensionsDefined() throws JsonProcessin
serializedMetricDirective,
"{\"Dimensions\":[[\"Version\",\"Region\"],[\"Version\",\"Instance\"]],\"Metrics\":[],\"Namespace\":\"aws-embedded-metrics\"}");
}

@Test
public void testPutDimensionsAfterSetDimensions() throws JsonProcessingException {
MetricDirective metricDirective = new MetricDirective();
metricDirective.setDimensions(Collections.singletonList(DimensionSet.of("Version", "1")));
metricDirective.putDimensionSet(DimensionSet.of("Region", "us-east-1"));
metricDirective.putDimensionSet(DimensionSet.of("Instance", "inst-1"));

String serializedMetricDirective = objectMapper.writeValueAsString(metricDirective);

assertEquals(
serializedMetricDirective,
"{\"Dimensions\":[[\"Version\"],[\"Region\"],[\"Instance\"]],\"Metrics\":[],\"Namespace\":\"aws-embedded-metrics\"}");
}
}