Skip to content

Update Cloudwatch client to ensure limits in query#10848

Merged
zachmargolis merged 3 commits intomainfrom
margolis-query-cloudwatch-error
Jun 21, 2024
Merged

Update Cloudwatch client to ensure limits in query#10848
zachmargolis merged 3 commits intomainfrom
margolis-query-cloudwatch-error

Conversation

@zachmargolis
Copy link
Contributor

Why: Without a "| limit 10000", the script will not be able to detect missing data, so it won't return the expected results

**Why**: Without a "| limit 10000", the script will not be able
to detect missing data, so it won't return the expected results

changelog: Internal, Scripts, Update Cloudwatch querying client
@zachmargolis zachmargolis requested review from a team and rebecca-e-wright June 20, 2024 23:21
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Would it make sense to include this automatically?

@zachmargolis
Copy link
Contributor Author

Would it make sense to include this automatically?

I'm not sure if we can safely append that in 100% of queries, I'm not sure if clause order matters

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@rebecca-e-wright rebecca-e-wright left a comment

Choose a reason for hiding this comment

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

Is there any type of check that could also be put in that would make sure the "--complete" flag was also present? Not sure if thats an option since it's part of the script parameters and not the embedded cloudwatch query.

@zachmargolis
Copy link
Contributor Author

Is there any type of check that could also be put in that would make sure the "--complete" flag was also present? Not sure if thats an option since it's part of the script parameters and not the embedded cloudwatch query.

--complete is an optional flag, making it default to on seems a little trickier since not all queries are intended to have it?

@zachmargolis zachmargolis merged commit 1887caf into main Jun 21, 2024
@zachmargolis zachmargolis deleted the margolis-query-cloudwatch-error branch June 21, 2024 19:45
@zachmargolis
Copy link
Contributor Author

Would it make sense to include this automatically?

I'm not sure if we can safely append that in 100% of queries, I'm not sure if clause order matters

Following up after doing some more research for #10875, I think that anything with | stats or | dedup are also ones we can't reason about with this logic, so I do think it's good to throw an error and have a human double check the query strategy

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.

4 participants