Add tests for Metric Rollup Addition of [RemoteService, RemoteTarget, ...] #729
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #729 +/- ##
=============================================
- Coverage 85.71% 50.87% -34.84%
- Complexity 19 266 +247
=============================================
Files 3 39 +36
Lines 49 1315 +1266
Branches 5 144 +139
=============================================
+ Hits 42 669 +627
- Misses 3 614 +611
- Partials 4 32 +28 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Overall seems to be working but there's some design questions and investigations into the unintended effects of the way the logic works to check.
Also, do not merge this code until CW Agent performs their release, otherwise you will cause canary to break leading to sev 2.
| new Pair<>(CloudWatchService.REMOTE_SERVICE_DIMENSION, "AWS.SDK.S3"), | ||
| new Pair<>(CloudWatchService.REMOTE_TARGET_DIMENSION, "e2e-test-bucket-name"))); | ||
|
|
||
| // Populate actualMetricList with each set of dimension filters |
There was a problem hiding this comment.
| // Populate actualMetricList with each set of dimension filters | |
| // Populate actualMetricList with metrics that pass through one of the dimension filters |
| expectedMetricList, | ||
| actualMetricList); | ||
|
|
||
| // Add sets of dimesion filters to use for each query to CloudWatch |
There was a problem hiding this comment.
We've now added a second edge case to this logic, i.e. the [RemoteService] and [RemoteService, RemoteTarget] aggregations. It would be worth adding a larger comment here explaining what's happening and explain why in the PR description for future reference.
There was a problem hiding this comment.
Please be more descriptive in other comments in your PR as well.
| for (String remoteServiceName : remoteServiceNames) { | ||
| dimensionLists.add( | ||
| Arrays.asList( | ||
| new Pair<>(CloudWatchService.REMOTE_SERVICE_DIMENSION, remoteServiceName))); | ||
| } |
There was a problem hiding this comment.
Is this new logic able to pull [RemoteService, RemoteTarget] without pulling [<other stuff>, RemoteService, RemoteTarget]? If yes, we can add back the other remote service values to remoteServiceNames. If not, the code changes in this PR will cause flaky tests in the long run.
This is due to the fact that when trying to pull [RemoteService, RemoteTarget] if you also pull other aggregations that have the same two attributes you will pull metrics from all test runs in the past three hours that match the filter you set. CW listMetrics API cannot return all of these metrics reliably and we've seen this cause failures before. Please ensure your solution accounts for this by running a few tests in a row and seeing if metrics with attributes containing the wrong test ID appear in the actualMetricList.
| dimensionLists.add( | ||
| Arrays.asList( | ||
| new Pair<>(CloudWatchService.REMOTE_SERVICE_DIMENSION, "AWS.SDK.S3"), | ||
| new Pair<>(CloudWatchService.REMOTE_TARGET_DIMENSION, "e2e-test-bucket-name"))); |
There was a problem hiding this comment.
Once the metric exists, how do we know whether the test that just occurred is the one that populated it or if it simply exists from a previous test run? It's entirely possible that a test from 2 hours ago created the metric and then all tests since then did not appropriately populate this metric but because of our .withRecentlyActive("PT3H") we are not finding out at all that this metric is flakily populated.
This is the same issue we had with trying to check [RemoteService] when it is equal to ["www.amazon.com"] for example. (There are also other considerations for this aggregation which I'll mention in another comment)
Note: withRecentlyActive() only accepts PT3H
|
In discussion, a solution for both issues ( |
|
This PR is stale because it has been open 60 days with no activity. |
|
Closing in favor of: aws-observability/aws-application-signals-test-framework#41 |
Issue #, if available:
aws/amazon-cloudwatch-agent#1010
Recently the following metric rollups were added to 2 AppSignal Configuration files. Tests need to be added for them
[RemoteService, RemoteTarget][HostedIn.<Attributes>, Service, RemoteService, RemoteTarget]Description of changes:
Add tests for new metric rollups
Testing done:
EKS in IAD: https://github.com/jj22ee/amazon-cloudwatch-agent/actions/runs/7659009903/job/20873170967
EC2 in IAD: https://github.com/jj22ee/amazon-cloudwatch-agent/actions/runs/7659098851/job/20873465899
./gradlew testing:validator:testBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.