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.

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

Add support for datadog query function wrappers - WIP #2766

Closed

Conversation

dalgibbard
Copy link

@dalgibbard dalgibbard commented Mar 16, 2022

Allow the Datadog Scaler to parse/validate queries which contain a wrapping function; eg:

per_second(sum:system.cpu.user{*}.rollup(avg, 30))

Previously, the code would see these as missing an aggregator; and prefix them with avg: resulting in an invalid query of:

avg:per_second(sum:system.cpu.user{*}.rollup(avg, 30))

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2761

Signed-off-by: Darren Gibbard <[email protected]>
Signed-off-by: Darren Gibbard <[email protected]>
@dalgibbard dalgibbard requested a review from a team as a code owner March 16, 2022 11:06
@dalgibbard dalgibbard changed the title Add support for datadog query function wrappers [DO NOT MERGE] Add support for datadog query function wrappers Mar 16, 2022
@JorTurFer JorTurFer changed the title [DO NOT MERGE] Add support for datadog query function wrappers [DO NOT MERGE] Add support for datadog query function wrappers - WIP Mar 16, 2022

func init() {
aggregator = regexp.MustCompile(`^(avg|sum|min|max):.*`)
aggregator = regexp.MustCompile(`^([\w]+\()?(avg|sum|min|max):.*`)
Copy link
Member

@zroubalik zroubalik Mar 16, 2022

Choose a reason for hiding this comment

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

Pardon my ignorance, just looking at all these regexes, isn't there any validator in any Datadog library? That could be used here. What is being used in Datadog to validate this?

Copy link
Author

Choose a reason for hiding this comment

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

An excellent question; will check!

Copy link
Author

Choose a reason for hiding this comment

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

Had a quick look around, plenty for submitting metrics etc, but not a lot in the way of validating if a metrics query is valid.

The Datadog UI posts to https://app.datadoghq.com/api/v2/query/timeseries when changing queries interactively, and returns 400 when it's invalid - but seeing as this requires auth anyway, if going down that route, you may as well skip validation in the Scaler, and just handle errors when they come back from the upstream.

@dalgibbard dalgibbard changed the title [DO NOT MERGE] Add support for datadog query function wrappers - WIP Add support for datadog query function wrappers - WIP Mar 16, 2022
@dalgibbard
Copy link
Author

Closing this, as it makes sense to not validate in the code, and instead use the upstream API.

@dalgibbard dalgibbard closed this Mar 16, 2022
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.

Datadog query with functions never returns healthy
2 participants