Add OTel performance and regression tests - #739
Conversation
musa-asad
left a comment
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. |
2 participants
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.