Skip to content

Conversation

alexef
Copy link
Member

@alexef alexef commented Jul 18, 2024

the field is required by the v2 API and we currently leave it empty string.

@zachaller you removed it in #3643 and that is fine. this PR will not add it back to the schema and not break the v1 API.

without this change and without aggregator set on a v2 metric, one gets:

received non 2xx response code: 400 {"errors":["Value of parameter
            'aggregator' should be any of these [None, 'avg', 'min', 'max',
            'sum', 'last', 'mean', 'area', 'l2norm', 'percentile', 'stddev']"]}

If we set it to None, according to datadog docs it will default to avg. According to rollouts docs, we should default to last (which makes sense for analysis)

Screenshot 2024-07-18 at 11 46 40

https://docs.datadoghq.com/api/latest/metrics/#query-scalar-data-across-multiple-products

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.

the field is required by API and we currently leave it empty string

Signed-off-by: Alex Eftimie <[email protected]>
@alexef alexef changed the title bugfix: Datadog explicitly set aggregator to last fix: Datadog explicitly set aggregator to last Jul 18, 2024
@alexef alexef changed the title fix: Datadog explicitly set aggregator to last fix(datadog): explicitly set aggregator to last Jul 18, 2024
Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.29%. Comparing base (4f1edbe) to head (c9a5326).
Report is 95 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3730      +/-   ##
==========================================
+ Coverage   80.27%   80.29%   +0.02%     
==========================================
  Files         156      156              
  Lines       17964    18023      +59     
==========================================
+ Hits        14420    14472      +52     
- Misses       2631     2637       +6     
- Partials      913      914       +1     

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

Copy link
Contributor

github-actions bot commented Jul 18, 2024

Go Published Test Results

2 171 tests   2 171 ✅  2m 54s ⏱️
  119 suites      0 💤
    1 files        0 ❌

Results for commit c9a5326.

♻️ This comment has been updated with latest results.

Signed-off-by: Alex Eftimie <[email protected]>
@alexef alexef changed the title fix(datadog): explicitly set aggregator to last fix(analysis): explicitly set datadog aggregator to last Jul 18, 2024
@alexef alexef requested a review from zachaller July 18, 2024 08:38
@alexef alexef changed the title fix(analysis): explicitly set datadog aggregator to last fix(analysis): explicitly set datadog aggregator to last only on v2 Jul 18, 2024
Copy link
Contributor

E2E Tests Published Test Results

  4 files    4 suites   3h 23m 42s ⏱️
111 tests 105 ✅  6 💤 0 ❌
444 runs  420 ✅ 24 💤 0 ❌

Results for commit c9a5326.

@zachaller zachaller merged commit d962435 into argoproj:master Jul 18, 2024
@alexef alexef deleted the patch-3 branch July 18, 2024 15:10
@mickeyrouash
Copy link

@alexef I also experience it , is there a way to workaround it and still using Datadog V2 ?

@alexef
Copy link
Member Author

alexef commented Jul 23, 2024

@mickeyrouash yes, the workaround is adding aggregator: last to your analysis template (this will now do it implicitely)

nikoshet pushed a commit to nikoshet/argo-rollouts that referenced this pull request Jul 25, 2024
…rgoproj#3730)

* Datadog: explicitly set aggregator to last

the field is required by API and we currently leave it empty string

Signed-off-by: Alex Eftimie <[email protected]>

* add unit tests

Signed-off-by: Alex Eftimie <[email protected]>

---------

Signed-off-by: Alex Eftimie <[email protected]>
zachaller pushed a commit that referenced this pull request Aug 13, 2024
…3730)

* Datadog: explicitly set aggregator to last

the field is required by API and we currently leave it empty string

Signed-off-by: Alex Eftimie <[email protected]>

* add unit tests

Signed-off-by: Alex Eftimie <[email protected]>

---------

Signed-off-by: Alex Eftimie <[email protected]>
@zachaller zachaller added the cherry-pick-completed Used once we have cherry picked the PR to all requested releases label Aug 13, 2024
@aguiraorodriguez
Copy link

Is this PR addressing #3707 ?

@alexef
Copy link
Member Author

alexef commented Aug 14, 2024

@aguiraorodriguez exactly, that issue should be fixed by this PR, as part of 1.7.2 release

meeech pushed a commit to CircleCI-Public/argo-rollouts that referenced this pull request Feb 10, 2025
…rgoproj#3730)

* Datadog: explicitly set aggregator to last

the field is required by API and we currently leave it empty string

Signed-off-by: Alex Eftimie <[email protected]>

* add unit tests

Signed-off-by: Alex Eftimie <[email protected]>

---------

Signed-off-by: Alex Eftimie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-1.7 cherry-pick-completed Used once we have cherry picked the PR to all requested releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants