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: Allow switching between Datadog v1 and v2. Fixes #2549 #2592

Merged
merged 18 commits into from
Mar 3, 2023

Conversation

daniddelrio
Copy link
Contributor

@daniddelrio daniddelrio commented Feb 15, 2023

Since Datadog will deprecate the legacy v1 api, Rollouts should now use the v2 API version. Datadog imposes stricter rate limits on v1 because of this. This PR introduces an apiVersion field in the Datadog configuration that can allow users to toggle between v1 and v2.

Addresses: #2549
Relates to: #2580

Slack name: Dani Del Rio

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.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2023

Go Published Test Results

1 871 tests   1 871 ✔️  2m 31s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit 1e446dd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2023

E2E Tests Published Test Results

    2 files      2 suites   1h 38m 51s ⏱️
  96 tests   91 ✔️ 3 💤 2
194 runs  186 ✔️ 6 💤 2

For more details on these failures, see this check.

Results for commit 1e446dd.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Patch coverage: 76.04% and project coverage change: -0.04 ⚠️

Comparison is base (96a1b67) 81.54% compared to head (1e446dd) 81.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2592      +/-   ##
==========================================
- Coverage   81.54%   81.50%   -0.04%     
==========================================
  Files         133      133              
  Lines       19768    19852      +84     
==========================================
+ Hits        16120    16181      +61     
- Misses       2820     2836      +16     
- Partials      828      835       +7     
Impacted Files Coverage Δ
metricproviders/datadog/datadog.go 75.98% <76.04%> (-1.95%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zachaller
Copy link
Collaborator

I have a few questions the upgrade makes sense I am just not sure we want to break backwards compatibility by removing v1. Does datadog have an end date set for when they will stop supporting v1? If that date is still pretty far out do you think it would make sense to support both versions via a config option?

@daniddelrio
Copy link
Contributor Author

I have a few questions the upgrade makes sense I am just not sure we want to break backwards compatibility by removing v1. Does datadog have an end date set for when they will stop supporting v1? If that date is still pretty far out do you think it would make sense to support both versions via a config option?

I can't find anything on a timeline that Datadog will follow for stopping support of v1 completely, but there are already some endpoints that they've deprecated, like for Dashboards and Usage Attribution.

The primary benefit for this is to mitigate the rate limit errors that some have been encountering. We personally have encountered this rate limit and there's also this #2578. We can introduce a config option, but I'm not sure if that'll be the best idea given that we'll eventually move away from v1 anyway.

@zachaller
Copy link
Collaborator

The primary benefit for this is to mitigate the rate limit errors that some have been encountering. We personally have encountered this rate limit and there's also this #2578. We can introduce a config option, but I'm not sure if that'll be the best idea given that we'll eventually move away from v1 anyway.

Yup I agree 100% I like retries in general but I also agree moving to v2 is a good thing I just don't want to force users to have to upgrade we need to be mindful of backward compatibility and if users are not ready to migrate we need to support both still.

I am thinking something like apiVersion: v2 and we default to v1 and then have documentation the we recommend v2 for x reasons and we can start deprecating v1 over time as well and eventually switch to v2 as a default, thoughts?

provider:
      datadog:
        apiVersion: v2
        interval: 5m
        query: |
          sum:requests.error.count{service:{{args.service-name}}} /
          sum:requests.request.count{service:{{args.service-name}}}

@zachaller
Copy link
Collaborator

I would also maybe add a log line that v1 is deprecated as well

@daniddelrio
Copy link
Contributor Author

I think that would be a good idea. I can try to implement that

@daniddelrio
Copy link
Contributor Author

@zachaller it seems that the checks are failing because of the generated files. I did see this but I wasn't sure about how to update them. How can I fix these?

@daniddelrio daniddelrio changed the title feat: Upgrade datadog API version. Fixes #2549 feat: Introduce field that allows switching between Datadog v1 and v2. Fixes #2549 Feb 22, 2023
@zachaller
Copy link
Collaborator

zachaller commented Feb 22, 2023

@zachaller it seems that the checks are failing because of the generated files. I did see this but I wasn't sure about how to update them. How can I fix these?

@daniddelrio run make codegen

@zachaller zachaller changed the title feat: Introduce field that allows switching between Datadog v1 and v2. Fixes #2549 feat: Allow switching between Datadog v1 and v2. Fixes #2549 Mar 2, 2023
@sonarcloud
Copy link

sonarcloud bot commented Mar 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

No Coverage information No Coverage information
22.0% 22.0% Duplication

Queries []map[string]string `json:"queries"`
}

type datadogQuery struct {
Copy link
Member

Choose a reason for hiding this comment

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

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