Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cloudwatch metrics: expression warning too simplistic #20136

Closed
robjongbloed opened this issue Apr 29, 2022 · 8 comments · Fixed by #24313 · May be fixed by WontonSam/apollo-federation-subgraph-compatibility#508, WontonSam/apollo-federation-subgraph-compatibility#511 or WontonSam/apollo-federation-subgraph-compatibility#523
Closed

cloudwatch metrics: expression warning too simplistic #20136

robjongbloed opened this issue Apr 29, 2022 · 8 comments · Fixed by #24313 · May be fixed by WontonSam/apollo-federation-subgraph-compatibility#508, WontonSam/apollo-federation-subgraph-compatibility#511 or WontonSam/apollo-federation-subgraph-compatibility#523
Assignees
@madeline-k
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@robjongbloed
Copy link

Describe the bug

CDK displays a warning for metrics expressions that are valid.

Specifically, it is trying to check that all variables used in the expression are defined, bit it does not deal with the SEARCH function, and likely not the METRICS either.

Expected Behavior

Not display a warning for valid expressions.

Current Behavior

Warning at /realtime-chime-meeting-local-997654549727-us-east-1-rjongbloed/Mon/Dash] Math expression '100 * SEARCH('{AWS/ApiGateway,ApiName,Method,Resource,Stage} ApiName=my-api MetricName=("4XXError" OR "5XXError")', 'Average', 60)' references unknown identifiers: piGateway, piName, ethod, esource, tage, piName, realtime, chime, meeting, local, rjongbloed, etricName, rror, rror, verage. Please add them to the 'usingMetrics' map.

Reproduction Steps

Create a metric using SEARCH function.

Possible Solution

  • Parse the complex expression including functions, quotes, parentheses etc.
  • Don't do the check if the expression has "SEARCH" or "METRICS" in it.
  • Remove the warning completely.

Additional Information/Context

No response

CDK CLI Version

2.22.0 (build 1db4b16)

Framework Version

1.153.1

Node.js Version

v16.15.0

OS

MacOS

Language

Typescript, Python

Language Version

No response

Other information

No response
@robjongbloed robjongbloed added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 29, 2022
@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Apr 29, 2022
@gavllew
Copy link

gavllew commented May 10, 2022

For reference, the warnings were added in #19825.

@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels May 25, 2022
@peterwoodworth peterwoodworth mentioned this issue May 25, 2022
Closed
@peterwoodworth peterwoodworth added p1 and removed p2 labels May 25, 2022
@Litee
Copy link
Contributor

Litee commented Jun 6, 2022

I understand why IDs generation is useful, but are "non-existing identifiers" warnings that useful? If we cannot do proper AST analysis for expressions, maybe we should revert that regexp-based solution? @rix0rrr

@priyajeet
Copy link
Member

priyajeet commented Jun 9, 2022 •
edited
Loading

Curious, would it be better just to have a new SearchExpression instead of overloading MathExpression?

@Litee
Copy link
Contributor

Litee commented Jun 15, 2022

As far as I understand, expressions can be complex, like m1 + SUM(SEARCH(...)).

@ilya40umov
Copy link

Does anybody know if there is at least a way to suppress these specific warning somehow? (E.g. judging by this issue #15079 it's currently not supported, especially with Java where metadata is an immutable collection, and so we can't even apply the suggested workaround). We have updated our cdk version and getting tons of these due to SEARCH expressions.

@Litee
Copy link
Contributor

Litee commented Nov 5, 2022

I would revert this feature, TBH. Its implementation did not take into account some scenarios.

@rpmcginty
Copy link

Hi, I see this issue has been assigned and the priority increased, but May 2022 was the last update. Is there a rough ETA for when this might be picked up?

@ParkerVR ParkerVR mentioned this issue Feb 23, 2023
Merged
@mergify mergify bot closed this as completed in #24313 Feb 24, 2023
mergify bot pushed a commit that referenced this issue Feb 24, 2023
@ParkerVR
fix(cloudwatch): math expressions incorrectly warn about search and m…
f3596eb 
…etrics (#24313)

Closes [#20136](#20136).

It is intended that all metric identifiers referenced in a MathExpression are included in the usingMetrics map and we will raise warnings if the customer does not follow this contract. 
However for SEARCH and METRICS queries, we can refer directly to metrics attribute values inside the query. Therefore we should not raise warnings.

Change made based on work done in 55108b9 with regex extended to a few other expressions. Looks like integ requests will not be required based on that commit.

I had some firsthand experience getting thousands of this warning message after upgrading to CDK 2 and decided it would be easier to fix than suppress the excessive warnings.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

beck3905 pushed a commit to beck3905/aws-cdk that referenced this issue Feb 28, 2023
@ParkerVR @beck3905
fix(cloudwatch): math expressions incorrectly warn about search and m…
84b35be 
…etrics (aws#24313)

Closes [aws#20136](aws#20136).

It is intended that all metric identifiers referenced in a MathExpression are included in the usingMetrics map and we will raise warnings if the customer does not follow this contract. 
However for SEARCH and METRICS queries, we can refer directly to metrics attribute values inside the query. Therefore we should not raise warnings.

Change made based on work done in aws@55108b9 with regex extended to a few other expressions. Looks like integ requests will not be required based on that commit.

I had some firsthand experience getting thousands of this warning message after upgrading to CDK 2 and decided it would be easier to fix than suppress the excessive warnings.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
homakk pushed a commit to homakk/aws-cdk that referenced this issue Mar 28, 2023
@ParkerVR @homakk
fix(cloudwatch): math expressions incorrectly warn about search and m…
b98bbd3 
…etrics (aws#24313)

Closes [aws#20136](aws#20136).

It is intended that all metric identifiers referenced in a MathExpression are included in the usingMetrics map and we will raise warnings if the customer does not follow this contract. 
However for SEARCH and METRICS queries, we can refer directly to metrics attribute values inside the query. Therefore we should not raise warnings.

Change made based on work done in aws@55108b9 with regex extended to a few other expressions. Looks like integ requests will not be required based on that commit.

I had some firsthand experience getting thousands of this warning message after upgrading to CDK 2 and decided it would be easier to fix than suppress the excessive warnings.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This was referenced Sep 13, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@madeline-k madeline-k

Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Milestone
No milestone
8 participants
@Litee @priyajeet @gavllew @ilya40umov @rpmcginty @peterwoodworth @robjongbloed @madeline-k