✨ [source-datadog] refactor connector and add new stream service_level_objectives_history
#35362
Conversation
- Expose more endpoint-specific optional parameters - Include the Service Level Objectives History stream
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
octavia-squidington-iii
added
area/connectors
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
|
Also worth noting that this depends on #35341 |
🤦🏼 Thank you for cleaning it up. We're taking a look. |
| pagination_strategy: | ||
| type: "PageIncrement" | ||
| page_size: "{{ config['max_records_per_request'] }}" | ||
| start_from_page: 0 |
There was a problem hiding this comment.
If that was a string value, would the dependency PR still be required @dlecocq?
There was a problem hiding this comment.
If we set it to an int of our choosing, then they would be decoupled. I did try with "500" and the like, and it still complains. It has to be a literal yaml int in order for it to work.
FWIW, this is not a hill that I'd die on - if we wanted to hard code it for now, I think that's a sensible approach.
marcosmarxm
changed the title
service_level_objectives_history
| @@ -33,12 +33,11 @@ acceptance_tests: | |||
| # extra_records: yes | |||
| incremental: | |||
| bypass_reason: "This connector does not implement incremental sync" | |||
There was a problem hiding this comment.
| bypass_reason: "This connector does not implement incremental sync" |
I think for these tests to run, you'd need to remove bypass_reason.
| - "monitors" | ||
| - "service_level_objectives" | ||
| - "synthetic_tests" | ||
| - "users" | ||
|
|
There was a problem hiding this comment.
Thank you for inlining the spec 👏
There was a problem hiding this comment.
np - I wasn't sure if that was the convention or not, but to run read command line (which is how I did most of my iterating), it seemed to need it all in the manifest.
|
Once the CDK change gets merged in, let's run tests on this, and merge this. @dlecocq, if you have a minute, would you be open to improving a few other things in |
marcosmarxm
had a problem deploying
to
community-ci-auto
GitHub Actions
Failure
marcosmarxm
temporarily deployed
to
community-ci-auto
GitHub Actions
Inactive
marcosmarxm
had a problem deploying
to
community-ci-auto
GitHub Actions
Failure
marcosmarxm
had a problem deploying
to
community-ci-auto
GitHub Actions
Error
marcosmarxm
had a problem deploying
to
community-ci-auto
GitHub Actions
Error
marcosmarxm
had a problem deploying
to
community-ci-auto
GitHub Actions
Failure
marcosmarxm
temporarily deployed
to
community-ci-auto
GitHub Actions
Inactive
|
@marcosmarxm heads up, this is very behind master. It might be easier to cherry-pick the changes and reapply them in a fresh PR, if @dlecocq has time. |
|
Ah no I'm wrong, it's actually not bad. But, we'd need to carefully resolve the conflicts. |
marcosmarxm
had a problem deploying
to
community-ci-auto
GitHub Actions
Failure
marcosmarxm
temporarily deployed
to
community-ci-auto
GitHub Actions
Inactive
|
/format-fix
|
What
At least of the options in the DataDog connector were overloaded to be used across multiple streams. In some cases, this was appropriate (for example, the maximum number of records retrieved in a single request), and in some cases not (for example, the "
query", which may not be appropriate to use for both SLOs and logs (and other streams). In service of that, stream-specific configuration has been isolated to their own configuration groups, and imposing a naming convention (<stream>__<setting>) to differentiateservice_level_objectives__queryandlogs__query.In addition, not all of the options available on the DataDog endpoints were previously exposed. This MR adds several (for example,
monitors__tags,monitors__with_downtimes, etc.), though even this change is not exhaustive. My hope is that if this convention suits the community, we would expand it to the remaining endpoints so that Airbyte exposes more to the connector user.Lastly, this adds a new stream, the
service level objectives history.How
(Captured above)
🚨 User Impact 🚨
Breaking changes - some of the existing configuration options have been supplanted by their newer versions:
query->logs__queryqueries-> removed (it was not referenced anywhere in the manifest)Version bump - an argument could be made for a major bump, but I would suggest a minor version bump to
0.5.0.Community member or Airbyter
airbyte-cito handle this connector well to run the tests, but I'm working on it. In the meantime, there's value in submitting the MR.I'll also include some screenshots of it working in my local Airflow: