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

feat: Add support for aggregator type in DataDog metric provider #3293

Conversation

NaurisSadovskis
Copy link
Contributor

This pull request adds ability to override default DataDog (v2) API aggregator (last) for metrics-based queries [docs].

We stumbled upon this issue when using count-based distributed metrics and moving_rollup() function which aggregates data points in a given time period [docs]:
Screenshot_2024-01-08_at_21_06_39

Querying this data via DataDog API (and a formula b) reported the last value as 17.0:

Response: {"data":{"type":"scalar_response","attributes":{"columns":[{"values":[17.0],"name":"moving_rollup(b, 180, 'sum')","type":"number","meta":{"unit":null}}]}}}

Switching aggregator from last to sum behaves as expected.
Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@NaurisSadovskis
Copy link
Contributor Author

cc: @meeech and @zachaller since both of you seem to be involved in getting DataDog API v2 functionality out :)

Copy link
Contributor

github-actions bot commented Jan 8, 2024

Go Published Test Results

2 097 tests   2 097 ✅  2m 49s ⏱️
  118 suites      0 💤
    1 files        0 ❌

Results for commit 68cf420.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c688dd8) 81.82% compared to head (68cf420) 81.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3293      +/-   ##
==========================================
+ Coverage   81.82%   81.83%   +0.01%     
==========================================
  Files         135      135              
  Lines       20633    20636       +3     
==========================================
+ Hits        16882    16888       +6     
+ Misses       2880     2877       -3     
  Partials      871      871              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Jan 8, 2024

E2E Tests Published Test Results

  4 files    4 suites   3h 28m 30s ⏱️
106 tests  98 ✅  6 💤  2 ❌
436 runs  400 ✅ 24 💤 12 ❌

For more details on these failures, see this check.

Results for commit 68cf420.

♻️ This comment has been updated with latest results.

@zachaller zachaller added this to the v1.7 milestone Jan 9, 2024
Copy link
Contributor

@meeech meeech left a comment

Choose a reason for hiding this comment

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

Hey @NaurisSadovskis

thanks for this. Looks good overall. See my 2 comments. feel free to reach out on slack if you have any q's

manifests/crds/analysis-run-crd.yaml Show resolved Hide resolved
metricproviders/datadog/datadog_test.go Outdated Show resolved Hide resolved
@meeech
Copy link
Contributor

meeech commented Jan 9, 2024

@NaurisSadovskis One other thing I forgot - please have a look at the docs and update those to explain using this new option.

@NaurisSadovskis NaurisSadovskis force-pushed the nauris/datadog-v2-allow-selecting-aggregator branch 2 times, most recently from b3de059 to 8c4ad82 Compare January 10, 2024 11:46
@zachaller
Copy link
Collaborator

Is this ready for a final review?

@NaurisSadovskis
Copy link
Contributor Author

@zachaller @meeech - yes, this is ready for final review!

docs/analysis/datadog.md Outdated Show resolved Hide resolved
Copy link
Contributor

@meeech meeech left a comment

Choose a reason for hiding this comment

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

@NaurisSadovskis Thanks again for doing this pr. Sorry about the delay on getting back to it.

Looks 👍🏼

One minor nit fix in the docs, but otherwise good to go

NaurisSadovskis and others added 8 commits January 19, 2024 09:46
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
Co-authored-by: mitchell amihod <mitchell@amihod.com>
Signed-off-by: Nauris <nauris.sadovskis@wave.com>
@NaurisSadovskis NaurisSadovskis force-pushed the nauris/datadog-v2-allow-selecting-aggregator branch from c2cab30 to 8c9b882 Compare January 19, 2024 08:46
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@zachaller zachaller merged commit ae3a53a into argoproj:master Jan 26, 2024
23 checks passed
@zachaller
Copy link
Collaborator

zachaller commented Jan 26, 2024

Thank you for the contribution!

@NaurisSadovskis NaurisSadovskis deleted the nauris/datadog-v2-allow-selecting-aggregator branch January 30, 2024 14:29
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.

3 participants