-
Notifications
You must be signed in to change notification settings - Fork 883
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
feat: Allow switching between Datadog v1 and v2. Fixes #2549 #2592
Conversation
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Codecov ReportPatch coverage:
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
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. |
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. |
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
|
I would also maybe add a log line that v1 is deprecated as well |
I think that would be a good idea. I can try to implement that |
@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 |
Signed-off-by: Daniel Del Rio <[email protected]>
…rio/argo-rollouts into upgrade-datadog-version
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Signed-off-by: Daniel Del Rio <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Queries []map[string]string `json:"queries"` | ||
} | ||
|
||
type datadogQuery struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be nested inside a data
key, according to: https://docs.datadoghq.com/api/latest/metrics/?code-lang=curl#query-timeseries-data-across-multiple-products
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:
"fix(controller): Updates such and such. Fixes #1234"
.