Repository navigation
fix(aws-cloudwatch): DashboardNotFoundError shape, FILL/IF/SEARCH metric math, non-goals - #1537
Satyam-Trivedi-ZS wants to merge 8 commits into
Conversation
remove claude attribution please
|
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks for this. The dashboard error fix is right, and the IF and comparison/logical semantics match the user guide tables exactly. I checked everything on the pushed head (75b7f24) against the AWS CLI (query/XML), raw awsJson1_0, and aws-sdk-go-v2 (rpc-v2-cbor), with Terraform at the end. There's one Medium in the new FILL code. Everything else is Low, and most of the Lows were already there before this PR.
Issue coverage (#1520)
| Item | Status | Evidence |
|---|---|---|
| GetDashboard / DeleteDashboards not-found error | Fixed | CBOR via aws-sdk-go-v2: errors.As(err, *types.DashboardNotFoundError) is true, ErrorCode() is ResourceNotFound, status 404, on both ops. JSON: __type/X-Amzn-Errortype is DashboardNotFoundError, X-Amzn-Query-Error: ResourceNotFound;Sender, 404. CLI (query): ResourceNotFound. TestSDKDashboardNotFoundShape and TestJSONProtocolErrors fail on the merge base and pass here. Your reading is correct: the botocore model gives the shape code ResourceNotFound, status 404. |
| Metric math FILL / IF / SEARCH | Fixed, with one gap (M1) | Live GetMetricData over all three protocols. The comparison table, the AND table and all three IF tables from the guide reproduce exactly. FILL with S, TS, REPEAT and LINEAR fills the range, in ascending and descending order. SEARCH matches the schema, designators, partial/composite/exact tokens and NOT, and returns one row per match with the query Id. TestSDKGetMetricDataFILLAndIF and TestSDKGetMetricDataSEARCH fail on base and pass here. |
| ANOMALY_DETECTION_BAND | Already fixed | Band rows already came back before this PR. Nothing changed here apart from asSeries() plumbing. |
| AnomalyDetectorId / AnomalyDetectorIds | Not done, documented | The pinned SDK and botocore don't model these fields. Writing it up as a non-goal is reasonable. |
| GetMetricWidgetImage | Skipped, documented non-goal | Reasonable. The call still gets UnknownOperation. |
Findings
Medium
M1. FILL does nothing when the expression has no period it can resolve, which is the common FILL(SEARCH(...)) case. services/monitoring/metricmath/metricmath.go:175-178, functions.go:150, search.go:68-71
The FILL grid comes from Period(e.queries, q). That function only looks at the query's own Period and at MetricStat entries. It never sees the period argument of a SEARCH. When neither gives a value, gridPeriod is 0, grid() returns nil, and FILL hands its input back unchanged. The SEARCH itself still reads at 60s, because the provider defaults period 0 to 60. So the points come back on a 60s (or 300s) step but nothing gets filled. Reproduced live against serve, with no Period on the MetricDataQuery:
FILL(SEARCH('{Rev/App,InstanceId}', 'Average', 300), 0)returns 1 point per series. WithPeriod: 300on the query it returns all 8 filled points.FILL(SEARCH('{Rev/App,InstanceId}', 'Average'), 0)also returns 1 point per series.
A user who graphs or queries FILL(SEARCH('{AWS/EC2,InstanceId} CPUUtilization','Average',300), 0) gets the sparse series back with no error. Fix: let a SEARCH node report its period (the explicit argument, else the 60s default it actually reads at), and use that in the grid when Period() returns 0. Then the fill step always equals the step of the series being filled. A wire test for FILL(SEARCH(..., 300), 0) with no query Period would cover it.
Low
L1. Arithmetic between two series still drops timestamps that only one series has, and x/0 returns 0. services/monitoring/metricmath/eval.go:95, applyOp
The page this PR cites says, under Basic arithmetic functions: "Missing values in a time series are treated as 0. If the value of a data point causes a function to attempt to divide by zero, the data point is dropped." (Use metric math). Live, with the guide's own series m1=[30,20,0,0] and m2=[20,-,20,-], m1 - m2 returns only 2 points, [10, -20]. The doc rule gives [10, 20, -20, 0]. This behaviour predates the PR, but the PR rewrote this exact code into pointwise(..., union), so arithmetic could switch to union=true here (and drop divide-by-zero points) at little cost. If you'd rather keep it out of scope, the non-goals note should say so.
L2. PutMetricAlarm accepts a SEARCH expression. server/aws/cloudwatch/core_alarms.go (expressionRefsKnown, not in this diff)
MetricDataQuery says "Any expression used in a PutMetricAlarm operation must return a single time series", and the guide says "You can't create an alarm based on a SEARCH expression" (MetricDataQuery). Live, PutMetricAlarm with Expression: SEARCH('{Rev/App,InstanceId}','Average',60) succeeds, and because the alarm evaluator has no lister the alarm just sits in INSUFFICIENT_DATA. The parser now recognises SEARCH, so the alarm validation could reject it with a ValidationError.
L3. Typed SDK errors other than the dashboard one still don't map over rpc-v2-cbor. server/aws/cloudwatch/handler.go:252-258 (writeCBORError)
This is the root cause the PR found, fixed for one error. The CBOR path writes the query code (InvalidParameterValue, MissingParameter, ...) straight into __type with no X-Amzn-Query-Error, while the SDK switches on InvalidParameterValueException, MissingRequiredParameterException and so on. errors.As(err, *types.InvalidParameterValueException) is therefore always false over CBOR. JSON already maps these through jsonErrorShapes. Pointing the CBOR writer at the same table plus the header would fix every error at once. This is fine as a follow-up.
L4. DeleteDashboards is all-or-nothing. providers/aws/cloudwatch/dashboards.go:95-106 (not in this diff)
The DeleteDashboards reference says "If there is an error during this call, the operation attempts to delete as many dashboards as possible." Here, one missing name means none of the others get deleted. This predates the PR. The current reference page also no longer lists a not-found error for DeleteDashboards, although the pinned SDK model still does, so it's worth a recheck when the SDK is bumped.
L5. SEARCH limits aren't enforced. services/monitoring/metricmath/search.go
The guide gives a 1024-character maximum query size and 500 time series per graph (search expression syntax). Neither limit is checked, so a broad SEARCH returns every match.
L6. Decorative separator comment. services/monitoring/metricmath/search.go:316 // ---- search string parser ----. A plain comment, or moving the parser to its own file, would match the rest of the repo.
AWS docs check
- FILL: S, TS, REPEAT and LINEAR all follow the function table. What REPEAT and LINEAR do at the edges isn't in the docs, and the non-goals note records the choice made here. OK.
- IF: all three tables match, including the case where
IF(m1, scalar2, metric3)drops a false point when metric3 has no value. OK. - Comparison/logical: missing-as-0 union matches the table. OK. Arithmetic does not; see L1.
- SEARCH: schema exact dimension set, tokenization examples (
CustomCount1,SDBFailure,couNT,count1,network/errors), exact match with"...", designators, NOT, and the two-week scope all match the syntax page. The labels and the limits aren't covered (L5). - Dashboard errors: the model's
DashboardNotFoundErrorhas codeResourceNotFound, status 404, and the PR emits that on all three protocols.
Terraform (hashicorp/aws 6.66.0, endpoints { cloudwatch = ... } against serve)
aws_cloudwatch_dashboard (body with a FILL(SEARCH(...)) widget) and aws_cloudwatch_metric_alarm with metric_query (IF(m1 > 5, FILL(m1, REPEAT), 0) plus an m1 MetricStat):
apply completed with no waiter hang, then plan -detailed-exitcode returned 0. I then updated the threshold and the dashboard body, and plan returned 0 again. Destroy completed, after which get-dashboard returns ResourceNotFound 404 and describe-alarms returns nothing.
A separate math alarm FILL(m1, 100) > 50 over a gap went to ALARM, so FILL also works inside alarms.
CLI / protocol e2e
aws cloudwatch get-dashboard/delete-dashboardson a missing name:ResourceNotFound, exit 254.get-metric-data(query) with SEARCH (Label prefix applied per row), FILL REPEAT and IF: correct. The same SEARCH over raw awsJson1_0 and over the Go SDK (CBOR) gives the same rows.
What I verified
- Build, vet and gofmt pass on the touched packages.
go mod tidyleaves no diff, andgo generateleaves no diff. go test -racepasses onservices/monitoring/metricmath,server/aws/cloudwatchandproviders/aws/cloudwatch. Coverage is 92.3%, 87.7% and 93.7% (base: 93.9% and 87.5% for the first two). The wire package was already under 90% before this PR.- A throwaway
-raceprobe passed: 8 goroutines running PutMetricData,FILL(SEARCH(...), REPEAT)GetMetricData, and Put/Delete/GetDashboard at the same time. - golangci-lint (v2.14.0) on the touched packages: the PR removes one
dupland adds twopreallochints insearch.go. CI Lint passes. - The new wire tests fail on the merge base and pass on the head.
- Layering: the evaluator change sits in
services/monitoring/metricmath(AWS-only consumers) and the wire change only translates errors. No driver interface changed, so the mirror rule and persistence don't apply. No new operations, so the auth declarations are unchanged. - CI is green.
Verdict
Changes requested for M1: FILL should use the same period the SEARCH reads at. Once that's fixed with a test, this is good to merge. L1 to L6 can be follow-ups.
| // entries keep their own periods, as far apart as the points it reads. | ||
| grid := period | ||
| if grid == 0 { | ||
| grid = Period(e.queries, q) |
There was a problem hiding this comment.
When neither the query nor any MetricStat has a period, Period() returns 0 here. It never sees a SEARCH's own period argument, so the FILL grid ends up nil and FILL does nothing. Live: FILL(SEARCH('{Rev/App,InstanceId}', 'Average', 300), 0) with no Period on the query returns 1 point per series. Add Period: 300 and it returns all 8 filled points. The SEARCH still reads at 60s (the provider's default for period 0), so the series has a step but nothing fills it. Could the SEARCH node report its period (explicit arg, else the 60s it actually reads at) and the grid use that?
There was a problem hiding this comment.
Fixed in 879091c. A SEARCH now reports the period it reads at (its period argument, else the entry Period, else the 60s a period-less read uses), and FILL builds its grid from that step. With no Period on the query, FILL(SEARCH(..., 300), 0) now returns every 300s period, and FILL(SEARCH(...), 0) returns every 60s period. Covered by TestSDKGetMetricDataFILLOverSEARCHNoPeriod (wire) and TestFILLOverSEARCHUsesSearchPeriod (unit). Both fail on the previous head.
|
|
||
| period := n.period | ||
| if period == 0 { | ||
| period = s.gridPeriod |
There was a problem hiding this comment.
When gridPeriod is 0 this passes period 0 to fetch, which the provider turns into 60. FILL, though, sees 0 and skips the grid (see the comment on metricmath.go). Resolving one effective period here and using it for both the fetch and the FILL grid would keep the two consistent.
There was a problem hiding this comment.
Done in 879091c. The SEARCH resolves one effective period (effectivePeriod), uses it for the fetch, and returns it with the result. FILL uses the same value, so the read step and the fill step always match.
| case union: | ||
| return result{series: unionSeries(f, left.series, right.series)} | ||
| default: | ||
| return result{series: intersectSeries(f, left.series, right.series)} |
There was a problem hiding this comment.
Arithmetic still intersects. The page you cite says for arithmetic: "Missing values in a time series are treated as 0. If the value of a data point causes a function to attempt to divide by zero, the data point is dropped." With the guide's m1=[30,20,0,0], m2=[20,-,20,-], m1 - m2 returns [10, -20] here; the doc rule gives [10, 20, -20, 0]. applyOp also returns 0 for x/0 where the doc drops the point. This predates the PR, but with pointwise(..., union) in place it looks like a small change. If it's out of scope, the non-goals note could mention it.
There was a problem hiding this comment.
Fixed in ad761b7, following the arithmetic rule on that page. Arithmetic between two series now runs over the union of their timestamps with a missing point read as 0, and a point that would divide by zero is dropped (this also applies to TS/S and S/S). With the guide series m1=[30,20,0,0] and m2=[20,-,20,-], m1 - m2 is now [10,20,-20,0] and m1 / m2 is [1.5, 0]. Covered by TestSDKGetMetricDataArithmeticMissingAsZero (wire) and TestArithmeticMissingAsZero (unit). TestSeriesAlignByTimestamp and the old m1/0 case now expect the documented results.
| return | ||
| } | ||
|
|
||
| w.Header().Set(queryErrorHeader, code+";"+faultOf(status)) |
There was a problem hiding this comment.
This header and the shape mapping now only happen for the dashboard error. The generic writeCBORError above still puts the query code (InvalidParameterValue, MissingParameter, ...) in __type with no X-Amzn-Query-Error, so over rpc-v2-cbor errors.As(err, *types.InvalidParameterValueException) never matches (the SDK switches on the ...Exception shape names). Reusing jsonErrorShapes and this header in writeCBORError would fix that whole class. Fine as a follow-up.
There was a problem hiding this comment.
Agreed on the root cause. I tried pointing writeCBORError at jsonErrorShapes plus the header. It changes the raw CBOR __type that the existing parity tests assert for about a dozen unrelated operations (PutMetricData, PutMetricAlarm, metric streams, dashboards). It also needs care for ValidationError, which has no shape in the model (the table maps it to ValidationException, so the SDK would still fall back to a generic error). I left it for a dedicated follow-up PR that covers every operation, rather than widening this one.
| } | ||
| } | ||
|
|
||
| // ---- search string parser ---- |
There was a problem hiding this comment.
Nit: a plain comment would match the rest of the repo better than the // ---- separator. Moving the parser into its own file would also work.
There was a problem hiding this comment.
Fixed in 879091c: it is a plain comment now (// The search string parser.).
|
One project rule to fix before merge: please remove the AI attribution from this PR.
The repo doesn't accept AI attribution lines in commits or PR descriptions. Thanks! |
75b7f24 to
ad761b7
Compare
|
Thanks for the thorough review, @NitinKumar004. Here is where each item landed (head is now M1, FILL over SEARCH with no resolvable period: fixed in L1, arithmetic missing values and x/0: fixed in L2, PutMetricAlarm accepts SEARCH: left as a follow-up. L3, typed CBOR errors beyond the dashboard one: left as a follow-up. L4, DeleteDashboards all-or-nothing: left as a follow-up. L5, SEARCH limits: left as a follow-up. L6, separator comment: fixed in Attribution: removed. None of the branch's commit messages carries the trailer any more (rewritten and force-pushed), and the description no longer has the footer. Checks on the new head:
|
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Thanks for the follow-up commits. M1 and L1 are fixed and the separator comment is gone. While I was testing the new SEARCH period path, though, I found that an out-of-range SEARCH period can hang the server or crash the request. That needs a fix before merge. Details below.
What I checked
- The new commits 879091c and ad761b7 against each point from the 2026-10-12 review.
- The branch still merges cleanly onto the current development (46 commits ahead, and none of them touch cloudwatch or metricmath). Build,
go mod tidyandgo generateon the merge result leave no diff. Scoped tests pass on the merge result forservices/monitoring/...,server/aws/cloudwatch,providers/aws/cloudwatch,providers/oci/monitoringandpersist. - On the head: build, vet and gofmt are clean.
go test -racepasses on metricmath (92.4%), server/aws/cloudwatch (87.7%) and providers/aws/cloudwatch (93.7%). I ran golangci-lint 2.14.0 on the touched packages with no--new-from-revand compared it with a merge-base worktree. The PR removes onedupland adds twopreallochints insearch.go, which is unchanged since last time. gocyclo is the same on both sides. CI is green. - Live checks against
cloudemu servefrom the head with the AWS CLI, raw awsJson1_0 and Terraform hashicorp/aws 6.66.0 (apply, plan 0, update, plan 0, destroy). - There are no AI attribution lines in the body or the commits.
Earlier findings
| Point | Status | Evidence |
|---|---|---|
| M1 FILL(SEARCH) with no Period | Fixed | Live, with no Period on the query: FILL(SEARCH('{Rev/App,InstanceId}','Sum',300),0) returns 4 filled points per series over 20 minutes, and FILL(SEARCH(...,'Sum'),0) returns 20. New tests TestSDKGetMetricDataFILLOverSEARCHNoPeriod and TestFILLOverSEARCHUsesSearchPeriod cover it. There is a narrower leftover case; see Low L7. |
| L1 arithmetic missing-as-0 and divide by zero | Fixed | Live with the guide's series m1=[30,20,0,0] and m2=[20,-,20,-]: m1-m2 gives [10,20,-20,0], m1/m2 gives [1.5,0], m1/0 is empty and m1>m2 gives [1,1,0,0]. That matches the "Basic arithmetic functions" text on the Use metric math page. |
| L2 PutMetricAlarm accepts SEARCH | Open | put-metric-alarm with SEARCH('{Rev/App,InstanceId}','Average',60) still succeeds. |
| L3 typed errors over rpc-v2-cbor | Open, deferred | Your reasoning for a separate PR covering every operation makes sense. Please open a ticket so it doesn't get lost. |
| L4 DeleteDashboards all-or-nothing | Open | delete-dashboards a nope returns ResourceNotFound and a is still listed. This was already the case before the PR. |
| L5 SEARCH limits (1024 chars, 500 series) | Open | Unchanged. |
| L6 separator comment | Fixed | It now reads // The search string parser. |
High
H1. An out-of-range SEARCH period hangs the server goroutine or panics the request. services/monitoring/metricmath/parser.go:550-556 (searchPeriod), functions.go:200-215 (grid), search.go:64-75 (effectivePeriod)
searchPeriod accepts any positive whole number. Since 879091c that value also sets the FILL step. grid then computes time.Duration(period) * time.Second, and that overflows for any period above about 9.2e9 seconds:
FILL(SEARCH('{Rev/App,InstanceId}','Sum',9223372037),0)wraps the step to a negative value. Thee.end.Sub(e.start)/step > maxFillPointscheck passes because the quotient is negative, andfor ts := e.start; ts.Before(e.end); ts = ts.Add(step)never ends. Live againstserve, the CLI timed out, but the handler kept running at 30-130% CPU with RSS climbing to 1.7 GB. On SIGTERM the server could not shut down cleanly (context deadline exceeded). Nothing checks the request context, so one request is enough to pin a core and eventually exhaust memory.- With a period of 2^55 (36028797018963968), the step wraps to exactly 0, and
gridpanics with an integer divide by zero atfunctions.go:207. Without FILL, the same SEARCH panics in the provider atproviders/aws/cloudwatch/cloudwatch.go:278(buildMetricResult, whereperiodDuris 0). The connection drops and the CLI reports "Connection was closed". - The integer check
num.val != float64(int(num.val))also behaves differently on each platform once the value is past the int64 range. On arm64,int(2^63)saturates to MaxInt64, the round trip compares equal, and 2^63 is accepted. On amd64 it becomes MinInt64 and is rejected.
The same overflow is reachable through the entry's own Period (FILL(m1,0) with Period: 36028797018963968 also drops the connection), since GetMetricData never validates MetricDataQuery.Period or MetricStat.Period. That validation gap was there before this PR, but FILL and SEARCH are the new code that loops on the bad value.
Suggested fix:
- In
searchPeriod, bound the float before converting it, for examplenum.val > maxPeriodreturns false, where the cap is a value AWS can return such as the 455-day retention or a day multiple. Also require 1, 5, 10, 20, 30, or a multiple of 60, matching the documented MetricStat periods. Return a parse error otherwise. - Make
gridsafe whatever its caller passes. Count periods in seconds first (int64(e.end.Sub(e.start)/time.Second) / int64(period) > maxFillPoints) and rejectstep <= 0. - Ideally, also validate
Periodon GetMetricData queries at the wire and return a ValidationError, asvalidPeriodalready does for alarms.
A regression test for FILL(SEARCH(..., 9223372037), 0) and for a 2^55 period would cover this.
Medium
None.
Low
L7. FILL loses the SEARCH period once the SEARCH goes through arithmetic. services/monitoring/metricmath/eval.go:61-70 (mapArray)
mapArray returns result{isArray: true, array: out} without the input's period. As a result, FILL(SEARCH('{Rev/App,InstanceId}','Sum',300)*2, 0) with no query Period returns one point per series ([8] and [14]) instead of 4 filled points. With Period: 300 it fills correctly. Carrying period through mapArray (and pointwise for array operands) would close the last part of M1.
L8. The new wire test misses rows that never come back. server/aws/cloudwatch/metric_math_functions_test.go (TestSDKGetMetricDataArithmeticMissingAsZero)
The loop only checks the rows that are returned. If diff or ratio were dropped, the test would still pass. Asserting len(out.MetricDataResults) == 2, or checking every key of want against the results, would catch that.
Terraform and CLI
aws_cloudwatch_dashboardwith aFILL(SEARCH(...,300),0)widget andaws_cloudwatch_metric_alarmwithIF(m1 > 5, FILL(m1, REPEAT), m1 / 0 + 1)plus an m1 MetricStat: apply, then plan returns 0. After changing the threshold and title, apply and plan return 0 again. Destroy succeeds, andget-dashboardafterwards returns ResourceNotFound.get-dashboardanddelete-dashboardson a missing name return ResourceNotFound. Over awsJson1_0 the response is 404 withX-Amzn-Errortype: DashboardNotFoundErrorandX-Amzn-Query-Error: ResourceNotFound;Sender.
Verdict
Changes requested for H1. With the period bounded at parse time, grid made safe against overflow and a regression test added, this is ready. L2 to L5, L7 and L8 can be follow-ups.
| // searchPeriod reads the period argument of SEARCH: a positive whole number. | ||
| func searchPeriod(arg node) (int, bool) { | ||
| num, ok := arg.(numberNode) | ||
| if !ok || num.val <= 0 || num.val != float64(int(num.val)) { |
There was a problem hiding this comment.
Any positive whole number gets through here, and since the last commit it also becomes the FILL step. With 9223372037, time.Duration(period) * time.Second in grid wraps negative and the fill loop never ends. Live, the handler kept burning CPU and grew to 1.7 GB after the client gave up, and the server could not shut down cleanly. With 2^55 the step is 0 and the request panics with a divide by zero. Separately, float64(int(num.val)) past int64 range differs on arm64 and amd64: arm64 accepts 2^63. Could this bound num.val before the conversion and only allow 1, 5, 10, 20, 30 or a multiple of 60 up to a sane cap?
There was a problem hiding this comment.
Fixed in 5d8c9c1. searchPeriod now checks the float before converting it (< 1, > math.MaxInt32, not whole), then applies the MetricStat Period rule: 1, 5, 10, 20, 30 or a multiple of 60. That rule now lives in metricmath.ValidPeriod, and the provider's ValidatePeriod from #1703 calls it, so there is one definition. An out-of-range period is a parse error, so the SEARCH reads nothing, and GetMetricData rejects the expression with ValidationError: Error in expression 'e1': Invalid SEARCH period. .... Because the check happens before the conversion, 2^63 is rejected on arm64 and amd64 alike.
| return nil | ||
| } | ||
|
|
||
| step := time.Duration(period) * time.Second |
There was a problem hiding this comment.
This multiplication overflows for a large period, and a negative step passes the maxFillPoints check (the quotient is negative), so the loop below never ends. Counting the periods in seconds first, e.g. int64(e.end.Sub(e.start)/time.Second)/int64(period) > maxFillPoints, plus a step <= 0 guard, would make grid safe whatever reaches it.
There was a problem hiding this comment.
Fixed in 5d8c9c1. grid now counts in whole seconds (last := int64(e.end.Sub(e.start)/time.Second) / int64(period)), checks last against maxFillPoints, and builds each point as start + i*period seconds. Every offset is at most the length of the range, so it can't overflow or loop for any period, including one that comes from an entry Period or a MetricStat. TestFILLHugeEntryPeriod passes 9223372037, 2^55, 2^62 and MaxInt64 as the entry Period, with a 5s timeout guard. Before the fix those cases hung or panicked with divide by zero.
| } | ||
|
|
||
| return seriesSeries(op, left.series, right.series) | ||
| return result{isArray: true, array: out} |
There was a problem hiding this comment.
The period from a SEARCH is dropped here, so FILL(SEARCH('{Rev/App,InstanceId}','Sum',300)*2, 0) with no query Period comes back unfilled (one point per series). Copying the input's period onto the mapped result would cover the arithmetic-over-SEARCH case too.
There was a problem hiding this comment.
Fixed in 5d8c9c1 (L7). mapArray now takes the input result and copies its period onto the mapped array, so SEARCH(...)*2 and 2*SEARCH(...) keep the SEARCH step. FILL's array branch reuses it. Covered by TestFILLOverSEARCHArithmeticKeepsPeriod (unit) and a new arithmetic over SEARCH case in TestSDKGetMetricDataFILLOverSEARCHNoPeriod. Live with no query Period, FILL(SEARCH('{Rev/App,InstanceId}','Sum',300)*2,0) now returns 4 points per series.
| want := map[string][]float64{"diff": {10, 20, -20, 0}, "ratio": {1.5, 0}} | ||
|
|
||
| for _, r := range out.MetricDataResults { | ||
| id := aws.ToString(r.Id) |
There was a problem hiding this comment.
If diff or ratio were missing from the response this loop would still pass. Could it assert the result count, or iterate over want?
There was a problem hiding this comment.
Fixed in 5d8c9c1 (L8). The test now collects the results by Id, checks that the count matches want, and loops over want, so a missing diff or ratio fails.
…dashboards GetDashboard and DeleteDashboards list DashboardNotFoundError (awsQuery code ResourceNotFound, HTTP 404) in the CloudWatch model. The rpc-v2-cbor and JSON codecs wrote __type ResourceNotFound, so aws-sdk-go-v2 fell back to a generic API error instead of types.DashboardNotFoundError. Write the shape name in __type and the query code in X-Amzn-Query-Error; the query/XML code stays ResourceNotFound, which is already correct. Part of stackshy#1520
…ors in metric math
FILL, IF and SEARCH were stored but parsed as unsupported, so GetMetricData
and metric-math alarms returned no data for them. Implement them following
the CloudWatch user guide (Use metric math, search expression syntax):
- comparison (== != < <= > >=) and logical (AND/&&, OR/||) operators, with a
point missing from one series read as 0
- IF(cond, a[, b]) with the documented per-point rules and tables
- FILL(x, S|TS|REPEAT|LINEAR) over the periods of the query range
- SEARCH(' {schema} term ', 'Stat'[, period]) over metrics with data in the
past two weeks; each found metric is its own GetMetricData result row
Part of stackshy#1520
…-goals GetMetricWidgetImage renders a PNG graph, which the emulator does not do. Record it, the metric math functions that are evaluated, the labels the AWS docs leave unstated, and the newer AnomalyDetectorId surface as non-goals. Part of stackshy#1520
Pointer params for the 96-byte result, operator and keyword constants, smaller parser and search lexer functions, and drop the nolint:dupl on deleteMetricStream that the dashboard change made unused. Part of stackshy#1520
…s at FILL took its grid from the entry period, which never saw a SEARCH's own period argument, so FILL(SEARCH(..., 300), 0) on an entry with no Period returned the sparse series unchanged. A SEARCH now reports the period it reads at (its argument, else the entry period, else 60s) and FILL uses it. Part of stackshy#1520
The user guide's arithmetic rule is "Missing values in a time series are treated as 0. If the value of a data point causes a function to attempt to divide by zero, the data point is dropped." Arithmetic between two series dropped any timestamp only one series had, and x/0 returned 0. Both series now combine over the union of their timestamps with a missing point as 0, and a division by zero drops the point. Part of stackshy#1520
…verflow-safe A SEARCH period argument past the time.Duration range made the FILL grid step wrap negative (endless loop) or to exactly zero (divide by zero). - SEARCH's period argument now follows the MetricStat Period rule (1, 5, 10, 20, 30 or a multiple of 60, up to the Integer maximum), checked on the float before the int conversion. The rule moves to metricmath.ValidPeriod and the provider's ValidatePeriod from stackshy#1703 uses it, so there is a single definition. - GetMetricData rejects such an expression with a ValidationError ("Error in expression 'e1': Invalid SEARCH period. ..."). - The FILL grid counts in whole seconds and never turns the period itself into a Duration, so it ends for any period, including an entry Period passed by a library caller. - Arithmetic over a SEARCH keeps the SEARCH period, so FILL around it fills every period when the query sets no Period. - The arithmetic wire test now checks every expected row comes back.
ad761b7 to
5d8c9c1
Compare
|
Thanks for catching this, @NitinKumar004. Head is now Rebase. The branch is rebased onto the current
H1, fixed in
Lows
Checks.
|
golangci-lint v2.14 reports prealloc on the two slices built while matching SEARCH terms. Size them up front; behaviour is unchanged.
|
One more small commit, 0f7d113: with golangci-lint v2.14.0 (full run, no |
Summary
Part of #1520 (AWS CloudWatch real-cloud parity gaps).
types.DashboardNotFoundErrorin the SDK (rpc-v2-cbor and JSON).FILL,IF,SEARCH, and the comparison and logical operators. Before, these returned no data.FILL(SEARCH(...))now fills on the period the SEARCH reads at (M1).GetMetricWidgetImageand the newerAnomalyDetectorIdsurface are recorded as non-goals.Per-item status
DashboardNotFoundError. That is the shape name. Its wire code isResourceNotFoundwith HTTP 404 (botocorecloudwatch/2010-08-01/service-2.json; the GetDashboard API reference listsResourceNotFound, 404). The query/XML code was already correct. The real gap was in rpc-v2-cbor and JSON: they put__type: ResourceNotFound, but aws-sdk-go-v2 cloudwatch v1.56.2 matches the shape name, soerrors.Asnever foundtypes.DashboardNotFoundError.ANOMALY_DETECTION_BAND(m1[,k])already returned the lower and upper rows (metricDataRows,TestGetMetricDataAnomalyBand). FILL, IF and SEARCH failed to parse, so they returned no data.docs/coverage/nongoals/monitoring.md.Changes
Dashboard errors (
server/aws/cloudwatch, wire translation only):wireErrorgets an optionalshape.dashboardErrturns a store NotFound into codeResourceNotFound, shapeDashboardNotFoundError, status 404.__typeandResourceNotFound;SenderinX-Amzn-Query-Error.ErrorCode()therefore staysResourceNotFound, and the CLI still showsResourceNotFound.Metric math (
services/monitoring/metricmath). Onlyproviders/aws/cloudwatchandserver/aws/cloudwatchimport this package. Semantics come from Use metric math and the search expression syntax / examples:== != < <= > >=and logical operatorsAND/&&,OR/||. When a timestamp exists in only one series, the other series counts as 0 there.x/0returned 0.IF(cond, a[, b])follows the documented per-point rules; all three IF tables from the guide are unit-tested verbatim.FILL(x, S | TS | REPEAT | LINEAR)fills the periods of the query range[StartTime, EndTime). The step is the period of its input: the period a SEARCH reads at (its argument, else the entryPeriod, else the 60s a period-less read uses), and otherwise the expression period.SEARCH(' {Namespace,Dim...} term ', 'Stat'[, period])covers:"..."matching,Name=valuedesignators, andAND/OR/NOTwith parenthesesmetricmath.ValidPeriod, which the provider'sValidatePeriod(fix(aws-cloudwatch): validate metric Period on GetMetricData and GetMetricStatistics #1703) also uses. GetMetricData rejects an invalid SEARCH period withValidationError: Error in expression '<id>': Invalid SEARCH period. ....Docs: new
docs/coverage/nongoals/monitoring.md, plus the regenerateddocs/coverage. No driver interface changed.Provider Coverage
Checklist
go test ./...)golangci-lint run, compared withdevelopment)cloudemu_test.go(covered by aws-sdk-go-v2 wire tests inserver/aws/cloudwatch)go generate ./...) and committed the resultTest Plan
Each new test fails on the code before its fix and passes after:
TestSDKDashboardNotFoundShape(aws-sdk-go-v2):errors.Asfindstypes.DashboardNotFoundError,ErrorCode()=="ResourceNotFound", 404.TestJSONProtocolErrorsexpects__type: DashboardNotFoundErrorwithX-Amzn-Query-Error: ResourceNotFound;Sender.TestSDKGetMetricDataFILLAndIFandTestSDKGetMetricDataSEARCH(aws-sdk-go-v2).services/monitoring/metricmath/functions_test.go:TestIFExpressions,TestComparisonAndLogicalOperators,TestFILL*,TestSEARCH,TestSEARCHReadsEachMetric,TestFILLOverSEARCH,TestFunctionReferences.TestSDKGetMetricDataFILLOverSEARCHNoPeriod(wire):FILL(SEARCH(..., 300), 0)andFILL(SEARCH(...), 0)with noPeriodon the query.TestFILLOverSEARCHUsesSearchPeriod(unit).TestSDKGetMetricDataArithmeticMissingAsZero(wire) andTestArithmeticMissingAsZero(unit), using the guide's metric1[30,20,0,0]and metric2[20,-,20,-].m1 - m2gives[10,20,-20,0](before:[10,-20]), andm1 / m2gives[1.5, 0].TestSeriesAlignByTimestampand the divide-by-zero case inTestEvaluateExpressionsnow expect the documented results.TestFILLOverSEARCHHugePeriod(unit):FILL(SEARCH('{Rev/App,InstanceId}','Sum',9223372037),0), a 2^55 period and 2^63. Before the fix: an endless loop, or a divide-by-zero panic.TestFILLHugeEntryPeriod(unit): FILL with an entry Period of 9223372037, 2^55, 2^62 or MaxInt64. Before the fix: a hang or a panic.TestInvalidSearchPeriod,TestValidPeriod(unit).TestGetMetricDataSearchPeriodValidation(query protocol, 10s client timeout) andTestSDKGetMetricDataSearchPeriodValidation(aws-sdk-go-v2): 400ValidationError. Before the fix, the query request hung until the client timeout.TestFILLOverSEARCHArithmeticKeepsPeriod(unit) and thearithmetic over SEARCHcase ofTestSDKGetMetricDataFILLOverSEARCHNoPeriod.Checks:
go build ./...go veton the touched packagesgo test ./...(all 660 packages) andgo test -raceonservices/monitoring/...,server/aws/cloudwatch,providers/aws/cloudwatchandpersistgo generate ./...leaves no diffgolangci-lint run(no--new-from-rev) onservices/monitoring/...,server/aws/cloudwatch/...andproviders/aws/cloudwatch/...: 10 issues on the branch versus 11 ondevelopment, all pre-existing. The branch removes onedupland adds none.Real-user test:
cloudemu serve -aws-port 4610 ...in the background, driven with AWS CLI 1.44.87 / botocore 1.42.97 at--endpoint-url http://127.0.0.1:4610 --region us-east-1:Follow-ups (from review, not in this PR)
__typeof every CBOR error that existing parity tests assert, andValidationErrorhas no model shape, so it deserves its own PR. Tracked in aws-cloudwatch: map typed SDK errors over rpc-v2-cbor (writeCBORError) #1716.Related Issues
Part of #1520