Add OTel performance and regression tests - #739
Conversation
musa-asad
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The gap is real: nothing in the existing suite watches the pure OTel pipeline's own resource usage. I read through the full diff and checked the queries and helpers against the repo's existing patterns. Comments are inline. The recurring themes are missing cluster scoping on the queries and a suite that is not wired into the generator, so it never runs in CI. Add to that a region mismatch between the metrics client and the DynamoDB client, and fail-open paths that turn infrastructure errors into green runs.
Two smaller notes that did not fit a line:
- The Tests section says compilation was verified with
go build -tags integration ./test/otel/performance/..., butgo builddoes not compile_test.gofiles, so that command type-checks none of the added code.go test -run=NO_MATCH -tags integration ./test/otel/performance/...does. - A formatting pass would help: about ten lines carry trailing whitespace,
calcStatsuses named returns which the repo'snonamedreturnslinter forbids, and there are a couple of comment typos (resuts,accross). Theintegrationbuild tag hides these files from the linters, so none of this gets flagged automatically.
| start := end.Add(-queryRangeMinutes * time.Minute) | ||
| step := 30 * time.Second | ||
|
|
||
| cpuQuery := fmt.Sprintf(`{"__name__"="k8s.pod.cpu.utilization", %s, %s}`, agentPodFilter, agentNSFilter) |
There was a problem hiding this comment.
Both range selectors filter on pod name and namespace but not on cluster name. The monitoring endpoint is account and region wide, so cloudwatch-agent pods from any other cluster in the same account and region land in these results and can swing both the threshold and regression verdicts.
The other otel suites scope their queries with "@resource.k8s.cluster.name", and TestMain already resolves clusterName into cfg, so the value is available here. Could we add the predicate to both queries?
There was a problem hiding this comment.
Done — added "@resource.k8s.cluster.name" to both the CPU and memory range queries in fetchSharedMetrics, using cfg.ClusterName.
| } | ||
| ], | ||
| "node_allocatable_queries": { | ||
| "cpu": "kube_node_status_allocatable{resource=\"cpu\"}", |
There was a problem hiding this comment.
These two queries match every node in the account and region, and getNodeAllocatable averages all returned series. A second cluster with a different node shape would skew the percent-of-node denominator, and the calibrated thresholds stop meaning what the table in the description says. Suggest adding the cluster predicate here too, and consider asserting the node instance type, since the absolute percentages were calibrated on t3.medium.
There was a problem hiding this comment.
Done — the node-allocatable queries are now cluster-scoped.
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package performance |
There was a problem hiding this comment.
I could not find a path that runs this suite: the generator's eks_daemon list ends at ./test/otel/neuron, and no terraform root sets test_dir to ./test/otel/performance. The integration build tag also hides these files from make compile and the linters.
As delivered, the new tests never execute or type-check in CI. Could we register the suite in generator/test_case_generator.go so the regression protection actually runs?
There was a problem hiding this comment.
Done — registered the suite in generator/test_case_generator.go (new ./test/otel/performance entry in the eks_daemon list) and added the matching terraform/eks/daemon/otel-performance root.
| commitHash = cfg.ClusterName | ||
| } | ||
| current := collectCurrentResults(t) | ||
| storeResult(t, commitHash, current) |
There was a problem hiding this comment.
storeResult runs before the comparison, so a run that fails the 30% check still persists its regressed values, and the next run compares against the regressed baseline and passes. One red run permanently resets the baseline.
Storing after the comparison, or marking failed rows so fetchPreviousResult can skip them, would keep the baseline trustworthy. A side effect of the current shape: UniqueID embeds the run timestamp, so every run appends a new row and rows per commit grow without bound.
There was a problem hiding this comment.
Done — storeResult now runs only after a passing comparison, so a regressed run reports the failure but doesn't overwrite the baseline. Also made UniqueID deterministic (useCase-commitHash-instanceType, no timestamp) so re-runs overwrite one row per commit+instance instead of appending, matching the EC2 performance validator.
| env := environment.GetEnvironmentMetaData() | ||
| commitHash := env.CwaCommitSha | ||
| if commitHash == "" { | ||
| commitHash = cfg.ClusterName |
There was a problem hiding this comment.
When -cwaCommitSha is empty this falls back to the cluster name. I checked the terraform roots: five EC2 roots pass -cwaCommitSha and no EKS root does, so on EKS the stored CommitHash is always the cluster name.
The fetch filter #ch <> :ch then excludes every prior row from the same cluster, and the comparison never runs: each run logs first-run and exits green. Requiring the commit sha, or resolving it from the deployed image, seems safer than the fallback.
There was a problem hiding this comment.
Done — removed the cluster-name fallback; cwaCommitSha is now required (require.NotEmpty).
| result.DaemonSetCPUMax = max | ||
| } | ||
| } else { | ||
| result.ScraperCPUMax = max |
There was a problem hiding this comment.
The DaemonSet branches guard the max accumulation, but both scraper branches assign unconditionally (this line for CPU, line 128 for memory), so when the window contains more than one scraper series the last one wins rather than the largest. Multiple series are reachable: the runner restarts the agent right before the tests, so pre- and post-restart pods can both fall inside the 5 minute window. Mirroring the guard fixes it:
if max > result.ScraperCPUMax {
result.ScraperCPUMax = max
}There was a problem hiding this comment.
Done — mirrored the guard on both scraper branches (CPU and memory) in collectCurrentResults, so the largest value in the window wins instead of the last, matching the DaemonSet branches.
| // Returns latest previous result, the commit hash of that result, if one was found. | ||
| func fetchPreviousResult(t *testing.T, currentCommitHash string) (PerfResult, string, bool) { | ||
| t.Helper() | ||
| data, err := awsservice.DynamodbClient.Query(context.Background(), &dynamodb.QueryInput{ |
There was a problem hiding this comment.
awsservice.DynamodbClient is built in a package init() from AWS_REGION with a us-west-2 default, and nothing rebuilds it from the -region flag that TestMain honours. A run in another region (the description's validation used eu-west-1) reads metrics from the flag region but queries and writes the table in us-west-2. Calling awsservice.ConfigureAWSClients(region) from TestMain after resolving the region would line the two up.
There was a problem hiding this comment.
Done — TestMain now calls awsservice.ConfigureAWSClients(region) after resolving the region (guarded for the us-west-2 default), so the DynamoDB client and metrics client use the same region. Mirrors the pattern in test/e2e/envutils.go.
| }, | ||
| ScanIndexForward: aws.Bool(false), | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
A query failure here becomes hasPrevious=false, which the caller logs as a first run before returning green. A missing table, a permissions error, or throttling silently turns the regression test into a permanent pass. Together with the us-west-2 default on the client, a table that only exists in the metrics region makes every run a first-run pass.
Failing the test on a query error keeps this fail-closed. getFloat below has the same fail-open shape: missing or unexpectedly typed fields decode to zero, and compareAndReport skips zero baselines, so a corrupt row also passes silently.
There was a problem hiding this comment.
Done — fetchPreviousResult now fails closed: require.NoError on the query (and unmarshal/Results) instead of treating an error as a first run; a genuine empty result still counts as first run. Also compareAndReport now fails on a zero baseline instead of skipping it, so a corrupt/missing row can't pass silently.
| t.Log("") | ||
| } | ||
|
|
||
| for _, series := range results { |
There was a problem hiding this comment.
This loop checks only the pod classes that show up in the results, and the continue below skips unknown names, so nothing asserts that both the DaemonSet and scraper classes were observed. If one class stops emitting a metric, the remaining series can pass and the test reports success while covering half its declared thresholds.
collectCurrentResults in regression_test.go has the same gap: a missing class stays at zero, and a positive baseline reads that as reduced usage. Could we assert both classes are present per metric?
There was a problem hiding this comment.
Done — both tests now assert both pod classes are present. TestPerformanceThresholds tracks expected classes from the config and fails if one isn't observed per metric; collectCurrentResults requires both sawDaemonSet and sawScraper before returning.
|
Some things to note .... The threshold test expresses agent usage as a percentage of node allocatable, so the pass/fail bands are only meaningful for the instance type they were calibrated on (t3.medium). if the agents footprint is absolute, rather than proportional to the host size, this would raise two concerns:
I've left the percent-of-node set up as is, with the assumption that the cluster, instance type etc are held constant. let me know if you have any thoughts or suggestions on this. |
musa-asad
left a comment
There was a problem hiding this comment.
Thanks for the thorough revision. The earlier feedback is addressed well: cluster predicates on every query, the suite wired into the generator with a real terraform root, store-after-compare with stable keys, guarded scraper maxima, fatal DynamoDB errors, and both-class assertions.
I went through the new commit end to end and found five issues in the new code, mostly around the terraform root, that I would like resolved before merge since they affect what the test actually measures. Details inline.
| kubectl -n amazon-cloudwatch patch AmazonCloudWatchAgent cloudwatch-agent --type='json' \ | ||
| -p='[{"op": "replace", "path": "/spec/image", "value": "${var.cwagent_image_repo}:${var.cwagent_image_tag}"}]' | ||
| kubectl -n amazon-cloudwatch patch AmazonCloudWatchAgent cloudwatch-agent-cluster-scraper --type='json' \ | ||
| -p='[{"op": "replace", "path": "/spec/image", "value": "${var.cwagent_image_repo}:${var.cwagent_image_tag}"}]' 2>/dev/null || true |
There was a problem hiding this comment.
The scraper patch swallows failures (2>/dev/null || true), and lines 187 and 189 do the same for its rollout restart and status. If the patch fails, the scraper keeps running the chart's stock image while the tests measure it and store its numbers as the baseline, so a regression in the candidate image passes unmeasured. The DaemonSet lines fail loudly, and I'd make the scraper lines match.
If the suppression is there because the scraper resource may not exist on some clusters, an explicit existence check would keep that case without hiding real patch failures.
There was a problem hiding this comment.
Removed the error suppression from all three scraper lines (patch, rollout restart, rollout status) so they fail loudly like the DaemonSet ones.
| cd ../../../.. | ||
|
|
||
| echo "Waiting 3 minutes for metrics to propagate..." | ||
| sleep 180 |
There was a problem hiding this comment.
This waits 3 minutes, but the tests query the previous 5 (queryRangeMinutes = 5 in setup_test.go), so roughly the first 2 minutes of every measured window predate the wait. Rollout transients and possibly old-image samples land in the averages and maxima that the thresholds and baselines are computed from. Waiting longer than the query window, or having the test discard samples older than the rollout, would make the window describe the candidate image in steady state.
There was a problem hiding this comment.
Bumped the wait to 6.5 minutes (sleep 390), ~1.5 min settle after rollout plus the 5-min query window, so the whole window is in a post-rollout steady state and rollout/old-image samples don't land in the averages.
| require.NotEmpty(t, metrics.CPUResults, "no CPU data") | ||
| require.NotEmpty(t, metrics.MemResults, "no memory data") | ||
| var result PerfResult | ||
| var sawDaemonSet, sawScraper bool |
There was a problem hiding this comment.
sawDaemonSet and sawScraper are shared across the CPU and memory loops and checked once at the end, so a class present in only one metric still passes both checks. Example: DaemonSet series only in CPU and scraper series only in memory sets both flags, and the run stores a baseline with zero DaemonSet memory and zero scraper CPU. Tracking presence per metric (four flags, or a small per-metric map) closes it.
There was a problem hiding this comment.
Split into four per-metric flags (sawDaemonSetCPU, sawScraperCPU, sawDaemonSetMem, sawScraperMem) and assert all four, so a class present in only one metric no longer passes both checks.
| } | ||
| totalSum := 0.0 | ||
| for _, value := range values { | ||
| if value < 0 && threshold >= 0 { |
There was a problem hiding this comment.
NaN slips through both checks here: NaN < 0 is false, so it passes the negative guard, and once summed the average is NaN, for which avg > upperBound || avg < lowerBound is also false, so the threshold check reports PASS with a NaN average. Prometheus range results can carry NaN samples. An explicit math.IsNaN(value) rejection in this loop keeps an invalid series from reading as a pass.
There was a problem hiding this comment.
Added an explicit math.IsNaN rejection in the loop so a NaN sample fails the series instead of producing a NaN average that reads as PASS. Added the same guard on the regression side (collectCurrentResults), since calcStats had the same exposure.
| // us-west-2. Reconfigure them for the resolved region so the baseline is | ||
| // read/written in the same region the metrics client queries. Mirrors the | ||
| // pattern in test/e2e/envutils.go. | ||
| if region != "us-west-2" { |
There was a problem hiding this comment.
This guard skips the reconfigure when the resolved region is us-west-2, but the init() in util/awsservice built the clients from AWS_REGION, which may be something else. In that case metrics come from us-west-2 while DynamoDB reads and writes go to the AWS_REGION value. Calling awsservice.ConfigureAWSClients(region) unconditionally makes the invariant hold in both directions, and the call is cheap either way.
There was a problem hiding this comment.
Removed the if region != "us-west-2" guard and call awsservice.ConfigureAWSClients(region) unconditionally, so DynamoDB and the metrics client always share the resolved region regardless of what AWS_REGION was set to.
…ing, and settle timing in otel performance suite
musa-asad
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. All five earlier comments are fixed cleanly: the error suppression is gone, the wait now covers the query window, presence is tracked per metric, NaN is rejected, and the client reconfigure is unconditional.
Going through the updated code I found four remaining issues. The first two silently disable the regression comparison itself, so I'd like them resolved before approving. The other two are small.
| -computeType=EKS \ | ||
| -eksDeploymentStrategy=DAEMON \ | ||
| -region=${var.region} \ | ||
| -cwaCommitSha=${var.cwagent_image_tag} |
There was a problem hiding this comment.
With the default cwagent_image_tag = "latest", every run passes commitHash="latest", and the fetch filter #ch <> :ch then excludes every prior row. Each run logs first-run, stores a fresh baseline, and the comparison never executes, so under defaults the suite never actually checks for regressions.
Every other test module in the repo passes -cwaCommitSha=${var.cwa_github_sha} (terraform/ec2/creds, efa, linux, mac, userdata). Declaring cwa_github_sha here and passing it instead would give rows a real commit identity and keep the image tag concern separate from the baseline key.
There was a problem hiding this comment.
I checked how this is wired in the agent repo (test-artifacts.yml), and there's a clean split by compute type:
EKS integration tests — the EKSIntegrationTest job runs a single shared terraform apply step (a matrix, one apply per suite) for every eks_daemon suite and passes -var="cwagent_image_tag=${{ inputs.build_id }}" (the per-build identifier — a commit SHA or RC build number), but not cwa_github_sha. So the image tag is where the commit identity comes from on EKS, and it's what all the otel roots consume — otel (standard), otel-attr-limit, otel-ebs-csi, otel-efa, otel-gpu, otel-lis-csi, otel-multi-efa, otel-neuron, and this suite. That's why I reused cwagent_image_tag here…….it matches the other EKS/otel suites.
EC2 tests, on the other hand, pass -var="cwa_github_sha=${{ inputs.build_id }}" and not the image tag, which is why the EC2 roots (ec2/linux, ec2/creds, ec2/efa, ec2/mac, ec2/userdata) use cwa_github_sha.
Since the EKS job passes a single fixed -var list to every otel root, switching this suite to cwa_github_sha would mean either editing the shared workflow (and conditionally passing it, like they already do for helm_chart_branch) plus declaring the variable, or declaring it across all the otel roots.
To keep this PR self-contained and consistent with the EKS/otel convention, I instead removed the latest default on cwagent_image_tag and added a validation block requiring it to be non-empty. The tag reaches the test through the existing flag — main.tf passes -cwaCommitSha=${var.cwagent_image_tag}, and regression_test.go keeps require.NotEmpty on it as a second guard. So a run that doesn't set the tag now fails at plan time with a clear message, instead of silently keying every run on "latest".
| for _, v := range series.Values { | ||
| require.False(t, math.IsNaN(v), "CPU series for %s contains a NaN sample", podName) | ||
| } | ||
| _, max := calcStats(series.Values) |
There was a problem hiding this comment.
A labeled series with an empty Values slice still sets its presence flag here, and calcStats returns 0 for it, so a first run can store a zero baseline. The next run then hits the fail-loud corrupt-baseline path and every run after that fails until the row is deleted from the table by hand.
Skipping series with empty Values (or requiring non-empty values before setting the flag) closes it.
There was a problem hiding this comment.
Done. Both loops in collectCurrentResults now skip series with an empty Values slice, so an empty series doesn't store a 0 baseline.
| TableName: aws.String(tableName), | ||
| IndexName: aws.String("UseCaseDate"), | ||
| KeyConditionExpression: aws.String("#uc = :uc"), | ||
| FilterExpression: aws.String("#ch <> :ch AND #it = :it"), |
There was a problem hiding this comment.
DynamoDB applies FilterExpression after each 1 MB page is read, and this query never follows LastEvaluatedKey. When the first page contains only rows this filter drops, Items comes back empty even though an older matching baseline exists, and the run reports first-run and stores instead of comparing.
Looping on LastEvaluatedKey until a row survives the filter (or restructuring the key so the filter is part of the key condition) makes the fetch reliable.
There was a problem hiding this comment.
Done. fetchPreviousResult now loops on LastEvaluatedKey, paging until a row survives the #ch <> :ch AND #it = :it filter or there are no more pages, so a filtered-out first page no longer reports a false first-run.
| exec { | ||
| api_version = "client.authentication.k8s.io/v1beta1" | ||
| command = "aws" | ||
| args = ["eks", "get-token", "--cluster-name", aws_eks_cluster.this.name] |
There was a problem hiding this comment.
This aws eks get-token call omits --region (same at line 25), so it resolves against the AWS CLI's ambient default region rather than var.region. On a machine whose CLI default differs from the selected region, cluster auth fails or targets the wrong region. Adding "--region", var.region to both arg lists pins it.
There was a problem hiding this comment.
Done. Added --region var.region to both get-token arg lists (kubernetes and helm providers), so auth targets var.region instead of the CLI's ambient default.
musa-asad
left a comment
There was a problem hiding this comment.
Thanks for working through three rounds of this. All four comments are addressed at this revision: the image tag is now mandatory with a validation naming it as the baseline key, empty series no longer set presence flags or store zero baselines, the fetch follows LastEvaluatedKey so an existing baseline cannot read as a first run, and both get-token calls pin --region.
Two non-blocking notes for whenever you are next in here. Pinning the helm chart to a fixed ref instead of the movable branch would keep runs comparable across chart changes. The generator entry could also set instanceType like its peer entries, so the generated row does not lean on the default.
Approving.
Description of the issue
There is currently no performance test for the pure OTel pipeline in the CloudWatch Agent test suite.
The existing performance tests fetch metrics using the CloudWatch
GetMetricData/GetMetricStatisticsAPI. They do not make use of the OTel-compatible metrics endpoint (monitoring.{region}.amazonaws.com) and do not measure OTel-defined metrics likek8s.pod.cpu.utilization. So the entire path from how the agent reports its own resource usage to how the test retrieves and uses that data is entirely within the native CloudWatch ecosystem — OTel is currently not involved at any layer.This means that changes made to OTel components (receivers, processors and exporters) that causes an increase in memory usage would not be caught by the existing performance tests. A code change that causes the pure OTel pipeline to double in memory usage would pass all current tests undetected.
Based on this, there is a need for a dedicated performance regression test that operates entirely within the OTel ecosystem — querying OTel-defined metrics via the OTel-compatible PromQL endpoint — to ensure that new code changes to the pure OTel pipeline do not introduce CPU or memory regressions, and to also look at how much memory is being consumed when the OTel pipeline is being used and ensure the memory usage is within a safe limit.
Description of changes
Adds two integration tests for the
otel-containerinsightsuse case undertest/otel/performance/:performance_test.go— Queries CPU and memory usage of agent pods, computes the average as a percentage of node allocatable resources, and asserts the values fall within ±15% of calibrated thresholds (defined inperformance_thresholds.json).regression_test.go— Queries max CPU and memory usage, stores results in a DynamoDB table (CWAPerformanceMetrics), fetches the most recent result from a different commit, and fails if any metric grew by more than 30%.setup_test.go— SharedTestMainsetup (cluster config, OTel metrics client) and afetchSharedMetricshelper that queries once and caches results for both tests.performance_thresholds.json— Threshold definitions calibrated from 20 observed runs ont3.mediumnodes (1930m CPU, 3371436Ki memory allocatable).The test tracks two types of CloudWatch agent pods on the cluster:
Thresholds were set to match observed averages so the ±15% error bound catches real anomalies without flapping on normal variance:
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
go test -run=NO_MATCH -tags integration ./test/otel/performance/...to verify compilation.terraform apply -var="region=eu-west-1"underterraform/eks/daemon/oteland deployed the agent.cwagent-eks-integcluster (t3.mediumnodes, eu-west-1) multiple times with different commit hashes to populate DynamoDB and validate both the threshold band check and the regression comparison logic.aws dynamodb scanto confirm values are stored and retrieved correctly.terraform destroy -var="region=eu-west-1"after testing.