fix(metricprovider): support Datadog v2 API Fixes #2813 #2997
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2997 +/- ##
==========================================
+ Coverage 81.78% 81.82% +0.03%
==========================================
Files 134 134
Lines 20459 20507 +48
==========================================
+ Hits 16733 16780 +47
- Misses 2863 2864 +1
Partials 863 863
☔ View full report in Codecov by Sentry. |
meeech
force-pushed
the
2813-datadog-v2-api
branch
2 times, most recently
from
41ae1e2
to
417866a
Compare
meeech
changed the title
meeech
force-pushed
the
2813-datadog-v2-api
branch
from
d4cf3e7
to
26804b4
Compare
meeech
changed the title
meeech
force-pushed
the
2813-datadog-v2-api
branch
3 times, most recently
from
56d2a66
to
9701e9e
Compare
meeech
force-pushed
the
2813-datadog-v2-api
branch
3 times, most recently
from
77ed492
to
128e409
Compare
meeech
commented
Sep 21, 2023
meeech
force-pushed
the
2813-datadog-v2-api
branch
3 times, most recently
from
46ca1a9
to
fdb3a3a
Compare
meeech
force-pushed
the
2813-datadog-v2-api
branch
from
fdb3a3a
to
befeb4d
Compare
meeech
force-pushed
the
2813-datadog-v2-api
branch
2 times, most recently
from
988cbf1
to
7ba20d7
Compare
meeech
commented
Oct 4, 2023
meeech
force-pushed
the
2813-datadog-v2-api
branch
2 times, most recently
from
b11893c
to
2f2e291
Compare
Signed-off-by: mitchell amihod <[email protected]>
…H in the call. Signed-off-by: mitchell amihod <[email protected]>
* Expand working with v2 information * Contacted Datadog to get latest info re: v1 deprecation, api limits. * Add tips about rate limits, using helm for templates * Add more example templates Signed-off-by: mitchell amihod <[email protected]>
* Make Query omitempty since now possible it won't exist * Add some descriptions * Add new properties we need for v2 Queries: We can pass in key:query for queries Formula: Makes formulas using the keys from queries * Defaults! Use annotations to declare defaults for some fields. This lets us remove some guard rails from the code itself Interval: 5m - Move this from code to here ApiVersion: v1 - Move this from code to here * Enums! Much like defaults, having enums lets us make assumptions about the incoming metric so we dont need as many guardrails. ApiVersion: Enum to restrict to v1 or v2 Signed-off-by: mitchell amihod <[email protected]>
Everything looks ok. Signed-off-by: mitchell amihod <[email protected]>
Validating the metric on initialization, rather than spread out throughout. You get earlier feedback if you have a bad metric defined. (Not perfect, but there's limitations with our annotation generator for the rules in the crd. eg: If we could use oneOf, we wouldn't need a lot of this validation) We check all the mutually exclusive props. The props where one requires another. We don't have to check for defaults and set them anymore, since they are guaranteed by the crd. rules: - ensure we have only query OR queries - restrict v1 to query only - make sure you only provide a formula with queries - make sure multiple queries are accompanied by a formula Signed-off-by: mitchell amihod <[email protected]>
ApiVersion is guaranteed to have value, and the enum ensures its v1/v2 when user provided. Updated v1 tests to reflect some of these new realities Signed-off-by: mitchell amihod <[email protected]>
run was getting a bit long according to the checks Signed-off-by: mitchell amihod <[email protected]>
It is a straightline to initialize since default is set to 5m for incoming metrics where it is not set. Signed-off-by: mitchell amihod <[email protected]>
Split into createRequest v1/v2 v1 : pretty much unchanged. just extracted v2: support for v2/query/scalar We don't need all the timeseries. I did some testing fetching both scalar and timeseries, and they pretty much lined up. Also confirmed with DD: From support ticket with DD: "...I have also tested the scalar api endpoint with the last aggregator as well as the timeseries api endpoint. They do indeed return the same values when retrieving the values via the api endpoints. Observing the time it takes to retrieve the values, they remain relatively the same..." re: query + v2: Keep backwards compat. if we get in a query, we turn it into a queries object to pass on to requestv2 queries into the QueriesPayload. Signed-off-by: mitchell amihod <[email protected]>
* update the datadogResponseV2 for scalar values * handle no results so it has parity with v1 - empty will now usually result in `[]` unless something goes very wrong on dd side Signed-off-by: mitchell amihod <[email protected]>
Signed-off-by: mitchell amihod <[email protected]>
* add some new test cases * update mock server to handle queries / formulas validation * update no data tests to reflect reality * stop all values being the same. it makes it difficult to know find which test case failed. move meaning from comments into the metric name. * stop using deprecated ioutil Signed-off-by: mitchell amihod <[email protected]>
meeech
force-pushed
the
2813-datadog-v2-api
branch
from
2f2e291
to
250b99e
Compare
2 tasks
|
@zachaller What do we do re: conflict with generated? |
Signed-off-by: zachaller <[email protected]>
|
@meeech I had reviewed this and it was good I see you made the only change with the error let me give it one quick go over again and merge it. |
…datadog-v2-api Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
zachaller
approved these changes
Oct 14, 2023
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #2813 #2819
Add support for Datadog v2 api.
Add new properties
queriesandformula.Update the docs with some more examples, more details around v2.
Update CRDs to express validation requirements, allowing us to remove some guardrails in the code.
Rationale:
given the validation at the heart of k8s objects is held to a high standard
given the context the analysis Runs exec is k8s
given the the Analysis Template has a defined spec
eg: we can then make some assumptions
apiVersion is empty, v1, v2
set some defaults.
Why not use the DD go client?
I explored it. There was no obvious benefit vs making the requests how we do now, with the drawback of having a new external dep. Which is in an unstable state - this is emitted when using the DD client api.
WARNING: Using unstable operation 'v2.QueryTimeseriesData'"v1 vs v2
That being said, I do have a request out to Datadog to clarify the future of v1 api so we can definitively clear up this business where we recommend moving to v2 because of impending deprecation of v1, and more guidance around the rate limits.(as of sept 20, 2023)
From DD re: v1 deprecation: " I received confirmation from our engineering team that the api endpoint, /api/v1/query, is not going to be deprecated in the near future."
From DD re: Rate limits: "increasing the rate limit of /api/v1/query api endpoint is possible and reviewed on a case-to-case basis."
Why scalar and not timeseries?
Looking into timeseries vs scalar. I think since we only need 1 value, we do better to hit the
v2/query/scalarendpoint instead ofv2/query/timeseries. A lot more data is being transferred for timeseries with no benefit- we throw it away.Also confirmed with DD: From support ticket with DD: "...I have also tested the scalar api endpoint with the
lastaggregator as well as the timeseries api endpoint. They do indeed return the same values when retrieving the values via the api endpoints. Observing the time it takes to retrieve the values, they remain relatively the same..."I have done similar tests and can confirm values returning remain relatively the same.
Checklist:
"fix(controller): Updates such and such. Fixes #1234".