Skip to content

Add OTel performance and regression tests - #739

Merged
musa-asad merged 5 commits into
aws:mainfrom
fareedah999:otel-perf-regression-test
Sep 3, 2026
Merged

Add OTel performance and regression tests#739
musa-asad merged 5 commits into
aws:mainfrom
fareedah999:otel-perf-regression-test

Conversation

@fareedah999

@fareedah999 fareedah999 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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/GetMetricStatistics API. They do not make use of the OTel-compatible metrics endpoint (monitoring.{region}.amazonaws.com) and do not measure OTel-defined metrics like k8s.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-containerinsights use case under test/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 in performance_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 — Shared TestMain setup (cluster config, OTel metrics client) and a fetchSharedMetrics helper that queries once and caches results for both tests.
  • performance_thresholds.json — Threshold definitions calibrated from 20 observed runs on t3.medium nodes (1930m CPU, 3371436Ki memory allocatable).

The test tracks two types of CloudWatch agent pods on the cluster:

  • DaemonSet pod
  • Cluster Scraper pod:

Thresholds were set to match observed averages so the ±15% error bound catches real anomalies without flapping on normal variance:

Metric Pod Observed Avg Threshold set Passes if within (±15%)
CPU DaemonSet 1.28% 1.3 1.1% – 1.5%
CPU Scraper 0.87% 0.9 0.77% – 1.04%
Memory DaemonSet 4.68% 4.7 4.0% – 5.4%
Memory Scraper 4.91% 4.9 4.2% – 5.6%

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

  • Ran go test -run=NO_MATCH -tags integration ./test/otel/performance/... to verify compilation.
  • Spun up an EKS cluster using terraform apply -var="region=eu-west-1" under terraform/eks/daemon/otel and deployed the agent.
  • Executed the full test suite against a live cwagent-eks-integ cluster (t3.medium nodes, eu-west-1) multiple times with different commit hashes to populate DynamoDB and validate both the threshold band check and the regression comparison logic.
  • Verified DynamoDB records via aws dynamodb scan to confirm values are stored and retrieved correctly.
  • Calibrated thresholds from 20 runs across two clusters over multiple days to observe how the scraper and DaemonSet pods behave naturally.
  • Destroyed the cluster via terraform destroy -var="region=eu-west-1" after testing.

@fareedah999
fareedah999 requested a review from a team as a code owner August 7, 2026 15:53

@musa-asad musa-asad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/..., but go build does not compile _test.go files, 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, calcStats uses named returns which the repo's nonamedreturns linter forbids, and there are a couple of comment typos (resuts, accross). The integration build tag hides these files from the linters, so none of this gets flagged automatically.

Comment thread test/otel/performance/setup_test.go Outdated
start := end.Add(-queryRangeMinutes * time.Minute)
step := 30 * time.Second

cpuQuery := fmt.Sprintf(`{"__name__"="k8s.pod.cpu.utilization", %s, %s}`, agentPodFilter, agentNSFilter)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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\"}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — removed the cluster-name fallback; cwaCommitSha is now required (require.NotEmpty).

result.DaemonSetCPUMax = max
}
} else {
result.ScraperCPUMax = max

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@fareedah999

Copy link
Copy Markdown
Contributor Author

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:

  • Instance-type sensitivity. If the node changes, the allocatable denominator changes, so the reported percentage shifts even though the agent's actual usage didn't. For example, if the band is ~1–1.5% of memory, on a t3.medium that ~1% might be ~100MB and pass. On a much larger host, 1% could be ~1GB — but the agent is still using ~100MB, so it now reports ~0.1% and fails as "way below range." The verdict flips purely due to host size, not agent behavior.

  • A big decrease currently looks "good" to the regression check and passes. Note the threshold test would still catch an abnormally low value (it's below the band). Not sure if there is a reliable way for the test alone to tell "something broke and numbers are down" from "we optimized, so numbers should go down" since both look like "number went down."

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 musa-asad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread test/otel/performance/setup_test.go Outdated
// 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" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 musa-asad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 musa-asad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@musa-asad
musa-asad merged commit 618e3d5 into aws:main Sep 3, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants